On 10/11/2011 01:32 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Oct 11, 2011 at 07:13:38PM +0200, Dario Faggioli wrote:
>> Hello everyone,
>>
>> Since I really plan to spend some time here, let me introduce myself
>> first. My name is Dario Faggioli and I just joined the Citrix Platform
>> Team in Cambridge (although I'll be working from Italy). I've some
>> experience in Linux kernel (mainly scheduling) and not that much
>> experience in Xen or virtualization in general, but I really want to
>> learn and be able to contribute ASAP!
>>
>> In fact, while "doing my homework", I stumbled against the following
>> BUG(). I'm able to reproduce it with xen-unstable and by just by
>> enabling spinlock and mutex debug checks in linus' Linux
>> (65112dccf8a113737684366349d7f9ec373ddc47) _iff_ using 'tap:qcow2' disk
>> image, while no such thing happens with 'phy:'.
>
> Aha.. I saw that at some point but never narrowed it down to the
> right combination. Thank you for tracking this down.
>
> I am CC Daniel who might have some patches for this or ideas.
>>
>> [ 996.282544] BUG: sleeping function called from invalid context at
>> /local/scratch/dariof/linux/kernel/mutex.c:271
>> [ 996.282570] in_atomic(): 1, irqs_disabled(): 0, pid: 3256, name: qemu-dm
>> [ 996.282581] 1 lock held by qemu-dm/3256:
>> [ 996.282589] #0: (&(&priv->lock)->rlock){......}, at:
>> [<ffffffff813223da>] gntdev_ioctl+0x2bd/0x4d5
>> [ 996.282628] Pid: 3256, comm: qemu-dm Tainted: G W 3.1.0-rc8+ #5
>> [ 996.282638] Call Trace:
>> [ 996.282687] [<ffffffff81054594>] __might_sleep+0x131/0x135
>> [ 996.282704] [<ffffffff816bd64f>] mutex_lock_nested+0x25/0x45
>> [ 996.282721] [<ffffffff8131c7c8>] free_xenballooned_pages+0x20/0xb1
>> [ 996.282735] [<ffffffff8132194d>] gntdev_put_map+0xa8/0xdb
>> [ 996.282749] [<ffffffff816be546>] ? _raw_spin_lock+0x71/0x7a
>> [ 996.282763] [<ffffffff813223da>] ? gntdev_ioctl+0x2bd/0x4d5
>> [ 996.282776] [<ffffffff8132243c>] gntdev_ioctl+0x31f/0x4d5
>> [ 996.282790] [<ffffffff81007d62>] ? check_events+0x12/0x20
>> [ 996.282804] [<ffffffff811433bc>] do_vfs_ioctl+0x488/0x4d7
>> [ 996.282818] [<ffffffff81007d4f>] ? xen_restore_fl_direct_reloc+0x4/0x4
>> [ 996.282832] [<ffffffff8109168b>] ? lock_release+0x21c/0x229
>> [ 996.282847] [<ffffffff81135cdd>] ? rcu_read_unlock+0x21/0x32
>> [ 996.282860] [<ffffffff81143452>] sys_ioctl+0x47/0x6a
>> [ 996.282873] [<ffffffff816bfd82>] system_call_fastpath+0x16/0x1b
>>
>> This seems to be due to free_xenballooned_pages(), called by
>> gntdev_put_map(), taking balloon_mutex, with the latter that can be
>> called within a spin_lock() (e.g., in gntdev_release()).
>>
>> I'm not enough confident with the code do attempt fixing it, but I
>> thought it was worth to at least point it out!
>
It looks like gntdev_release doesn't need to take the spinlock at all, since
it's a per-file lock on the file's release path. The unmap ioctl looks like
it'll also trigger this message; that's also fixed here. Compile tested only.
-------------------------------------------------------->8
gntdev_put_map tries to acquire a mutex when freeing pages back to the
xenballoon pool, so it cannot be called with a spinlock held. In
gntdev_release, the spinlock is not needed as we are freeing the
structure later; in the ioctl, only the list manipulation needs to be
under the lock.
Reported-By: Dario Faggioli <raistlin@xxxxxxxx>
Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
---
drivers/xen/gntdev.c | 5 ++---
1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index f914b26..23b1c83 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -462,13 +462,11 @@ static int gntdev_release(struct inode *inode, struct
file *flip)
pr_debug("priv %p\n", priv);
- spin_lock(&priv->lock);
while (!list_empty(&priv->maps)) {
map = list_entry(priv->maps.next, struct grant_map, next);
list_del(&map->next);
gntdev_put_map(map);
}
- spin_unlock(&priv->lock);
if (use_ptemod)
mmu_notifier_unregister(&priv->mn, priv->mm);
@@ -532,10 +530,11 @@ static long gntdev_ioctl_unmap_grant_ref(struct
gntdev_priv *priv,
map = gntdev_find_map_index(priv, op.index >> PAGE_SHIFT, op.count);
if (map) {
list_del(&map->next);
- gntdev_put_map(map);
err = 0;
}
spin_unlock(&priv->lock);
+ if (map)
+ gntdev_put_map(map);
return err;
}
--
1.7.6.4
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home