[PATCH] mm, page_alloc: don't check cpuset allowed twice in fast-path

[PATCH] mm, page_alloc: don't check cpuset allowed twice in fast-path

[Vlastimil Babka]

Since commit 682a3385e773 ("mm, page_alloc: inline the fast path of the
zonelist iterator") we replace a NULL nodemask with cpuset_current_mems_allowed
in the fast path, so that get_page_from_freelist() filters nodes allowed by the
cpuset via for_next_zone_zonelist_nodemask(). In that case it's pointless to
also check __cpuset_zone_allowed(), which we can avoid by not using
ALLOC_CPUSET in that scenario.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 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 2c6d5f64feca..3d86fbe2f4f4 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3754,9 +3754,10 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
 
 	if (cpusets_enabled()) {
 		alloc_mask |= __GFP_HARDWALL;
-		alloc_flags |= ALLOC_CPUSET;
 		if (!ac.nodemask)
 			ac.nodemask = &cpuset_current_mems_allowed;
+		else
+			alloc_flags |= ALLOC_CPUSET;
 	}
 
 	gfp_mask &= gfp_allowed_mask;
-- 
2.11.0

Re: [PATCH] mm, page_alloc: don't check cpuset allowed twice in fast-path

[Michal Hocko]

On Fri 06-01-17 09:18:05, Vlastimil Babka wrote:
> Since commit 682a3385e773 ("mm, page_alloc: inline the fast path of the
> zonelist iterator") we replace a NULL nodemask with cpuset_current_mems_allowed
> in the fast path, so that get_page_from_freelist() filters nodes allowed by the
> cpuset via for_next_zone_zonelist_nodemask(). In that case it's pointless to
> also check __cpuset_zone_allowed(), which we can avoid by not using
> ALLOC_CPUSET in that scenario.

OK, this seems to be really worth it as most allocations go via
__alloc_pages so we can save __cpuset_zone_allowed in the fast path.

I was about to object how fragile this might be wrt. other ALLOC_CPUSET
checks but then I've realized this is only for the hotpath as the
slowpath goes through gfp_to_alloc_flags() which sets it back on.

Maybe all that could be added to the changelog?
 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  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 2c6d5f64feca..3d86fbe2f4f4 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3754,9 +3754,10 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
>  
>  	if (cpusets_enabled()) {
>  		alloc_mask |= __GFP_HARDWALL;
> -		alloc_flags |= ALLOC_CPUSET;
>  		if (!ac.nodemask)
>  			ac.nodemask = &cpuset_current_mems_allowed;
> +		else
> +			alloc_flags |= ALLOC_CPUSET;
>  	}
>  
>  	gfp_mask &= gfp_allowed_mask;
> -- 
> 2.11.0
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm, page_alloc: don't check cpuset allowed twice in fast-path

[Vlastimil Babka]

On 01/06/2017 11:40 AM, Michal Hocko wrote:
> On Fri 06-01-17 09:18:05, Vlastimil Babka wrote:
>> Since commit 682a3385e773 ("mm, page_alloc: inline the fast path of the
>> zonelist iterator") we replace a NULL nodemask with cpuset_current_mems_allowed
>> in the fast path, so that get_page_from_freelist() filters nodes allowed by the
>> cpuset via for_next_zone_zonelist_nodemask(). In that case it's pointless to
>> also check __cpuset_zone_allowed(), which we can avoid by not using
>> ALLOC_CPUSET in that scenario.
> 
> OK, this seems to be really worth it as most allocations go via
> __alloc_pages so we can save __cpuset_zone_allowed in the fast path.

Well the "really fast path" assumes that there are no cpusets (except
the root one), which is done using static key check in
cpusets_enabled(). But we can still do better even if they are enabled.

> I was about to object how fragile this might be wrt. other ALLOC_CPUSET
> checks but then I've realized this is only for the hotpath as the
> slowpath goes through gfp_to_alloc_flags() which sets it back on.
> 
> Maybe all that could be added to the changelog?

OK, will do after collecting more feedback.

>  
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> 
>> ---
>>  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 2c6d5f64feca..3d86fbe2f4f4 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -3754,9 +3754,10 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
>>  
>>  	if (cpusets_enabled()) {
>>  		alloc_mask |= __GFP_HARDWALL;
>> -		alloc_flags |= ALLOC_CPUSET;
>>  		if (!ac.nodemask)
>>  			ac.nodemask = &cpuset_current_mems_allowed;
>> +		else
>> +			alloc_flags |= ALLOC_CPUSET;
>>  	}
>>  
>>  	gfp_mask &= gfp_allowed_mask;
>> -- 
>> 2.11.0
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

Re: [PATCH] mm, page_alloc: don't check cpuset allowed twice in fast-path

[Anshuman Khandual]

On 01/06/2017 04:10 PM, Michal Hocko wrote:
> On Fri 06-01-17 09:18:05, Vlastimil Babka wrote:
>> Since commit 682a3385e773 ("mm, page_alloc: inline the fast path of the
>> zonelist iterator") we replace a NULL nodemask with cpuset_current_mems_allowed
>> in the fast path, so that get_page_from_freelist() filters nodes allowed by the
>> cpuset via for_next_zone_zonelist_nodemask(). In that case it's pointless to
>> also check __cpuset_zone_allowed(), which we can avoid by not using
>> ALLOC_CPUSET in that scenario.
> 
> OK, this seems to be really worth it as most allocations go via
> __alloc_pages so we can save __cpuset_zone_allowed in the fast path.
> 
> I was about to object how fragile this might be wrt. other ALLOC_CPUSET
> checks but then I've realized this is only for the hotpath as the
> slowpath goes through gfp_to_alloc_flags() which sets it back on.
> 
> Maybe all that could be added to the changelog?

Agreed, all these should be added into the change log as the effect
of cpuset based nodemask during fast path and slow path is little
bit confusing.

>  
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Acked-by: Michal Hocko <mhocko@suse.com>

Reviewed-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>