Re: [PATCH v4] mm/numa_balancing: Teach mpol_to_str about the balancing mode

[Tvrtko Ursulin]

On 09/07/2024 07:19, Huang, Ying wrote:
> Tvrtko Ursulin <tursulin@xxxxxxxxxx> writes:
>
> > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > 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@xxxxxxxxx>
> > Cc: Mel Gorman <mgorman@xxxxxxx>
> > Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: Rik van Riel <riel@xxxxxxxxxxx>
> > Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
> > Cc: "Matthew Wilcox (Oracle)" <willy@xxxxxxxxxxxxx>
> > Cc: Dave Hansen <dave.hansen@xxxxxxxxx>
> > Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > Cc: Michal Hocko <mhocko@xxxxxxxx>
> > Cc: David Rientjes <rientjes@xxxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx> # v5.12+
>
> LGTM, Thanks!
>
> Reviewed-by: "Huang, Ying" <ying.huang@xxxxxxxxx>

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))