Re: [PATCH v2] mm/page_alloc: don't wake kswapd from rmqueue() unless __GFP_KSWAPD_RECLAIM is specified

["Huang, Ying" <ying.huang@intel.com>]

Hi, Tetsuo,

"Huang, Ying" <ying.huang@intel.com> writes:

> Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:
>
>> Commit 73444bc4d8f9 ("mm, page_alloc: do not wake kswapd with zone lock
>> held") moved wakeup_kswapd() from steal_suitable_fallback() to rmqueue()
>> using ZONE_BOOSTED_WATERMARK flag.
>>
>> Only allocation contexts that include ALLOC_KSWAPD (which corresponds to
>> __GFP_KSWAPD_RECLAIM) should wake kswapd, for callers are supposed to
>> remove __GFP_KSWAPD_RECLAIM if trying to hold pgdat->kswapd_wait has a
>> risk of deadlock. But since zone->flags is a shared variable, a thread
>> doing !__GFP_KSWAPD_RECLAIM allocation request might observe this flag
>> being set immediately after another thread doing __GFP_KSWAPD_RECLAIM
>> allocation request set this flag, causing possibility of deadlock.
>
> Sorry, I don't understand what is the deadlock here.
>
> I checked commit 73444bc4d8f9 ("mm, page_alloc: do not wake kswapd with
> zone lock held") and the corresponding mail thread.  From the below
> mail,
>
> https://lore.kernel.org/all/20190107204627.GA25526@cmpxchg.org/
>
> commit 73444bc4d8f9 fixed a circular locking ordering as follows,
>
> pi lock -> rq lock -> timer base lock -> zone lock -> wakeup lock
> (kswapd_wait, fixed) -> pi lock
>
> But I don't know what is the deadlock that your patch fixed.  Can you
> teach me on that?

Just read your email in another thread related to this patch as follow,

https://lore.kernel.org/linux-mm/d642e597-cf7d-b410-16ce-22dff483fd8e@I-love.SAKURA.ne.jp/

Is that the deadlock that you tried to fix in this patch?

It appears that commit 73444bc4d8f9 didn't fix the deadlock above.  It
just convert the circular locking ordering to,

pi lock -> rq lock -> timer base lock -> wakeup lock (kswapd_wait,
fixed) -> pi lock

If so, I think that it's better to add corresponding information in
patch description to avoid the possible confusion.

Best Regards,
Huang, Ying

>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
>> Fixes: 73444bc4d8f9 ("mm, page_alloc: do not wake kswapd with zone lock held")
>> ---
>> Changes in v2:
>>   Check ALLOC_KSWAPD before checking ZONE_BOOSTED_WATERMARK and update
>>   description, suggested by Mel Gorman <mgorman@techsingularity.net>.
>>
>>  mm/page_alloc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 47421bedc12b..ecad680cec53 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -3052,7 +3052,8 @@ struct page *rmqueue(struct zone *preferred_zone,
>>  
>>  out:
>>  	/* Separate test+clear to avoid unnecessary atomics */
>> -	if (unlikely(test_bit(ZONE_BOOSTED_WATERMARK, &zone->flags))) {
>> +	if ((alloc_flags & ALLOC_KSWAPD) &&
>> +	    unlikely(test_bit(ZONE_BOOSTED_WATERMARK, &zone->flags))) {
>>  		clear_bit(ZONE_BOOSTED_WATERMARK, &zone->flags);
>>  		wakeup_kswapd(zone, 0, 0, zone_idx(zone));
>>  	}