linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode
@ 2024-06-25 13:26 Tvrtko Ursulin
  2024-06-26  8:48 ` Huang, Ying
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Tvrtko Ursulin @ 2024-06-25 13:26 UTC (permalink / raw)
  To: linux-mm
  Cc: linux-kernel, kernel-dev, Tvrtko Ursulin, Huang Ying, Mel Gorman,
	Peter Zijlstra, Ingo Molnar, Rik van Riel, Johannes Weiner,
	Matthew Wilcox (Oracle),
	Dave Hansen, Andi Kleen, Michal Hocko, David Rientjes

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

If a task has had MPOL_F_NUMA_BALANCING set it is useful to show that in
procfs. Teach the mpol_to_str helper about its existance and while at it
update the comment to account for "weighted interleave" when suggesting
a recommended buffer size.

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: 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>
---
 mm/mempolicy.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index aec756ae5637..d147287c4505 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)
 {
@@ -3331,12 +3332,15 @@ 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
+		 * The below two flags 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)
+			p += snprintf(p, buffer + maxlen - p, "balancing");
 	}
 
 	if (!nodes_empty(nodes))
-- 
2.44.0



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode
  2024-06-25 13:26 [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode Tvrtko Ursulin
@ 2024-06-26  8:48 ` Huang, Ying
  2024-06-26 11:51   ` Tvrtko Ursulin
  2024-06-27 21:37 ` Andrew Morton
  2024-06-27 21:47 ` Matthew Wilcox
  2 siblings, 1 reply; 10+ messages in thread
From: Huang, Ying @ 2024-06-26  8:48 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: linux-mm, linux-kernel, kernel-dev, Tvrtko Ursulin, Mel Gorman,
	Peter Zijlstra, Ingo Molnar, Rik van Riel, Johannes Weiner,
	Matthew Wilcox (Oracle),
	Dave Hansen, Andi Kleen, Michal Hocko, David Rientjes

Tvrtko Ursulin <tursulin@igalia.com> writes:

> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> If a task has had MPOL_F_NUMA_BALANCING set it is useful to show that in

IIUC, MPOL_F_NUMA_BALANCING works for VMA area via mbind() too.

> procfs. Teach the mpol_to_str helper about its existance and while at it
> update the comment to account for "weighted interleave" when suggesting
> a recommended buffer size.

Otherwise LGTM, Thanks!

Reviewed-by: "Huang, Ying" <ying.huang@intel.com>

> 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: 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>
> ---
>  mm/mempolicy.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index aec756ae5637..d147287c4505 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)
>  {
> @@ -3331,12 +3332,15 @@ 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
> +		 * The below two flags 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)
> +			p += snprintf(p, buffer + maxlen - p, "balancing");
>  	}
>  
>  	if (!nodes_empty(nodes))


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode
  2024-06-26  8:48 ` Huang, Ying
@ 2024-06-26 11:51   ` Tvrtko Ursulin
  2024-06-27  3:04     ` Huang, Ying
  0 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2024-06-26 11:51 UTC (permalink / raw)
  To: Huang, Ying, Tvrtko Ursulin
  Cc: linux-mm, linux-kernel, kernel-dev, Mel Gorman, Peter Zijlstra,
	Ingo Molnar, Rik van Riel, Johannes Weiner,
	Matthew Wilcox (Oracle),
	Dave Hansen, Andi Kleen, Michal Hocko, David Rientjes


On 26/06/2024 09:48, Huang, Ying wrote:
> Tvrtko Ursulin <tursulin@igalia.com> writes:
> 
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>
>> If a task has had MPOL_F_NUMA_BALANCING set it is useful to show that in
> 
> IIUC, MPOL_F_NUMA_BALANCING works for VMA area via mbind() too.

Ah okay.. I think I forgot to actually check and went by what commit 
text of bda420b98505 said, which is probably outdated.

