| Message ID | 20170718120347.GA25342@amt.cnet (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 18, 2017 at 09:03:47AM -0300, Marcelo Tosatti wrote: > > wait_lock for mmu_lock, being allocated in kmalloc memory, > can't be tracked by LOCKDEP. > > Disable LOCKDEP verification for it. > > Fixes > > INFO: trying to register non-static key. > the code is fine but needs lockdep annotation. > turning off the locking correctness validator. > CPU: 0 PID: 12386 Comm: qemu-kvm Not tainted 3.10.0-631.rt56.546.el7.x86_64.debug #1 > Hardware name: To be filled by O.E.M. To be filled by O.E.M./PMH61ML, BIOS 4.6.4 07/14/2011 > 0000000000000002 00000000ee340fa8 ffff88008e2d7bc0 ffffffffa57534fc > ffff88008e2d7bd0 ffffffffa574ce9f ffff88008e2d7c50 ffffffffa510e565 > ffff88008e2d7bf8 00000001a50cf83d 0000000000000000 ffff880000000000 > Call Trace: > [<ffffffffa57534fc>] dump_stack+0x19/0x1b > [<ffffffffa574ce9f>] register_lock_class.part.27+0x38/0x3c > [<ffffffffa510e565>] __lock_acquire+0xcc5/0xdc0 > [<ffffffffa510efb2>] lock_acquire+0xb2/0x230 > [<ffffffffa5759ccd>] ? rt_spin_lock_slowlock+0x6d/0x3c0 > [<ffffffffa575c5f0>] _raw_spin_lock_irqsave+0x70/0xc0 > [<ffffffffa5759ccd>] ? rt_spin_lock_slowlock+0x6d/0x3c0 > [<ffffffffa5759ccd>] rt_spin_lock_slowlock+0x6d/0x3c0 > [<ffffffffa575b7ac>] rt_spin_lock+0x2c/0x60 > [<ffffffffc09b387f>] kvm_page_track_register_notifier+0x1f/0x60 [kvm] > [<ffffffffc099a8eb>] kvm_mmu_init_vm+0x2b/0x30 [kvm] > [<ffffffffc0987e04>] kvm_arch_init_vm+0x264/0x290 [kvm] > [<ffffffffc096a14e>] kvm_dev_ioctl+0xde/0x740 [kvm] > [<ffffffffa5250d45>] do_vfs_ioctl+0x365/0x5b0 > [<ffffffffa5251031>] SyS_ioctl+0xa1/0xc0 > [<ffffffffa57656ed>] tracesys+0xdd/0xe2 > --------------------------- > | preempt count: 00000001 ] > | 1-level deep critical section nesting: > ---------------------------------------- > .. [<ffffffffa575c5cd>] .... _raw_spin_lock_irqsave+0x4d/0xc0 > .....[<ffffffffa5759ccd>] .. ( <= rt_spin_lock_slowlock+0x6d/0x3c0) > > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 7e80f62..6375db8 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -613,6 +613,8 @@ static struct kvm *kvm_create_vm(unsigned long type) > return ERR_PTR(-ENOMEM); > > spin_lock_init(&kvm->mmu_lock); > + > + lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock); > mmgrab(current->mm); > kvm->mm = current->mm; > kvm_eventfd_init(kvm); This doesn't make sense... It looks like a spin_lock_init() is missing, at which point it'll try and use the lock address itself and then bails because that is in dynamic memory. That doesn't mean you cannot initialize it properly. And using novalidate for this is absolutely bonkers.
On Tue, Jul 18, 2017 at 02:16:06PM +0200, Peter Zijlstra wrote: > On Tue, Jul 18, 2017 at 09:03:47AM -0300, Marcelo Tosatti wrote: > > > > wait_lock for mmu_lock, being allocated in kmalloc memory, > > can't be tracked by LOCKDEP. > > > > Disable LOCKDEP verification for it. > > > > Fixes > > > > INFO: trying to register non-static key. > > the code is fine but needs lockdep annotation. > > turning off the locking correctness validator. > > CPU: 0 PID: 12386 Comm: qemu-kvm Not tainted 3.10.0-631.rt56.546.el7.x86_64.debug #1 > > Hardware name: To be filled by O.E.M. To be filled by O.E.M./PMH61ML, BIOS 4.6.4 07/14/2011 > > 0000000000000002 00000000ee340fa8 ffff88008e2d7bc0 ffffffffa57534fc > > ffff88008e2d7bd0 ffffffffa574ce9f ffff88008e2d7c50 ffffffffa510e565 > > ffff88008e2d7bf8 00000001a50cf83d 0000000000000000 ffff880000000000 > > Call Trace: > > [<ffffffffa57534fc>] dump_stack+0x19/0x1b > > [<ffffffffa574ce9f>] register_lock_class.part.27+0x38/0x3c > > [<ffffffffa510e565>] __lock_acquire+0xcc5/0xdc0 > > [<ffffffffa510efb2>] lock_acquire+0xb2/0x230 > > [<ffffffffa5759ccd>] ? rt_spin_lock_slowlock+0x6d/0x3c0 > > [<ffffffffa575c5f0>] _raw_spin_lock_irqsave+0x70/0xc0 > > [<ffffffffa5759ccd>] ? rt_spin_lock_slowlock+0x6d/0x3c0 > > [<ffffffffa5759ccd>] rt_spin_lock_slowlock+0x6d/0x3c0 > > [<ffffffffa575b7ac>] rt_spin_lock+0x2c/0x60 > > [<ffffffffc09b387f>] kvm_page_track_register_notifier+0x1f/0x60 [kvm] > > [<ffffffffc099a8eb>] kvm_mmu_init_vm+0x2b/0x30 [kvm] > > [<ffffffffc0987e04>] kvm_arch_init_vm+0x264/0x290 [kvm] > > [<ffffffffc096a14e>] kvm_dev_ioctl+0xde/0x740 [kvm] > > [<ffffffffa5250d45>] do_vfs_ioctl+0x365/0x5b0 > > [<ffffffffa5251031>] SyS_ioctl+0xa1/0xc0 > > [<ffffffffa57656ed>] tracesys+0xdd/0xe2 > > --------------------------- > > | preempt count: 00000001 ] > > | 1-level deep critical section nesting: > > ---------------------------------------- > > .. [<ffffffffa575c5cd>] .... _raw_spin_lock_irqsave+0x4d/0xc0 > > .....[<ffffffffa5759ccd>] .. ( <= rt_spin_lock_slowlock+0x6d/0x3c0) > > > > > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 7e80f62..6375db8 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -613,6 +613,8 @@ static struct kvm *kvm_create_vm(unsigned long type) > > return ERR_PTR(-ENOMEM); > > > > spin_lock_init(&kvm->mmu_lock); > > + > > + lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock); > > mmgrab(current->mm); > > kvm->mm = current->mm; > > kvm_eventfd_init(kvm); > > This doesn't make sense... It looks like a spin_lock_init() is missing, > at which point it'll try and use the lock address itself and then bails > because that is in dynamic memory. Do you see the spin_lock_init just above, after "return PTR_ERR(-ENOMEM)"... That should take care of wait_lock i suppose. "struct kvm" (which contains the mmu_lock spinlock) is allocated with kmalloc, can LOCKDEP handle spinlocks in kmalloc'ed memory? > That doesn't mean you cannot initialize it properly. > > And using novalidate for this is absolutely bonkers. I imagined so, not entirely sure what is the proper LOCKDEP annotation in this case...
On Tue, Jul 18, 2017 at 09:22:38AM -0300, Marcelo Tosatti wrote: > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > > index 7e80f62..6375db8 100644 > > > --- a/virt/kvm/kvm_main.c > > > +++ b/virt/kvm/kvm_main.c > > > @@ -613,6 +613,8 @@ static struct kvm *kvm_create_vm(unsigned long type) > > > return ERR_PTR(-ENOMEM); > > > > > > spin_lock_init(&kvm->mmu_lock); > > > + > > > + lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock); > > > mmgrab(current->mm); > > > kvm->mm = current->mm; > > > kvm_eventfd_init(kvm); > > > > This doesn't make sense... It looks like a spin_lock_init() is missing, > > at which point it'll try and use the lock address itself and then bails > > because that is in dynamic memory. > > Do you see the spin_lock_init just above, after "return > PTR_ERR(-ENOMEM)"... That should take care of wait_lock i suppose. D'0h... so much for being 'awake'.. > "struct kvm" (which contains the mmu_lock spinlock) is allocated with > kmalloc, can LOCKDEP handle spinlocks in kmalloc'ed memory? Yep, what _should_ happen is that we use a macro like: (I don't have an -RT tree handy atm) # define raw_spin_lock_init(lock) \ do { \ static struct lock_class_key __key; \ \ __raw_spin_lock_init((lock), #lock, &__key); \ } while (0) Which defines a key per callsite. So while the lock itself is in dynamic memory, the key will be static.
Hi Marcelo,
[auto build test ERROR on linux-rt-devel/for-kbuild-bot/current-stable]
url: https://github.com/0day-ci/linux/commits/Marcelo-Tosatti/kvm-lockdep-annotate-mmu_lock-wait_lock/20170719-025327
base: https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git for-kbuild-bot/current-stable
config: x86_64-allyesdebian (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
In file included from include/linux/hardirq.h:5:0,
from include/linux/kvm_host.h:10,
from arch/x86/kvm/../../../virt/kvm/kvm_main.c:21:
arch/x86/kvm/../../../virt/kvm/kvm_main.c: In function 'kvm_create_vm':
>> arch/x86/kvm/../../../virt/kvm/kvm_main.c:617:45: error: 'spinlock_t {aka struct spinlock}' has no member named 'lock'; did you mean 'rlock'?
lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
^
include/linux/lockdep.h:300:22: note: in definition of macro 'lockdep_set_class_and_name'
lockdep_init_map(&(lock)->dep_map, name, key, 0)
^~~~
arch/x86/kvm/../../../virt/kvm/kvm_main.c:617:2: note: in expansion of macro 'lockdep_set_novalidate_class'
lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +617 arch/x86/kvm/../../../virt/kvm/kvm_main.c
606
607 static struct kvm *kvm_create_vm(unsigned long type)
608 {
609 int r, i;
610 struct kvm *kvm = kvm_arch_alloc_vm();
611
612 if (!kvm)
613 return ERR_PTR(-ENOMEM);
614
615 spin_lock_init(&kvm->mmu_lock);
616
> 617 lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
618 mmgrab(current->mm);
619 kvm->mm = current->mm;
620 kvm_eventfd_init(kvm);
621 mutex_init(&kvm->lock);
622 mutex_init(&kvm->irq_lock);
623 mutex_init(&kvm->slots_lock);
624 refcount_set(&kvm->users_count, 1);
625 INIT_LIST_HEAD(&kvm->devices);
626
627 r = kvm_arch_init_vm(kvm, type);
628 if (r)
629 goto out_err_no_disable;
630
631 r = hardware_enable_all();
632 if (r)
633 goto out_err_no_disable;
634
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, Jul 18, 2017 at 02:45:04PM +0200, Peter Zijlstra wrote: > On Tue, Jul 18, 2017 at 09:22:38AM -0300, Marcelo Tosatti wrote: > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > > > index 7e80f62..6375db8 100644 > > > > --- a/virt/kvm/kvm_main.c > > > > +++ b/virt/kvm/kvm_main.c > > > > @@ -613,6 +613,8 @@ static struct kvm *kvm_create_vm(unsigned long type) > > > > return ERR_PTR(-ENOMEM); > > > > > > > > spin_lock_init(&kvm->mmu_lock); > > > > + > > > > + lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock); > > > > mmgrab(current->mm); > > > > kvm->mm = current->mm; > > > > kvm_eventfd_init(kvm); > > > > > > This doesn't make sense... It looks like a spin_lock_init() is missing, > > > at which point it'll try and use the lock address itself and then bails > > > because that is in dynamic memory. > > > > Do you see the spin_lock_init just above, after "return > > PTR_ERR(-ENOMEM)"... That should take care of wait_lock i suppose. > > D'0h... so much for being 'awake'.. > > > "struct kvm" (which contains the mmu_lock spinlock) is allocated with > > kmalloc, can LOCKDEP handle spinlocks in kmalloc'ed memory? > > Yep, what _should_ happen is that we use a macro like: > > (I don't have an -RT tree handy atm) > > # define raw_spin_lock_init(lock) \ > do { \ > static struct lock_class_key __key; \ > \ > __raw_spin_lock_init((lock), #lock, &__key); \ > } while (0) > > Which defines a key per callsite. So while the lock itself is in dynamic > memory, the key will be static. One other interesting observation: the dump_stack() comes from the acquire path, not from the _init() path, even though the _init() path itself includes a check to ensure that the lock_class_key points to static data. (__raw_spin_lock_init -> lockdep_init_map -> if (!static_obj(key)) BUG) So, that would lead me to believe that if we got this far, at some point lock->key _was_ pointing to static data, but now is not? Julia Caveat: I'm looking at v3.10.53-rt56; I guessed it was the closest public stable tag to the undecipherable RHEL version. > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marcelo,
[auto build test WARNING on linux-rt-devel/for-kbuild-bot/current-stable]
url: https://github.com/0day-ci/linux/commits/Marcelo-Tosatti/kvm-lockdep-annotate-mmu_lock-wait_lock/20170719-025327
base: https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git for-kbuild-bot/current-stable
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from include/linux/hardirq.h:5:0,
from include/linux/kvm_host.h:10,
from arch/x86/kvm/../../../virt/kvm/kvm_main.c:21:
arch/x86/kvm/../../../virt/kvm/kvm_main.c: In function 'kvm_create_vm':
arch/x86/kvm/../../../virt/kvm/kvm_main.c:617:45: error: 'spinlock_t {aka struct spinlock}' has no member named 'lock'; did you mean 'rlock'?
lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
^
include/linux/lockdep.h:300:22: note: in definition of macro 'lockdep_set_class_and_name'
lockdep_init_map(&(lock)->dep_map, name, key, 0)
^~~~
>> arch/x86/kvm/../../../virt/kvm/kvm_main.c:617:2: note: in expansion of macro 'lockdep_set_novalidate_class'
lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/lockdep_set_novalidate_class +617 arch/x86/kvm/../../../virt/kvm/kvm_main.c
606
607 static struct kvm *kvm_create_vm(unsigned long type)
608 {
609 int r, i;
610 struct kvm *kvm = kvm_arch_alloc_vm();
611
612 if (!kvm)
613 return ERR_PTR(-ENOMEM);
614
615 spin_lock_init(&kvm->mmu_lock);
616
> 617 lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock);
618 mmgrab(current->mm);
619 kvm->mm = current->mm;
620 kvm_eventfd_init(kvm);
621 mutex_init(&kvm->lock);
622 mutex_init(&kvm->irq_lock);
623 mutex_init(&kvm->slots_lock);
624 refcount_set(&kvm->users_count, 1);
625 INIT_LIST_HEAD(&kvm->devices);
626
627 r = kvm_arch_init_vm(kvm, type);
628 if (r)
629 goto out_err_no_disable;
630
631 r = hardware_enable_all();
632 if (r)
633 goto out_err_no_disable;
634
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 2017-07-18 09:03:47 [-0300], Marcelo Tosatti wrote: > > wait_lock for mmu_lock, being allocated in kmalloc memory, > can't be tracked by LOCKDEP. there is: struct kvm *kvm = kvm_arch_alloc_vm(); spin_lock_init(&kvm->mmu_lock); and kvm_arch_alloc_vm() itself is "kzalloc(sizeof(struct kvm), GFP_KERNEL);". I don't see anything wrong with it. This does not explain why this lockdep warning. > Disable LOCKDEP verification for it. > > Fixes > INFO: trying to register non-static key. > the code is fine but needs lockdep annotation. > turning off the locking correctness validator. not convinced. > CPU: 0 PID: 12386 Comm: qemu-kvm Not tainted 3.10.0-631.rt56.546.el7.x86_64.debug #1 oh. Sebastian
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 7e80f62..6375db8 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -613,6 +613,8 @@ static struct kvm *kvm_create_vm(unsigned long type) return ERR_PTR(-ENOMEM); spin_lock_init(&kvm->mmu_lock); + + lockdep_set_novalidate_class(&kvm->mmu_lock.lock.wait_lock); mmgrab(current->mm); kvm->mm = current->mm; kvm_eventfd_init(kvm);
wait_lock for mmu_lock, being allocated in kmalloc memory, can't be tracked by LOCKDEP. Disable LOCKDEP verification for it. Fixes INFO: trying to register non-static key. the code is fine but needs lockdep annotation. turning off the locking correctness validator. CPU: 0 PID: 12386 Comm: qemu-kvm Not tainted 3.10.0-631.rt56.546.el7.x86_64.debug #1 Hardware name: To be filled by O.E.M. To be filled by O.E.M./PMH61ML, BIOS 4.6.4 07/14/2011 0000000000000002 00000000ee340fa8 ffff88008e2d7bc0 ffffffffa57534fc ffff88008e2d7bd0 ffffffffa574ce9f ffff88008e2d7c50 ffffffffa510e565 ffff88008e2d7bf8 00000001a50cf83d 0000000000000000 ffff880000000000 Call Trace: [<ffffffffa57534fc>] dump_stack+0x19/0x1b [<ffffffffa574ce9f>] register_lock_class.part.27+0x38/0x3c [<ffffffffa510e565>] __lock_acquire+0xcc5/0xdc0 [<ffffffffa510efb2>] lock_acquire+0xb2/0x230 [<ffffffffa5759ccd>] ? rt_spin_lock_slowlock+0x6d/0x3c0 [<ffffffffa575c5f0>] _raw_spin_lock_irqsave+0x70/0xc0 [<ffffffffa5759ccd>] ? rt_spin_lock_slowlock+0x6d/0x3c0 [<ffffffffa5759ccd>] rt_spin_lock_slowlock+0x6d/0x3c0 [<ffffffffa575b7ac>] rt_spin_lock+0x2c/0x60 [<ffffffffc09b387f>] kvm_page_track_register_notifier+0x1f/0x60 [kvm] [<ffffffffc099a8eb>] kvm_mmu_init_vm+0x2b/0x30 [kvm] [<ffffffffc0987e04>] kvm_arch_init_vm+0x264/0x290 [kvm] [<ffffffffc096a14e>] kvm_dev_ioctl+0xde/0x740 [kvm] [<ffffffffa5250d45>] do_vfs_ioctl+0x365/0x5b0 [<ffffffffa5251031>] SyS_ioctl+0xa1/0xc0 [<ffffffffa57656ed>] tracesys+0xdd/0xe2 --------------------------- | preempt count: 00000001 ] | 1-level deep critical section nesting: ---------------------------------------- .. [<ffffffffa575c5cd>] .... _raw_spin_lock_irqsave+0x4d/0xc0 .....[<ffffffffa5759ccd>] .. ( <= rt_spin_lock_slowlock+0x6d/0x3c0) Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>