Re: [PATCH v2 1/2] sched: Add PF_MEMALLOC_NOLOCKDEP flag

[Waiman Long <longman@redhat.com>]

On 6/17/20 8:01 PM, Dave Chinner wrote:
> On Wed, Jun 17, 2020 at 01:53:09PM -0400, Waiman Long wrote:
>> There are cases where calling kmalloc() can lead to false positive
>> lockdep splat. One notable example that can happen in the freezing of
>> the xfs filesystem is as follows:
>>
>>   Possible unsafe locking scenario:
>>
>>         CPU0                    CPU1
>>         ----                    ----
>>    lock(sb_internal);
>>                                 lock(fs_reclaim);
>>                                 lock(sb_internal);
>>    lock(fs_reclaim);
>>
>>   *** DEADLOCK ***
>>
>> This is a false positive as all the dirty pages are flushed out before
>> the filesystem can be frozen. However, there is no easy way to modify
>> lockdep to handle this situation properly.
>>
>> One possible workaround is to disable lockdep by setting __GFP_NOLOCKDEP
>> in the appropriate kmalloc() calls.  However, it will be cumbersome to
>> locate all the right kmalloc() calls to insert __GFP_NOLOCKDEP and it
>> is easy to miss some especially when the code is updated in the future.
>>
>> Another alternative is to have a per-process global state that indicates
>> the equivalent of __GFP_NOLOCKDEP without the need to set the gfp_t flag
>> individually. To allow the latter case, a new PF_MEMALLOC_NOLOCKDEP
>> per-process flag is now added. After adding this new bit, there are
>> still 2 free bits left.
>>
>> Suggested-by: Dave Chinner <david@fromorbit.com>
>> Signed-off-by: Waiman Long <longman@redhat.com>
>> ---
>>   include/linux/sched.h    |  7 +++++++
>>   include/linux/sched/mm.h | 15 ++++++++++-----
>>   2 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index b62e6aaf28f0..44247cbc9073 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -1508,6 +1508,7 @@ extern struct pid *cad_pid;
>>   #define PF_MEMALLOC_NOIO	0x00080000	/* All allocation requests will inherit GFP_NOIO */
>>   #define PF_LOCAL_THROTTLE	0x00100000	/* Throttle writes only against the bdi I write to,
>>   						 * I am cleaning dirty pages from some other bdi. */
>> +#define __PF_MEMALLOC_NOLOCKDEP	0x00100000	/* All allocation requests will inherit __GFP_NOLOCKDEP */
> Why is this considered a safe thing to do? Any context that sets
> __PF_MEMALLOC_NOLOCKDEP will now behave differently in memory
> reclaim as it will think that PF_LOCAL_THROTTLE is set when lockdep
> is enabled.

Oh, my mistake, it should be 0x01000000 which is not currently being 
used. Thank for catching that. I will repost a new version. I have no 
intention to reuse any existing bit. As said in the commit log, there 
are actually 2 more free bits left.


>
>>   #define PF_KTHREAD		0x00200000	/* I am a kernel thread */
>>   #define PF_RANDOMIZE		0x00400000	/* Randomize virtual address space */
>>   #define PF_SWAPWRITE		0x00800000	/* Allowed to write to swap */
>> @@ -1519,6 +1520,12 @@ extern struct pid *cad_pid;
>>   #define PF_FREEZER_SKIP		0x40000000	/* Freezer should not count it as freezable */
>>   #define PF_SUSPEND_TASK		0x80000000      /* This thread called freeze_processes() and should not be frozen */
>>   
>> +#ifdef CONFIG_LOCKDEP
>> +#define PF_MEMALLOC_NOLOCKDEP	__PF_MEMALLOC_NOLOCKDEP
>> +#else
>> +#define PF_MEMALLOC_NOLOCKDEP	0
>> +#endif
>> +
>>   /*
>>    * Only the _current_ task can read/write to tsk->flags, but other
>>    * tasks can access tsk->flags in readonly mode for example
>> diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
>> index 480a4d1b7dd8..4a076a148568 100644
>> --- a/include/linux/sched/mm.h
>> +++ b/include/linux/sched/mm.h
>> @@ -177,22 +177,27 @@ static inline bool in_vfork(struct task_struct *tsk)
>>    * Applies per-task gfp context to the given allocation flags.
>>    * PF_MEMALLOC_NOIO implies GFP_NOIO
>>    * PF_MEMALLOC_NOFS implies GFP_NOFS
>> + * PF_MEMALLOC_NOLOCKDEP implies __GFP_NOLOCKDEP
>>    * PF_MEMALLOC_NOCMA implies no allocation from CMA region.
>>    */
>>   static inline gfp_t current_gfp_context(gfp_t flags)
>>   {
>> -	if (unlikely(current->flags &
>> -		     (PF_MEMALLOC_NOIO | PF_MEMALLOC_NOFS | PF_MEMALLOC_NOCMA))) {
>> +	unsigned int pflags = current->flags;
>> +
>> +	if (unlikely(pflags & (PF_MEMALLOC_NOIO | PF_MEMALLOC_NOFS |
>> +			       PF_MEMALLOC_NOCMA | PF_MEMALLOC_NOLOCKDEP))) {
> That needs a PF_MEMALLOC_MASK.

Will add that in the next version.

Thanks,
Longman