Message-ID: <916e4781-54b5-417a-be22-499739e4d818@igalia.com>
Date: Fri, 12 Jul 2024 10:19:43 +0100
From: Tvrtko Ursulin <tvrtko.ursulin@...lia.com>
To: "Huang, Ying" <ying.huang@...el.com>, Tvrtko Ursulin
 <tursulin@...lia.com>, Andrew Morton <akpm@...ux-foundation.org>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org, kernel-dev@...lia.com,
 Mel Gorman <mgorman@...e.de>, Peter Zijlstra <peterz@...radead.org>,
 Ingo Molnar <mingo@...hat.com>, Rik van Riel <riel@...riel.com>,
 Johannes Weiner <hannes@...xchg.org>,
 "Matthew Wilcox (Oracle)" <willy@...radead.org>,
 Dave Hansen <dave.hansen@...el.com>, Andi Kleen <ak@...ux.intel.com>,
 Michal Hocko <mhocko@...e.com>, David Rientjes <rientjes@...gle.com>,
 stable@...r.kernel.org
Subject: Re: [PATCH v4] mm/numa_balancing: Teach mpol_to_str about the
 balancing mode


On 09/07/2024 07:19, Huang, Ying wrote:
> Tvrtko Ursulin <tursulin@...lia.com> writes:
> 
>> From: Tvrtko Ursulin <tvrtko.ursulin@...lia.com>
>>
>> Since balancing mode was added in
>> bda420b98505 ("numa balancing: migrate on fault among multiple bound nodes"),
>> it was possible to set this mode but it wouldn't be shown in
>> /proc/<pid>/numa_maps since there was no support for it in the
>> mpol_to_str() helper.
>>
>> Furthermore, because the balancing mode sets the MPOL_F_MORON flag, it
>> would be displayed as 'default' due a workaround introduced a few years
>> earlier in
>> 8790c71a18e5 ("mm/mempolicy.c: fix mempolicy printing in numa_maps").
>>
>> To tidy this up we implement two changes:
>>
>> Replace the MPOL_F_MORON check by pointer comparison against the
>> preferred_node_policy array. By doing this we generalise the current
>> special casing and replace the incorrect 'default' with the correct
>> 'bind' for the mode.
>>
>> Secondly, we add a string representation and corresponding handling for
>> the MPOL_F_NUMA_BALANCING flag.
>>
>> With the two changes together we start showing the balancing flag when it
>> is set and therefore complete the fix.
>>
>> Representation format chosen is to separate multiple flags with vertical
>> bars, following what existed long time ago in kernel 2.6.25. But as
>> between then and now there wasn't a way to display multiple flags, this
>> patch does not change the format in practice.
>>
>> Some /proc/<pid>/numa_maps output examples:
>>
>>   555559580000 bind=balancing:0-1,3 file=...
>>   555585800000 bind=balancing|static:0,2 file=...
>>   555635240000 prefer=relative:0 file=
>>
>> v2:
>>   * Fully fix by introducing MPOL_F_KERNEL.
>>
>> v3:
>>   * Abandoned the MPOL_F_KERNEL approach in favour of pointer comparisons.
>>   * Removed lookup generalisation for easier backporting.
>>   * Replaced commas as separator with vertical bars.
>>   * Added a few more words about the string format in the commit message.
>>
>> v4:
>>   * Use is_power_of_2.
>>   * Use ARRAY_SIZE and update recommended buffer size for two flags.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@...lia.com>
>> Fixes: bda420b98505 ("numa balancing: migrate on fault among multiple bound nodes")
>> References: 8790c71a18e5 ("mm/mempolicy.c: fix mempolicy printing in numa_maps")
>> Cc: Huang Ying <ying.huang@...el.com>
>> Cc: Mel Gorman <mgorman@...e.de>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: Rik van Riel <riel@...riel.com>
>> Cc: Johannes Weiner <hannes@...xchg.org>
>> Cc: "Matthew Wilcox (Oracle)" <willy@...radead.org>
>> Cc: Dave Hansen <dave.hansen@...el.com>
>> Cc: Andi Kleen <ak@...ux.intel.com>
>> Cc: Michal Hocko <mhocko@...e.com>
>> Cc: David Rientjes <rientjes@...gle.com>
>> Cc: <stable@...r.kernel.org> # v5.12+
> 
> LGTM, Thanks!
> 
> Reviewed-by: "Huang, Ying" <ying.huang@...el.com>

Thank you!

Andrew, this appears safe to pick up now as a replacement for the 
identically named patch you previously picked up but then had to back 
out from the queue.

Thanks!

Tvrtko

>> ---
>>   mm/mempolicy.c | 18 ++++++++++++++----
>>   1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> index aec756ae5637..a1bf9aa15c33 100644
>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -3293,8 +3293,9 @@ int mpol_parse_str(char *str, struct mempolicy **mpol)
>>    * @pol:  pointer to mempolicy to be formatted
>>    *
>>    * Convert @pol into a string.  If @buffer is too short, truncate the string.
>> - * Recommend a @maxlen of at least 32 for the longest mode, "interleave", the
>> - * longest flag, "relative", and to display at least a few node ids.
>> + * Recommend a @maxlen of at least 51 for the longest mode, "weighted
>> + * interleave", plus the longest flag flags, "relative|balancing", and to
>> + * display at least a few node ids.
>>    */
>>   void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
>>   {
>> @@ -3303,7 +3304,10 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
>>   	unsigned short mode = MPOL_DEFAULT;
>>   	unsigned short flags = 0;
>>   
>> -	if (pol && pol != &default_policy && !(pol->flags & MPOL_F_MORON)) {
>> +	if (pol &&
>> +	    pol != &default_policy &&
>> +	    !(pol >= &preferred_node_policy[0] &&
>> +	      pol <= &preferred_node_policy[ARRAY_SIZE(preferred_node_policy) - 1])) {
>>   		mode = pol->mode;
>>   		flags = pol->flags;
>>   	}
>> @@ -3331,12 +3335,18 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
>>   		p += snprintf(p, buffer + maxlen - p, "=");
>>   
>>   		/*
>> -		 * Currently, the only defined flags are mutually exclusive
>> +		 * Static and relative are mutually exclusive.
>>   		 */
>>   		if (flags & MPOL_F_STATIC_NODES)
>>   			p += snprintf(p, buffer + maxlen - p, "static");
>>   		else if (flags & MPOL_F_RELATIVE_NODES)
>>   			p += snprintf(p, buffer + maxlen - p, "relative");
>> +
>> +		if (flags & MPOL_F_NUMA_BALANCING) {
>> +			if (!is_power_of_2(flags & MPOL_MODE_FLAGS))
>> +				p += snprintf(p, buffer + maxlen - p, "|");
>> +			p += snprintf(p, buffer + maxlen - p, "balancing");
>> +		}
>>   	}
>>   
>>   	if (!nodes_empty(nodes))

Powered by blists - more mailing lists