>> procfs. Teach the mpol_to_str helper about its existance and while at it
>> update the comment to account for "weighted interleave" when suggesting
>> a recommended buffer size.
> 
> Otherwise LGTM, Thanks!
> 
> Reviewed-by: "Huang, Ying" <ying.huang@intel.com>

Thank you! Would you have an idea of which tree this should go to aka 
which maintainer to ask to merge it?

Second question - I also have a patch which enables choosing balancing 
for tmpfs (mpol_parse_str) but I am unsure of its value. It would make 
things symmetrical, but is there some other benefit I don't know. Any 
thoughts on this?

Regards,

Tvrtko

>> 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: 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>
>> ---
>>   mm/mempolicy.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> index aec756ae5637..d147287c4505 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)
>>   {
>> @@ -3331,12 +3332,15 @@ 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
>> +		 * The below two flags 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)
>> +			p += snprintf(p, buffer + maxlen - p, "balancing");
>>   	}
>>   
>>   	if (!nodes_empty(nodes))


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode
  2024-06-26 11:51   ` Tvrtko Ursulin
@ 2024-06-27  3:04     ` Huang, Ying
  0 siblings, 0 replies; 10+ messages in thread
From: Huang, Ying @ 2024-06-27  3:04 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: Tvrtko Ursulin, linux-mm, linux-kernel, kernel-dev, Mel Gorman,
	Peter Zijlstra, Ingo Molnar, Rik van Riel, Johannes Weiner,
	Matthew Wilcox (Oracle),
	Dave Hansen, Andi Kleen, Michal Hocko, David Rientjes,
	Andrew Morton

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

> On 26/06/2024 09:48, Huang, Ying wrote:
>> Tvrtko Ursulin <tursulin@igalia.com> writes:
>> 
>>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>
>>> If a task has had MPOL_F_NUMA_BALANCING set it is useful to show that in
>> IIUC, MPOL_F_NUMA_BALANCING works for VMA area via mbind() too.
>
> Ah okay.. I think I forgot to actually check and went by what commit
> text of bda420b98505 said, which is probably outdated.
>
>>> procfs. Teach the mpol_to_str helper about its existance and while at it
>>> update the comment to account for "weighted interleave" when suggesting
>>> a recommended buffer size.
>> Otherwise LGTM, Thanks!
>> Reviewed-by: "Huang, Ying" <ying.huang@intel.com>
>
> Thank you! Would you have an idea of which tree this should go to aka
> which maintainer to ask to merge it?

You can use scripts/get_maintainer.pl in kernel source tree to find out
the maintainer.  And IIUC, Andrew Morton is the maintainer for this.

> Second question - I also have a patch which enables choosing balancing
> for tmpfs (mpol_parse_str) but I am unsure of its value. It would make
> things symmetrical, but is there some other benefit I don't know. Any
> thoughts on this?

I am not familiar with tmpfs NUMA policy, after checking the source
code, my understanding is that the change may have user visible effect.
For example, you can mount tmpfs with numa balancing enabled/disabled
and check whether pages can be balanced among NUMA nodes.

[snip]

--
Best Regards,
Huang, Ying


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode
  2024-06-25 13:26 [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode Tvrtko Ursulin
  2024-06-26  8:48 ` Huang, Ying
@ 2024-06-27 21:37 ` Andrew Morton
  2024-06-28  9:03   ` Tvrtko Ursulin
  2024-06-27 21:47 ` Matthew Wilcox
  2 siblings, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2024-06-27 21:37 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: linux-mm, linux-kernel, kernel-dev, Tvrtko Ursulin, Huang Ying,
	Mel Gorman, Peter Zijlstra, Ingo Molnar, Rik van Riel,
	Johannes Weiner, Matthew Wilcox (Oracle),
	Dave Hansen, Andi Kleen, Michal Hocko, David Rientjes

On Tue, 25 Jun 2024 14:26:05 +0100 Tvrtko Ursulin <tursulin@igalia.com> wrote:

> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> 
> If a task has had MPOL_F_NUMA_BALANCING set it is useful to show that in
> procfs. Teach the mpol_to_str helper about its existance and while at it
> update the comment to account for "weighted interleave" when suggesting
> a recommended buffer size.
> 
> ...
>
> --- 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)
>  {
> @@ -3331,12 +3332,15 @@ 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
> +		 * The below two flags 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)
> +			p += snprintf(p, buffer + maxlen - p, "balancing");
>  	}
>  
>  	if (!nodes_empty(nodes))

