Re: [PATCH] mm, page_alloc: actually ignore mempolicies for high priority allocations

[Vlastimil Babka <vbabka@suse.cz>]

On 06/13/2018 09:42 PM, David Rientjes wrote:
> On Tue, 12 Jun 2018, Vlastimil Babka wrote:
> 
>> The __alloc_pages_slowpath() function has for a long time contained code to
>> ignore node restrictions from memory policies for high priority allocations.
>> The current code that resets the zonelist iterator however does effectively
>> nothing after commit 7810e6781e0f ("mm, page_alloc: do not break __GFP_THISNODE
>> by zonelist reset") removed a buggy zonelist reset. Even before that commit,
>> mempolicy restrictions were still not ignored, as they are passed in
>> ac->nodemask which is untouched by the code.
>>
>> We can either remove the code, or make it work as intended. Since
>> ac->nodemask can be set from task's mempolicy via alloc_pages_current() and
>> thus also alloc_pages(), it may indeed affect kernel allocations, and it makes
>> sense to ignore it to allow progress for high priority allocations.
>>
>> Thus, this patch resets ac->nodemask to NULL in such cases. This assumes all
>> callers can handle it (i.e. there are no guarantees as in the case of
>> __GFP_THISNODE) which seems to be the case. The same assumption is already
>> present in check_retry_cpuset() for some time.
>>
>> The expected effect is that high priority kernel allocations in the context of
>> userspace tasks (e.g. OOM victims) restricted by mempolicies will have higher
>> chance to succeed if they are restricted to nodes with depleted memory, while
>> there are other nodes with free memory left.
>>
> 
> Hi Vlastimil,
> 
> Is this expected as a change back to previous behavior that we have lost 
> or is this new development for high priority allocations?  I don't think 
> we have ignored mempolicies for things like GFP_ATOMIC allocations in the 
> past.

Well, it's not a new intention, but for the first time the code will
match the intention, AFAICS. It was intended by commit 183f6371aac2
("mm: ignore mempolicies when using ALLOC_NO_WATERMARK") in v3.6 but I
think it never really worked, as mempolicy restriction was already
encoded in nodemask, not zonelist, at that time.

So originally that was for ALLOC_NO_WATERMARK only. Then it was adjusted
by e46e7b77c909 ("mm, page_alloc: recalculate the preferred zoneref if
the context can ignore memory policies") and cd04ae1e2dc8 ("mm, oom: do
not rely on TIF_MEMDIE for memory reserves access") to the current
state. So yeah even GFP_ATOMIC would now ignore mempolicies after the
initial attempts fail... if the code worked as people thought it does.

>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>> Cc: Mel Gorman <mgorman@techsingularity.net>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Cc: David Rientjes <rientjes@google.com>
>> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>> ---
>>  mm/page_alloc.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 07b3c23762ad..ec8c92ff8b3c 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -4164,11 +4164,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>>  		alloc_flags = reserve_flags;
>>  
>>  	/*
>> -	 * Reset the zonelist iterators if memory policies can be ignored.
>> -	 * These allocations are high priority and system rather than user
>> -	 * orientated.
>> +	 * Reset the nodemask and zonelist iterators if memory policies can be
>> +	 * ignored. These allocations are high priority and system rather than
>> +	 * user oriented.
>>  	 */
>>  	if (!(alloc_flags & ALLOC_CPUSET) || reserve_flags) {
>> +		ac->nodemask = NULL;
>>  		ac->preferred_zoneref = first_zones_zonelist(ac->zonelist,
>>  					ac->high_zoneidx, ac->nodemask);
>>  	}