Re: [PATCH v2] lockdep: Teach lockdep about memalloc_noio_save

[Nikolay Borisov]

On  1.03.2017 11:46, Peter Zijlstra wrote:
> On Wed, Mar 01, 2017 at 09:59:00AM +0200, Nikolay Borisov wrote:
>> Commit 21caf2fc1931 ("mm: teach mm by current context info to not do I/O
>> during memory allocation") added the memalloc_noio_(save|restore) functions
>> to enable people to modify the MM behavior by disbaling I/O during memory
>> allocation. This prevents allocation paths recursing back into the filesystem
>> without explicitly changing the flags for every allocation site. Yet, lockdep
>> not being aware of that is prone to showing false positives. Fix this
>> by teaching it that the presence of PF_MEMALLOC_NOIO flag mean we are not
>> going to issue any I/O
> 
> I'm not up to date on the specific, but GFP_IO is separate from GFP_FS.
> 
> And MEMALLOC_NOIO only clears GFP_IO but leaves GFP_FS set.

static inline gfp_t memalloc_noio_flags(gfp_t flags)                            
{                                                                               
    if (unlikely(current->flags & PF_MEMALLOC_NOIO))                            
        flags &= ~(__GFP_IO | __GFP_FS);                                        
    return flags;                                                               
}


> 
> Therefore I think your change is wrong, but I might have overlooked a
> detail. Added original authors to Cc to clarify.
> 
> 
>> Signed-off-by: Nikolay Borisov <nborisov@xxxxxxxx>
>> ---
>>  kernel/locking/lockdep.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>>
>>  Turned out there was another place where RELCAIM_FS was being set, fix it 
>>  by using the memalloc_noio_flags when setting ->lockdep_reclaim_gfp flags.
>>
>> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
>> index 9812e5dd409e..87cf9910e66f 100644
>> --- a/kernel/locking/lockdep.c
>> +++ b/kernel/locking/lockdep.c
>> @@ -2866,7 +2866,8 @@ static void __lockdep_trace_alloc(gfp_t gfp_mask, unsigned long flags)
>>  		return;
>>  
>>  	/* this guy won't enter reclaim */
>> -	if ((curr->flags & PF_MEMALLOC) && !(gfp_mask & __GFP_NOMEMALLOC))
>> +	if (((curr->flags & PF_MEMALLOC) && !(gfp_mask & __GFP_NOMEMALLOC)) ||
>> +			curr->flags & PF_MEMALLOC_NOIO)
>>  		return;
>>  
>>  	/* We're only interested __GFP_FS allocations for now */
>> @@ -3852,7 +3853,7 @@ EXPORT_SYMBOL_GPL(lock_unpin_lock);
>>  
>>  void lockdep_set_current_reclaim_state(gfp_t gfp_mask)
>>  {
>> -	current->lockdep_reclaim_gfp = gfp_mask;
>> +	current->lockdep_reclaim_gfp = memalloc_noio_flags(gfp_mask);
>>  }
>>  
>>  void lockdep_clear_current_reclaim_state(void)
>> -- 
>> 2.7.4
>>
>