Is it strange to report this via mount options?  `static' and
`relative' can be set via mount options but afaict `balancing' cannot? 
I guess not...

Documentation/filesystems/tmpfs.rst appears to be a suitable place to
document this new userspace API please.




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode
  2024-06-25 13:26 [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode Tvrtko Ursulin
  2024-06-26  8:48 ` Huang, Ying
  2024-06-27 21:37 ` Andrew Morton
@ 2024-06-27 21:47 ` Matthew Wilcox
  2024-06-28  3:12   ` Huang, Ying
  2 siblings, 1 reply; 10+ messages in thread
From: Matthew Wilcox @ 2024-06-27 21:47 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: linux-mm, linux-kernel, kernel-dev, Tvrtko Ursulin, Huang Ying,
	Mel Gorman, Peter Zijlstra, Ingo Molnar, Rik van Riel,
	Johannes Weiner, Dave Hansen, Andi Kleen, Michal Hocko,
	David Rientjes

On Tue, Jun 25, 2024 at 02:26:05PM +0100, Tvrtko Ursulin wrote:
>  		/*
> -		 * Currently, the only defined flags are mutually exclusive
> +		 * The below two flags 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)
> +			p += snprintf(p, buffer + maxlen - p, "balancing");
>  	}

So if MPOL_F_STATIC_NODES and MPOL_F_NUMA_BALANCING are set, then we
get a string "staticbalancing"?  Is that intended?

Or are these three all mutually exclusive and that should have been
as "else if"?


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode
  2024-06-27 21:47 ` Matthew Wilcox
@ 2024-06-28  3:12   ` Huang, Ying
  2024-06-28  8:56     ` Tvrtko Ursulin
  0 siblings, 1 reply; 10+ messages in thread
From: Huang, Ying @ 2024-06-28  3:12 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Tvrtko Ursulin, linux-mm, linux-kernel, kernel-dev,
	Tvrtko Ursulin, Mel Gorman, Peter Zijlstra, Ingo Molnar,
	Rik van Riel, Johannes Weiner, Dave Hansen, Andi Kleen,
	Michal Hocko, David Rientjes

Hi, Matthew,

Matthew Wilcox <willy@infradead.org> writes:

> On Tue, Jun 25, 2024 at 02:26:05PM +0100, Tvrtko Ursulin wrote:
>>  		/*
>> -		 * Currently, the only defined flags are mutually exclusive
>> +		 * The below two flags 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)
>> +			p += snprintf(p, buffer + maxlen - p, "balancing");
>>  	}
>
> So if MPOL_F_STATIC_NODES and MPOL_F_NUMA_BALANCING are set, then we
> get a string "staticbalancing"?  Is that intended?
>
> Or are these three all mutually exclusive and that should have been
> as "else if"?

Yes, this is an issue!

Dig the git history, in commit 2291990ab36b ("mempolicy: clean-up
mpol-to-str() mempolicy formatting"), the support for multiple flags are
removed.  I think that we need to restore it.

Done some basic testing.  It was found that when MPOL_F_NUMA_BALANCING
is set, /proc/PID/numa_maps always display "default".  That is wrong.

This make me think that this patch has never been tested!

The "default" displaying is introduced in commit 8790c71a18e5
("mm/mempolicy.c: fix mempolicy printing in numa_maps").  We need to fix
it firstly for MPOL_F_NUMA_BALANCING with more accurate filtering.  The
fix needs to be backported to -stable kernel.

--
Best Regards,
Huang, Ying


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode
  2024-06-28  3:12   ` Huang, Ying
@ 2024-06-28  8:56     ` Tvrtko Ursulin
  2024-06-28  9:32       ` Huang, Ying
  0 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2024-06-28  8:56 UTC (permalink / raw)
  To: Huang, Ying, Matthew Wilcox
  Cc: Tvrtko Ursulin, linux-mm, linux-kernel, kernel-dev, Mel Gorman,
	Peter Zijlstra, Ingo Molnar, Rik van Riel, Johannes Weiner,
	Dave Hansen, Andi Kleen, Michal Hocko, David Rientjes


On 28/06/2024 04:12, Huang, Ying wrote:
> Hi, Matthew,
> 
> Matthew Wilcox <willy@infradead.org> writes:
> 
>> On Tue, Jun 25, 2024 at 02:26:05PM +0100, Tvrtko Ursulin wrote:
>>>   		/*
>>> -		 * Currently, the only defined flags are mutually exclusive
>>> +		 * The below two flags 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)
>>> +			p += snprintf(p, buffer + maxlen - p, "balancing");
>>>   	}
>>
>> So if MPOL_F_STATIC_NODES and MPOL_F_NUMA_BALANCING are set, then we
>> get a string "staticbalancing"?  Is that intended?
>>
>> Or are these three all mutually exclusive and that should have been
>> as "else if"?
> 
> Yes, this is an issue!

Sigh, my apologies. I was sure I tested it as this patch was part of a 
larger series I have, but then I decided to extract it and send out and 
the problems obviously go deeper. What I think happened is that I 
probably only tested the other direction, setting of via mpol_parse_str().

Andrew please dequeue it if you haven't already?

> Dig the git history, in commit 2291990ab36b ("mempolicy: clean-up
> mpol-to-str() mempolicy formatting"), the support for multiple flags are
> removed.  I think that we need to restore it.
> 
> Done some basic testing.  It was found that when MPOL_F_NUMA_BALANCING
> is set, /proc/PID/numa_maps always display "default".  That is wrong.
> 
> This make me think that this patch has never been tested!
> 
> The "default" displaying is introduced in commit 8790c71a18e5
> ("mm/mempolicy.c: fix mempolicy printing in numa_maps").  We need to fix
> it firstly for MPOL_F_NUMA_BALANCING with more accurate filtering.  The
> fix needs to be backported to -stable kernel.

Will you work on this or I can follow up if you want?

Regards,

Tvrtko


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode
  2024-06-27 21:37 ` Andrew Morton
@ 2024-06-28  9:03   ` Tvrtko Ursulin
  0 siblings, 0 replies; 10+ messages in thread
From: Tvrtko Ursulin @ 2024-06-28  9:03 UTC (permalink / raw)
  To: Andrew Morton, Tvrtko Ursulin
  Cc: linux-mm, linux-kernel, kernel-dev, Huang Ying, Mel Gorman,
	Peter Zijlstra, Ingo Molnar, Rik van Riel, Johannes Weiner,
	Matthew Wilcox (Oracle),
	Dave Hansen, Andi Kleen, Michal Hocko, David Rientjes


On 27/06/2024 22:37, Andrew Morton wrote:
> On Tue, 25 Jun 2024 14:26:05 +0100 Tvrtko Ursulin <tursulin@igalia.com> wrote:
> 
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>
>> If a task has had MPOL_F_NUMA_BALANCING set it is useful to show that in
>> procfs. Teach the mpol_to_str helper about its existance and while at it
>> update the comment to account for "weighted interleave" when suggesting
>> a recommended buffer size.
>>
>> ...
>>
>> --- 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)
>>   {
>> @@ -3331,12 +3332,15 @@ 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
>> +		 * The below two flags 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)
>> +			p += snprintf(p, buffer + maxlen - p, "balancing");
>>   	}
>>   
>>   	if (!nodes_empty(nodes))
> 
> Is it strange to report this via mount options?  `static' and
> `relative' can be set via mount options but afaict `balancing' cannot?
> I guess not...
> 
> Documentation/filesystems/tmpfs.rst appears to be a suitable place to
> document this new userspace API please.

That was the part I was unsure of - whether it makes sense to allow 
passing in 'balancing' via tmpfs.

I just noticed these few oddities while playing with some new NUMA 
policies and attempted (badly) to at least fix the fact 'balancing' is 
not shown in /proc/<pid>/numa_maps if selected via set_mempolicy/mbind.

So I also have a patch to allow 'balancing' to be parsed as tmpfs mount 
option but did not post it yet. Now that Ying has uncovered problems in 
this code go deeper we will see how this all develops.

Regards,

Tvrtko


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode
  2024-06-28  8:56     ` Tvrtko Ursulin
@ 2024-06-28  9:32       ` Huang, Ying
  0 siblings, 0 replies; 10+ messages in thread
From: Huang, Ying @ 2024-06-28  9:32 UTC (permalink / raw)
  To: Tvrtko Ursulin
  Cc: Matthew Wilcox, Tvrtko Ursulin, linux-mm, linux-kernel,
	kernel-dev, Mel Gorman, Peter Zijlstra, Ingo Molnar,
	Rik van Riel, Johannes Weiner, Dave Hansen, Andi Kleen,
	Michal Hocko, David Rientjes

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

> On 28/06/2024 04:12, Huang, Ying wrote:
>> Hi, Matthew,
>> Matthew Wilcox <willy@infradead.org> writes:
>> 
>>> On Tue, Jun 25, 2024 at 02:26:05PM +0100, Tvrtko Ursulin wrote:
>>>>   		/*
>>>> -		 * Currently, the only defined flags are mutually exclusive
>>>> +		 * The below two flags 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)
>>>> +			p += snprintf(p, buffer + maxlen - p, "balancing");
>>>>   	}
>>>
>>> So if MPOL_F_STATIC_NODES and MPOL_F_NUMA_BALANCING are set, then we
>>> get a string "staticbalancing"?  Is that intended?
>>>
>>> Or are these three all mutually exclusive and that should have been
>>> as "else if"?
>> Yes, this is an issue!
>
> Sigh, my apologies. I was sure I tested it as this patch was part of a
> larger series I have, but then I decided to extract it and send out
> and the problems obviously go deeper. What I think happened is that I
> probably only tested the other direction, setting of via
> mpol_parse_str().
>
> Andrew please dequeue it if you haven't already?
>
>> Dig the git history, in commit 2291990ab36b ("mempolicy: clean-up
>> mpol-to-str() mempolicy formatting"), the support for multiple flags are
>> removed.  I think that we need to restore it.
>> Done some basic testing.  It was found that when
>> MPOL_F_NUMA_BALANCING
>> is set, /proc/PID/numa_maps always display "default".  That is wrong.
>> This make me think that this patch has never been tested!
>> The "default" displaying is introduced in commit 8790c71a18e5
>> ("mm/mempolicy.c: fix mempolicy printing in numa_maps").  We need to fix
>> it firstly for MPOL_F_NUMA_BALANCING with more accurate filtering.  The
>> fix needs to be backported to -stable kernel.
>
> Will you work on this or I can follow up if you want?

Please go forward to work on this, Thanks!

--
Best Regards,
Huang, Ying


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-06-28  9:34 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-06-25 13:26 [PATCH] mm/numa_balancing: Teach mpol_to_str about the balancing mode Tvrtko Ursulin
2024-06-26  8:48 ` Huang, Ying
2024-06-26 11:51   ` Tvrtko Ursulin
2024-06-27  3:04     ` Huang, Ying
2024-06-27 21:37 ` Andrew Morton
2024-06-28  9:03   ` Tvrtko Ursulin
2024-06-27 21:47 ` Matthew Wilcox
2024-06-28  3:12   ` Huang, Ying
2024-06-28  8:56     ` Tvrtko Ursulin
2024-06-28  9:32       ` Huang, Ying

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox