Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx> writes:
On 03/07/2024 08:57, Huang, Ying wrote:
Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx> writes:
On 03/07/2024 06:28, Huang, Ying wrote:After bda420b98505, MPOL_BIND with MPOL_F_NUMA_BALANCING will be
Tvrtko Ursulin <tursulin@xxxxxxxxxx> writes:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>Please split these 2 changes into 2 patches. Because we will need
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:
First we introduce a new internal flag MPOL_F_KERNEL and with it mark the
kernel's internal default and fallback policies (for tasks and/or VMAs
with no explicit policy set). By doing this we generalise the current
special casing and replace the incorrect 'default' with the correct
'bind'.
Secondly, we add a string representation and corresponding handling for
MPOL_F_NUMA_BALANCING. We do this by adding a sparse mapping array of
flags to names. With the sparseness being the downside, but with the
advantage of generalising and removing the "policy" from flags display.
to
back port the first one to -stable kernel.
Why two? AFAICT there wasn't a issue until bda420b98505, and to fix it
all changes from this patch are needed.
shown
as "default", which is a bug. While it's a new feature to show
"balancing". The first fix should be back-ported to -stable kernel
after bda420b98505. While we don't need to do that for the second one.
You lost me but it could be I am not at my best today so if you could
please explain more precisely what you mean?
When bda420b98505 got in, it added MPOL_F_NUMA_BALANCING. But there
was no "balancing" in mpol_to_str(). That's one fix for bda420b98505.
IMO, it's not a big issue to miss "balancing" in mpol_to_str(). It's
not absolutely necessary to backport this part.
But also it did not change the pre-existing check for MPOL_F_MORON
added in 8790c71a18e5, many years before it, which was the thing
causing bind+balancing to be printed as default. So that's the second
part of the fix. But also AFAICS to tag as fixes bda420b98505.
Making 8790c71a18e5 target of Fixes: does not IMO make sense though
because *at the time* of that patch it wasn't broken. What am I
missing?
Yes, we should use "Fixes: bda420b98505 ..." for this part. This is a
big issue, because "default" will be shown for MPOL_BIND, which is
totally wrong. We need to backport this fix. It's good for backporting
to keep it small and focused.
It's not legitimate (yet) to use MPOL_PREFERRED +End result:Can we avoid to introduce a new flag? Whether the following code
$ numactl -b -m 0-1,3 cat /proc/self/numa_maps
555559580000 bind=balancing:0-1,3 file=/usr/bin/cat mapped=3 active=0 N0=3 kernelpagesize_kB=16
...
v2:
* Fully fix by introducing MPOL_F_KERNEL.
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>
---
include/uapi/linux/mempolicy.h | 1 +
mm/mempolicy.c | 44 ++++++++++++++++++++++++----------
2 files changed, 32 insertions(+), 13 deletions(-)
diff --git a/include/uapi/linux/mempolicy.h b/include/uapi/linux/mempolicy.h
index 1f9bb10d1a47..bcf56ce9603b 100644
--- a/include/uapi/linux/mempolicy.h
+++ b/include/uapi/linux/mempolicy.h
@@ -64,6 +64,7 @@ enum {
#define MPOL_F_SHARED (1 << 0) /* identify shared policies */
#define MPOL_F_MOF (1 << 3) /* this policy wants migrate on fault */
#define MPOL_F_MORON (1 << 4) /* Migrate On protnone Reference On Node */
+#define MPOL_F_KERNEL (1 << 5) /* Kernel's internal policy */
/*
* These bit locations are exposed in the vm.zone_reclaim_mode sysctl
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index aec756ae5637..8ecc6d9f100a 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -134,6 +134,7 @@ enum zone_type policy_zone = 0;
static struct mempolicy default_policy = {
.refcnt = ATOMIC_INIT(1), /* never free it */
.mode = MPOL_LOCAL,
+ .flags = MPOL_F_KERNEL,
};
static struct mempolicy preferred_node_policy[MAX_NUMNODES];
@@ -3095,7 +3096,7 @@ void __init numa_policy_init(void)
preferred_node_policy[nid] = (struct mempolicy) {
.refcnt = ATOMIC_INIT(1),
.mode = MPOL_PREFERRED,
- .flags = MPOL_F_MOF | MPOL_F_MORON,
+ .flags = MPOL_F_MOF | MPOL_F_MORON | MPOL_F_KERNEL,
.nodes = nodemask_of_node(nid),
};
}
@@ -3150,6 +3151,12 @@ static const char * const policy_modes[] =
[MPOL_PREFERRED_MANY] = "prefer (many)",
};
+static const char * const policy_flags[] = {
+ [ilog2(MPOL_F_STATIC_NODES)] = "static",
+ [ilog2(MPOL_F_RELATIVE_NODES)] = "relative",
+ [ilog2(MPOL_F_NUMA_BALANCING)] = "balancing",
+};
+
#ifdef CONFIG_TMPFS
/**
* mpol_parse_str - parse string to mempolicy, for tmpfs mpol mount option.
@@ -3293,17 +3300,18 @@ 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)
{
char *p = buffer;
nodemask_t nodes = NODE_MASK_NONE;
unsigned short mode = MPOL_DEFAULT;
- unsigned short flags = 0;
+ unsigned long flags = 0;
- if (pol && pol != &default_policy && !(pol->flags &
MPOL_F_MORON)) {
+ if (!(pol->flags & MPOL_F_KERNEL)) {
work?
if (pol && pol != &default_policy && !(pol->mode !=
MPOL_PREFERRED) && !(pol->flags & MPOL_F_MORON))
But I think that this is kind of fragile. A flag is better. But
personally, I don't think MPOL_F_KERNEL is a good name, maybe
MPOL_F_DEFAULT?
I thought along the same lines, but as you have also shown we need to
exclude both default and preferred fallbacks so naming the flag
default did not feel best. MPOL_F_INTERNAL? MPOL_F_FALLBACK?
MPOL_F_SHOW_AS_DEFAULT? :))
What I dislike about the flag more is the fact internal flags are for
some reason in the uapi headers. And presumably we cannot zap them.
But I don't think we can check for MPOL_PREFERRED since it can be a
legitimate user set policy.
MPOL_F_NUMA_BALANCING.
Yes. I think that this is doable.
We could check for the address of preferred_node_policy[] members with
a loop covering all possible nids? If that will be the consensus I am
happy to change it. But flag feels more elegant and robust.
(unsigned long)addr >= (unsigned
long)(preferred_node_policy) && \
(unsigned long)addr < (unsigned long)(preferred_node_policy) + \
sizeof(preferred_node_policy)
Not the prettiest but at least in the spirit of the existing
&default_policy check. I can do that, no problem. If someone has a
different opinion please shout soon.
If you think that your format is better, please explain why you notmode = pol->mode;Please refer to commit 2291990ab36b ("mempolicy: clean-up
flags = pol->flags;
}
@@ -3328,15 +3336,25 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
p += snprintf(p, maxlen, "%s", policy_modes[mode]);
if (flags & MPOL_MODE_FLAGS) {
- p += snprintf(p, buffer + maxlen - p, "=");
+ unsigned int bit, cnt = 0;
- /*
- * Currently, the only defined 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");
+ for_each_set_bit(bit, &flags, ARRAY_SIZE(policy_flags)) {
+ if (bit <= ilog2(MPOL_F_KERNEL))
+ continue;
+
+ if (cnt == 0)
+ p += snprintf(p, buffer + maxlen - p, "=");
+ else
+ p += snprintf(p, buffer + maxlen - p, ",");
+
+ if (WARN_ON_ONCE(!policy_flags[bit]))
+ p += snprintf(p, buffer + maxlen - p, "bit%u",
+ bit);
+ else
+ p += snprintf(p, buffer + maxlen - p,
+ policy_flags[bit]);
+ cnt++;
+ }
mpol-to-str()
mempolicy formatting") for the original format.
That was in 2008 so long time ago and in the meantime there were no
bars. The format in this patch tries to align with the input format
and I think it manages, apart from deciding to print unknown flags as
bit numbers (which is most probably an irrelevant difference). Why do
you think the pre-2008 format is better?
use
the original format in the patch description. You can also show
examples to compare.
Because there is no "old" format? If you refer to the one which ended
in 2008. Or if you refer to the one this patch replaces, then it is
effectively the same format for a single flag. And for multiple flags
before this patch that wasn't a possibility. So I am not sure what I
would include as a comparison. Broken "default" vs
"bind=balancing:0-1"? Am I missing something?
In the old format (not in the old code), it is,
bind=relative|balancing:0-1
while in your format,
bind=relative,balancing:0-1
Please explain why you make the change.