[PATCH v2 0/2] debugobjects: Fix potential hard lockup by disabling lockdep

[PATCH v2 0/2] debugobjects: Fix potential hard lockup by disabling lockdep

[Waiman Long]

 v2:
  - Add a lockdep patch to fix a dmesg warning message due to the use
    of the novalidate class.

The only locking used by the debugobjects code is just 2 sets of raw
spinlocks for synchronization purpose. When lockdep is enabled, the
locking operation itself will become much more expensive especially
if a number of locks have already been acquired previously. In some
extreme cases, it may lead to hard lockup.

As there isn't much value in debugging the debugobjects internal locks,
lockdep checking is now disabled for those internal locks. That will
speed up system operation without compromising the lock checking
operation of production code.

Waiman Long (2):
  locking/lockdep: Don't warn class/lock name mismatch for novalidate
    class
  debugobjects: Disable lockdep tracking of debugobjects internal locks

 kernel/locking/lockdep.c | 7 +++++--
 lib/debugobjects.c       | 9 ++++++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

-- 
2.18.0

[PATCH v2 1/2] locking/lockdep: Don't warn class/lock name mismatch for novalidate class

[Waiman Long]

For the special novalidate class (lockdep_set_novalidate_class), multiple
locks with different names may use the same class. The WARN_ON_ONCE()
check in look_up_lock_class() will then report an unnecessary warning.

The current users of lockdep_set_novalidate_class() includes the
device mutex, one mutex and one rwsem in drivers/md/bcache/btree.c.
So the novalidate class is now excluded from the check.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 kernel/locking/lockdep.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index dd13f865ad40..10babc35953b 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -693,9 +693,12 @@ look_up_lock_class(const struct lockdep_map *lock, unsigned int subclass)
 		if (class->key == key) {
 			/*
 			 * Huh! same key, different name? Did someone trample
-			 * on some memory? We're most confused.
+			 * on some memory? We're most confused unless it is
+			 * __lockdep_no_validate__ where different locks can
+			 * use the same class.
 			 */
-			WARN_ON_ONCE(class->name != lock->name);
+			WARN_ON_ONCE((key != __lockdep_no_validate__.subkeys) &&
+				     (class->name != lock->name));
 			return class;
 		}
 	}
-- 
2.18.0

[PATCH v2 2/2] debugobjects: Disable lockdep tracking of debugobjects internal locks

[Waiman Long]

It was discovered that running the ltp openposix_testsuite sigqueue-09-1
test on a certain 8-sock IvyBridge system caused it to have a hard lockup
with a full debug kernel.

 [89981.861500] NMI watchdog: Watchdog detected hard LOCKUP on cpu 17
   :
 [89981.939812] irq event stamp: 1166122
 [89981.943799] hardirqs last  enabled at (1166121): [<ffffffffb1048342>] kprobe_ftrace_handler+0x52/0x170
 [89981.954215] hardirqs last disabled at (1166122): [<ffffffffb08a3725>] tasklist_write_lock_irq+0x15/0x50
   :
 [89982.103134]  [<ffffffffb093a399>] lock_acquire+0x99/0x1e0
 [89982.109163]  [<ffffffffb0c0d19b>] ? debug_check_no_obj_freed+0xfb/0x270
 [89982.116562]  [<ffffffffb1042e1e>] _raw_spin_lock_irqsave+0x5e/0xa0
 [89982.123470]  [<ffffffffb0c0d19b>] ? debug_check_no_obj_freed+0xfb/0x270
 [89982.130851]  [<ffffffffb0c0d19b>] debug_check_no_obj_freed+0xfb/0x270
 [89982.138045]  [<ffffffffb08bf9f3>] ? __sigqueue_free.part.11+0x33/0x40
 [89982.145239]  [<ffffffffb0a6868a>] kmem_cache_free+0xca/0x390
 [89982.151553]  [<ffffffffb08bf9f3>] __sigqueue_free.part.11+0x33/0x40
 [89982.158552]  [<ffffffffb08c07b0>] flush_sigqueue+0x50/0x60
 [89982.164673]  [<ffffffffb08ad102>] release_task+0x3e2/0x6d0
   :

IRQ was disabled by the tasklist_write_lock_irq() call in
release_task(). The lockup is probably caused by the debug code running
for too long. We certainly want the debug code to verify the correctness
of the production code. However, there isn't too much value for one
piece of the debug code to verify the correctness of another piece of
debug code. In this particular case, the lockdep code is verifying the
correctness of the raw debug bucket lock within the debugobjects code.

The use of spin locks in the debugobjects code for synchronization is
pretty standard and looks to be correct to me.  This patch disables
the checking of the debugobjects internal locks by lockdep. In fact,
with this change, the hard lockup was not observed anymore.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 lib/debugobjects.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 70935ed91125..68d72ed9ca22 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -1106,8 +1106,15 @@ void __init debug_objects_early_init(void)
 {
 	int i;
 
-	for (i = 0; i < ODEBUG_HASH_SIZE; i++)
+	/*
+	 * We don't need lockdep to verify correctness of debugobjects
+	 * internal locks.
+	 */
+	lockdep_set_novalidate_class(&pool_lock);
+	for (i = 0; i < ODEBUG_HASH_SIZE; i++) {
 		raw_spin_lock_init(&obj_hash[i].lock);
+		lockdep_set_novalidate_class(&obj_hash[i].lock);
+	}
 
 	for (i = 0; i < ODEBUG_POOL_SIZE; i++)
 		hlist_add_head(&obj_static_pool[i].node, &obj_pool);
-- 
2.18.0

Re: [PATCH v2 2/2] debugobjects: Disable lockdep tracking of debugobjects internal locks

[Peter Zijlstra]

On Tue, Sep 25, 2018 at 10:41:09AM -0400, Waiman Long wrote:
> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index 70935ed91125..68d72ed9ca22 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -1106,8 +1106,15 @@ void __init debug_objects_early_init(void)
>  {
>  	int i;
>  
> -	for (i = 0; i < ODEBUG_HASH_SIZE; i++)
> +	/*
> +	 * We don't need lockdep to verify correctness of debugobjects
> +	 * internal locks.
> +	 */
> +	lockdep_set_novalidate_class(&pool_lock);
> +	for (i = 0; i < ODEBUG_HASH_SIZE; i++) {
>  		raw_spin_lock_init(&obj_hash[i].lock);
> +		lockdep_set_novalidate_class(&obj_hash[i].lock);
> +	}
>  
>  	for (i = 0; i < ODEBUG_POOL_SIZE; i++)
>  		hlist_add_head(&obj_static_pool[i].node, &obj_pool);

NAK, we do not _EVER_ set novalidate if it can at all be avoided.

If there is a severe performance problem with lockdep, try and cure
that. But really, who runs lockdep kernels on 8 sockets?

Re: [PATCH v2 2/2] debugobjects: Disable lockdep tracking of debugobjects internal locks

[Waiman Long]

On 09/25/2018 11:32 AM, Peter Zijlstra wrote:
> On Tue, Sep 25, 2018 at 10:41:09AM -0400, Waiman Long wrote:
>> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
>> index 70935ed91125..68d72ed9ca22 100644
>> --- a/lib/debugobjects.c
>> +++ b/lib/debugobjects.c
>> @@ -1106,8 +1106,15 @@ void __init debug_objects_early_init(void)
>>  {
>>  	int i;
>>  
>> -	for (i = 0; i < ODEBUG_HASH_SIZE; i++)
>> +	/*
>> +	 * We don't need lockdep to verify correctness of debugobjects
>> +	 * internal locks.
>> +	 */
>> +	lockdep_set_novalidate_class(&pool_lock);
>> +	for (i = 0; i < ODEBUG_HASH_SIZE; i++) {
>>  		raw_spin_lock_init(&obj_hash[i].lock);
>> +		lockdep_set_novalidate_class(&obj_hash[i].lock);
>> +	}
>>  
>>  	for (i = 0; i < ODEBUG_POOL_SIZE; i++)
>>  		hlist_add_head(&obj_static_pool[i].node, &obj_pool);
> NAK, we do not _EVER_ set novalidate if it can at all be avoided.
>
> If there is a severe performance problem with lockdep, try and cure
> that. But really, who runs lockdep kernels on 8 sockets?

We do. It is part of our testing process to run both production and
debug kernels on a variety of different machines to see if anything
breaks. Some of them just happen to be 8-socket systems.

The internal locks in the debugobjects code don't interact with other
locks at all as memory allocation isn't called with those lock held. So
disabling lockdep for those locks won't materially affect the accuracy
of the lockdep code.

How about the ability to declare a class of locks as terminal in the
sense that no further lock acquisition is allowed while holding a
terminal lock? That will allow the the lockdep code to fast track the
handling of those locks and hopefully prevent hard lockup problem like that.

Cheers,
Longman

Re: [PATCH v2 2/2] debugobjects: Disable lockdep tracking of debugobjects internal locks

[Peter Zijlstra]

On Tue, Sep 25, 2018 at 12:20:05PM -0400, Waiman Long wrote:
> On 09/25/2018 11:32 AM, Peter Zijlstra wrote:
> > On Tue, Sep 25, 2018 at 10:41:09AM -0400, Waiman Long wrote:
> >> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> >> index 70935ed91125..68d72ed9ca22 100644
> >> --- a/lib/debugobjects.c
> >> +++ b/lib/debugobjects.c
> >> @@ -1106,8 +1106,15 @@ void __init debug_objects_early_init(void)
> >>  {
> >>  	int i;
> >>  
> >> -	for (i = 0; i < ODEBUG_HASH_SIZE; i++)
> >> +	/*
> >> +	 * We don't need lockdep to verify correctness of debugobjects
> >> +	 * internal locks.
> >> +	 */
> >> +	lockdep_set_novalidate_class(&pool_lock);
> >> +	for (i = 0; i < ODEBUG_HASH_SIZE; i++) {
> >>  		raw_spin_lock_init(&obj_hash[i].lock);
> >> +		lockdep_set_novalidate_class(&obj_hash[i].lock);
> >> +	}
> >>  
> >>  	for (i = 0; i < ODEBUG_POOL_SIZE; i++)
> >>  		hlist_add_head(&obj_static_pool[i].node, &obj_pool);
> > NAK, we do not _EVER_ set novalidate if it can at all be avoided.
> >
> > If there is a severe performance problem with lockdep, try and cure
> > that. But really, who runs lockdep kernels on 8 sockets?
> 
> We do. It is part of our testing process to run both production and
> debug kernels on a variety of different machines to see if anything
> breaks. Some of them just happen to be 8-socket systems.
> 
> The internal locks in the debugobjects code don't interact with other
> locks at all as memory allocation isn't called with those lock held. So
> disabling lockdep for those locks won't materially affect the accuracy
> of the lockdep code.
> 
> How about the ability to declare a class of locks as terminal in the
> sense that no further lock acquisition is allowed while holding a
> terminal lock? That will allow the the lockdep code to fast track the
> handling of those locks and hopefully prevent hard lockup problem like that.

Who guarantees they stay leaf locks? What if someone mucks up the
debugobject locking because its Monday morning and they haven't had
their coffee yet. You want lockdep to catch all that.

Also, where is the performance benefit here. The normal lock_acquire
path doesn't change gobal state (it will not see new lock ordering in
99%+ of the cases).

Typically we only push the lock on the task local lock stack, compute
the new hash, and do the hash lookup, find it already exists and return.

So what is the expensive part, and can we do something about that?

Re: [PATCH v2 2/2] debugobjects: Disable lockdep tracking of debugobjects internal locks

[Waiman Long]

On 09/25/2018 12:31 PM, Peter Zijlstra wrote:
> On Tue, Sep 25, 2018 at 12:20:05PM -0400, Waiman Long wrote:
>> On 09/25/2018 11:32 AM, Peter Zijlstra wrote:
>>> On Tue, Sep 25, 2018 at 10:41:09AM -0400, Waiman Long wrote:
>>>> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
>>>> index 70935ed91125..68d72ed9ca22 100644
>>>> --- a/lib/debugobjects.c
>>>> +++ b/lib/debugobjects.c
>>>> @@ -1106,8 +1106,15 @@ void __init debug_objects_early_init(void)
>>>>  {
>>>>  	int i;
>>>>  
>>>> -	for (i = 0; i < ODEBUG_HASH_SIZE; i++)
>>>> +	/*
>>>> +	 * We don't need lockdep to verify correctness of debugobjects
>>>> +	 * internal locks.
>>>> +	 */
>>>> +	lockdep_set_novalidate_class(&pool_lock);
>>>> +	for (i = 0; i < ODEBUG_HASH_SIZE; i++) {
>>>>  		raw_spin_lock_init(&obj_hash[i].lock);
>>>> +		lockdep_set_novalidate_class(&obj_hash[i].lock);
>>>> +	}
>>>>  
>>>>  	for (i = 0; i < ODEBUG_POOL_SIZE; i++)
>>>>  		hlist_add_head(&obj_static_pool[i].node, &obj_pool);
>>> NAK, we do not _EVER_ set novalidate if it can at all be avoided.
>>>
>>> If there is a severe performance problem with lockdep, try and cure
>>> that. But really, who runs lockdep kernels on 8 sockets?
>> We do. It is part of our testing process to run both production and
>> debug kernels on a variety of different machines to see if anything
>> breaks. Some of them just happen to be 8-socket systems.
>>
>> The internal locks in the debugobjects code don't interact with other
>> locks at all as memory allocation isn't called with those lock held. So
>> disabling lockdep for those locks won't materially affect the accuracy
>> of the lockdep code.
>>
>> How about the ability to declare a class of locks as terminal in the
>> sense that no further lock acquisition is allowed while holding a
>> terminal lock? That will allow the the lockdep code to fast track the
>> handling of those locks and hopefully prevent hard lockup problem like that.
> Who guarantees they stay leaf locks? What if someone mucks up the
> debugobject locking because its Monday morning and they haven't had
> their coffee yet. You want lockdep to catch all that.

Lockdep will need to do checking to guarantee that.

> Also, where is the performance benefit here. The normal lock_acquire
> path doesn't change gobal state (it will not see new lock ordering in
> 99%+ of the cases).
>
> Typically we only push the lock on the task local lock stack, compute
> the new hash, and do the hash lookup, find it already exists and return.
>
> So what is the expensive part, and can we do something about that?

I will need to do some experimentation to find out. Will update this
thread with my finding.

Cheers,
Longman

Re: [PATCH v2 2/2] debugobjects: Disable lockdep tracking of debugobjects internal locks

[Waiman Long]

On 09/25/2018 12:36 PM, Waiman Long wrote:
> On 09/25/2018 12:31 PM, Peter Zijlstra wrote:
>> Also, where is the performance benefit here. The normal lock_acquire
>> path doesn't change gobal state (it will not see new lock ordering in
>> 99%+ of the cases).
>>
>> Typically we only push the lock on the task local lock stack, compute
>> the new hash, and do the hash lookup, find it already exists and return.
>>
>> So what is the expensive part, and can we do something about that?
> I will need to do some experimentation to find out. Will update this
> thread with my finding.
>

I have sent out a patchset on improving lockdep performance that
hopefully will help to alleviate the problems that I saw. I still have
other ideas about further changes I want to investigate, but that will
wait until I am done with the current lockdep patchset.

-Longman