[PATCH 0/3] One fix for /proc/<pid>/numa_maps and two RFCs

[PATCH 0/3] One fix for /proc/<pid>/numa_maps and two RFCs

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Hi all,

First patch fixes the issue where NUMA balancing mode was wrongly shown
'default' and is small and so hopefully backportable to stable.c

Second two patches are more like RFC. Although they work I am not 100% sure of
their benefit. Especially the last patch which enables turning on NUMA balancing
with tmpfs. Second one at least makes the flag handling a bit more robust in the
sense that it can prevent future mistakes.

Comments and opinions are welcome!

Cc: Huang Ying <ying.huang@intel.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>

Tvrtko Ursulin (3):
  mm/numa_balancing: Teach mpol_to_str about the balancing mode
  mm/mempolicy: Use flags lookup array in mpol_to_str
  mm/numa_balancing: Allow setting numa balancing for tmpfs

 mm/mempolicy.c | 123 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 92 insertions(+), 31 deletions(-)

-- 
2.44.0

[PATCH 1/3] mm/numa_balancing: Teach mpol_to_str about the balancing mode

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@igalia.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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.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@intel.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Rik van Riel <riel@surriel.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: David Rientjes <rientjes@google.com>
Cc: <stable@vger.kernel.org> # v5.12+
---
 mm/mempolicy.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index aec756ae5637..1bfb6c73a39c 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 42 for the longest mode, "weighted
+ * interleave", the longest flag, "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[MAX_NUMNODES - 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 (hweight16(flags & MPOL_MODE_FLAGS) > 1)
+				p += snprintf(p, buffer + maxlen - p, "|");
+			p += snprintf(p, buffer + maxlen - p, "balancing");
+		}
 	}
 
 	if (!nodes_empty(nodes))
-- 
2.44.0

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

[Matthew Wilcox]

On Fri, Jul 05, 2024 at 03:32:16PM +0100, Tvrtko Ursulin wrote:
> +		if (flags & MPOL_F_NUMA_BALANCING) {
> +			if (hweight16(flags & MPOL_MODE_FLAGS) > 1)

hweight() > 1 seems somewhat inefficient.
!is_power_of_2() would be better.  Or clear off the bits as they're
printed and print the bar if the remaining flags are not 0.

> +				p += snprintf(p, buffer + maxlen - p, "|");
> +			p += snprintf(p, buffer + maxlen - p, "balancing");
> +		}
>  	}
>  
>  	if (!nodes_empty(nodes))
> -- 
> 2.44.0
> 
> 

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

[Huang, Ying]

Tvrtko Ursulin <tursulin@igalia.com> writes:

> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.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.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.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@intel.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Rik van Riel <riel@surriel.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: <stable@vger.kernel.org> # v5.12+
> ---
>  mm/mempolicy.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index aec756ae5637..1bfb6c73a39c 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 42 for the longest mode, "weighted
> + * interleave", the longest flag, "balancing", and to display at least a few

And we may display 2 flags now, +9 further?

> + * 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[MAX_NUMNODES - 1])) {

Better to replace MAX_NUMNODES with ARRAY_SIZE() here.

>  		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 (hweight16(flags & MPOL_MODE_FLAGS) > 1)
> +				p += snprintf(p, buffer + maxlen - p, "|");
> +			p += snprintf(p, buffer + maxlen - p, "balancing");
> +		}

Still think that it's better to move this part to [2/3].  Unless you can
make the change small and the resulting code looks good.

>  	}
>  
>  	if (!nodes_empty(nodes))

--
Best Regards,
Huang, Ying

[PATCH 2/3] mm/mempolicy: Use flags lookup array in mpol_to_str

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Generalise displaying of flags in mpol_to_str() with the benefit of
removing the knowledge of which flags are mutually exclusive and which are
not.

Also add a warning for unknown flags which slightly "hardens" against
possible future omissions to add handling for new stuff, as had happened
in bda420b98505.

Simplify by switching to scnprintf while at it.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
References: bda420b98505 ("numa balancing: migrate on fault among multiple bound nodes")
Cc: Huang Ying <ying.huang@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 mm/mempolicy.c | 58 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 1bfb6c73a39c..77488878d8ca 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -3150,6 +3150,17 @@ static const char * const policy_modes[] =
 	[MPOL_PREFERRED_MANY]  = "prefer (many)",
 };
 
+/*
+ * Lookup array containing only uapi flags where the lowest user flag starts at
+ * array index zero.
+ */
+#define MPOL_FLAG_STR_INDEX(f) (ilog2(f) - __builtin_ffs(MPOL_MODE_FLAGS) + 1)
+static const char * const policy_flags[] = {
+	[MPOL_FLAG_STR_INDEX(MPOL_F_STATIC_NODES)] = "static",
+	[MPOL_FLAG_STR_INDEX(MPOL_F_RELATIVE_NODES)] = "relative",
+	[MPOL_FLAG_STR_INDEX(MPOL_F_NUMA_BALANCING)] = "balancing",
+};
+
 #ifdef CONFIG_TMPFS
 /**
  * mpol_parse_str - parse string to mempolicy, for tmpfs mpol mount option.
@@ -3302,14 +3313,21 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
 	char *p = buffer;
 	nodemask_t nodes = NODE_MASK_NONE;
 	unsigned short mode = MPOL_DEFAULT;
-	unsigned short flags = 0;
+	unsigned int bit, cnt;
+	unsigned long flags = 0;
+	int res;
 
 	if (pol &&
 	    pol != &default_policy &&
 	    !(pol >= &preferred_node_policy[0] &&
 	      pol <= &preferred_node_policy[MAX_NUMNODES - 1])) {
 		mode = pol->mode;
-		flags = pol->flags;
+		/*
+		 * Filter out internal flags and also move user flags to lsb for
+		 * easy lookup, matching the policy_flags[] indices.
+		 */
+		flags = (pol->flags & MPOL_MODE_FLAGS) >>
+			__ffs(MPOL_MODE_FLAGS);
 	}
 
 	switch (mode) {
@@ -3329,29 +3347,31 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
 		return;
 	}
 
-	p += snprintf(p, maxlen, "%s", policy_modes[mode]);
+	res = scnprintf(p, maxlen, "%s", policy_modes[mode]);
+	p += res;
+	maxlen -= res;
 
-	if (flags & MPOL_MODE_FLAGS) {
-		p += snprintf(p, buffer + maxlen - p, "=");
+	cnt = 0;
+	for_each_set_bit(bit, &flags,
+			 __fls(MPOL_MODE_FLAGS) - __ffs(MPOL_MODE_FLAGS) + 1) {
+		char prefix;
 
-		/*
-		 * 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 (WARN_ON_ONCE(bit >= ARRAY_SIZE(policy_flags) ||
+				 !policy_flags[bit]))
+			continue;
 
-		if (flags & MPOL_F_NUMA_BALANCING) {
-			if (hweight16(flags & MPOL_MODE_FLAGS) > 1)
-				p += snprintf(p, buffer + maxlen - p, "|");
-			p += snprintf(p, buffer + maxlen - p, "balancing");
-		}
+		if (cnt++ == 0)
+			prefix = '=';
+		else
+			prefix = '|';
+
+		res = scnprintf(p, maxlen, "%c%s", prefix, policy_flags[bit]);
+		p += res;
+		maxlen -= res;
 	}
 
 	if (!nodes_empty(nodes))
-		p += scnprintf(p, buffer + maxlen - p, ":%*pbl",
-			       nodemask_pr_args(&nodes));
+		scnprintf(p, maxlen, ":%*pbl", nodemask_pr_args(&nodes));
 }
 
 #ifdef CONFIG_SYSFS
-- 
2.44.0

[PATCH 3/3] mm/numa_balancing: Allow setting numa balancing for tmpfs

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Since MPOL_F_NUMA_BALANCING was added some time ago, lets also recognise
it as a valid tmpfs mount option.

As an extension to the exiting format:

  mpol=<mode>[=<flag>][:nodelist]

Multiple flags are added in a list separated by vertical bars, such as:

  mpol=<mode>[=<flag>[|<flag>]...][:nodelist]

For example:

 $ mount none -t tmpfs mnt -o mpol=bind=balancing:2,0
 $ mount | grep mpol
 none on ... type tmpfs (rw,relatime,mpol=bind=balancing:0,2)

Or:

 $ mount none mnt -t tmpfs -o "mpol=bind=balancing|static:1,0"
 $ mount | grep mpol
 none on ... type tmpfs (rw,relatime,mpol=bind=balancing|static:0-1)

I am not sure to be honest if this is useful for tmpfs, but is just an
asymmetry I noticed while doing stuff in this area.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Huang Ying <ying.huang@intel.com>
Cc: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 mm/mempolicy.c | 55 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 12 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 77488878d8ca..c69bf5438731 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -3162,6 +3162,47 @@ static const char * const policy_flags[] = {
 };
 
 #ifdef CONFIG_TMPFS
+static int mpol_lookup_flag(char *str)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(policy_flags); i++) {
+		if (policy_flags[i] && !strcmp(str, policy_flags[i]))
+			return BIT(i + __ffs(MPOL_MODE_FLAGS));
+	}
+
+	return -1;
+}
+
+static bool mpol_parse_flags(char *str, int *mode, unsigned short *flags)
+{
+	char buf[128], *opt;
+	int flag;
+
+	/* Make a local copy since caller wants the original untouched. */
+	if (WARN_ON_ONCE(strscpy(buf, str, sizeof(buf)) < 0))
+		return false;
+
+	str = buf;
+	for (;;) {
+		opt = strsep(&str, "|");
+
+		if (!opt)
+			break;
+		else if (*opt == '\0')
+			continue;
+
+		flag = mpol_lookup_flag(opt);
+		if (flag < 0)
+			return false;
+		*flags |= flag;
+	}
+
+	*mode |= *flags;
+
+	return sanitize_mpol_flags(mode, flags) == 0;
+}
+
 /**
  * mpol_parse_str - parse string to mempolicy, for tmpfs mpol mount option.
  * @str:  string containing mempolicy to parse
@@ -3247,18 +3288,8 @@ int mpol_parse_str(char *str, struct mempolicy **mpol)
 	}
 
 	mode_flags = 0;
-	if (flags) {
-		/*
-		 * Currently, we only support two mutually exclusive
-		 * mode flags.
-		 */
-		if (!strcmp(flags, "static"))
-			mode_flags |= MPOL_F_STATIC_NODES;
-		else if (!strcmp(flags, "relative"))
-			mode_flags |= MPOL_F_RELATIVE_NODES;
-		else
-			goto out;
-	}
+	if (flags && !mpol_parse_flags(flags, &mode, &mode_flags))
+		goto out;
 
 	new = mpol_new(mode, mode_flags, &nodes);
 	if (IS_ERR(new))
-- 
2.44.0