| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z6joYmcjyT8eY32H@thinkpad>
Date: Sun, 9 Feb 2025 12:40:15 -0500
From: Yury Norov <yury.norov@...il.com>
To: Andrea Righi <arighi@...dia.com>
Cc: Tejun Heo <tj@...nel.org>, David Vernet <void@...ifault.com>,
Changwoo Min <changwoo@...lia.com>, Ingo Molnar <mingo@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Juri Lelli <juri.lelli@...hat.com>,
Vincent Guittot <vincent.guittot@...aro.org>,
Dietmar Eggemann <dietmar.eggemann@....com>,
Steven Rostedt <rostedt@...dmis.org>,
Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
Valentin Schneider <vschneid@...hat.com>, Ian May <ianm@...dia.com>,
bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] mm/numa: Introduce numa_nearest_nodemask()
On Fri, Feb 07, 2025 at 09:40:48PM +0100, Andrea Righi wrote:
> Introduce the new helper numa_nearest_nodemask() to find the closest
> node, in a specified nodemask and state, from a given starting node.
>
> Returns MAX_NUMNODES if no node is found.
>
> Cc: Yury Norov <yury.norov@...il.com>
> Signed-off-by: Andrea Righi <arighi@...dia.com>
> ---
> include/linux/nodemask_types.h | 6 +++++-
> include/linux/numa.h | 8 +++++++
> mm/mempolicy.c | 38 ++++++++++++++++++++++++++++++++++
> 3 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/nodemask_types.h b/include/linux/nodemask_types.h
> index 6b28d97ea6ed0..8d0b7a66c3a49 100644
> --- a/include/linux/nodemask_types.h
> +++ b/include/linux/nodemask_types.h
> @@ -5,6 +5,10 @@
> #include <linux/bitops.h>
> #include <linux/numa.h>
>
> -typedef struct { DECLARE_BITMAP(bits, MAX_NUMNODES); } nodemask_t;
> +struct nodemask {
> + DECLARE_BITMAP(bits, MAX_NUMNODES);
> +};
> +
> +typedef struct nodemask nodemask_t;
>
> #endif /* __LINUX_NODEMASK_TYPES_H */
> diff --git a/include/linux/numa.h b/include/linux/numa.h
> index 3567e40329ebc..a549b87d1fca5 100644
> --- a/include/linux/numa.h
> +++ b/include/linux/numa.h
> @@ -27,6 +27,8 @@ static inline bool numa_valid_node(int nid)
> #define __initdata_or_meminfo __initdata
> #endif
>
> +struct nodemask;
Numa should include this via linux/nodemask_types.h, or maybe
nodemask.h.
> +
> #ifdef CONFIG_NUMA
> #include <asm/sparsemem.h>
>
> @@ -38,6 +40,7 @@ void __init alloc_offline_node_data(int nid);
>
> /* Generic implementation available */
> int numa_nearest_node(int node, unsigned int state);
> +int numa_nearest_nodemask(int node, unsigned int state, struct nodemask *mask);
>
> #ifndef memory_add_physaddr_to_nid
> int memory_add_physaddr_to_nid(u64 start);
> @@ -55,6 +58,11 @@ static inline int numa_nearest_node(int node, unsigned int state)
> return NUMA_NO_NODE;
> }
>
> +static inline int numa_nearest_nodemask(int node, unsigned int state, struct nodemask *mask)
> +{
> + return NUMA_NO_NODE;
> +}
> +
> static inline int memory_add_physaddr_to_nid(u64 start)
> {
> return 0;
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 162407fbf2bc7..1cfee509c7229 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -196,6 +196,44 @@ int numa_nearest_node(int node, unsigned int state)
> }
> EXPORT_SYMBOL_GPL(numa_nearest_node);
>
> +/**
> + * numa_nearest_nodemask - Find the node in @mask at the nearest distance
> + * from @node.
> + *
So, I have a feeling about this whole naming scheme. At first, this
function (and the existing numa_nearest_node()) searches for something,
but doesn't begin with find_, search_ or similar. Second, the naming
of existing numa_nearest_node() doesn't reflect that it searches
against the state. Should we always include some state for search? If
so, we can skip mentioning the state, otherwise it should be in the
name, I guess...
The problem is that I have no idea for better naming, and I have no
understanding about the future of this functions family. If it's just
numa_nearest_node() and numa_nearest_nodemask(), I'm OK to go this
way. If we'll add more flavors similarly to find_bit() family, we
could probably discuss a naming scheme.
Also, mm/mempolicy.c is a historical place for them, but maybe we need
to move it somewhere else?
Any thoughts appreciated.
> + * @node: the node to start the search from.
> + * @state: the node state to filter nodes by.
> + * @mask: a pointer to a nodemask representing the allowed nodes.
> + *
> + * This function iterates over all nodes in the given state and calculates
> + * the distance to the starting node.
> + *
> + * Returns the node ID in @mask that is the closest in terms of distance
> + * from @node, or MAX_NUMNODES if no node is found.
> + */
> +int numa_nearest_nodemask(int node, unsigned int state, nodemask_t *mask)
Your only user calls the function with N_POSSIBLE:
s32 scx_pick_idle_cpu(const struct cpumask *cpus_allowed, int node, u64 flags)
{
nodemask_t unvisited = NODE_MASK_ALL;
if (!static_branch_maybe(CONFIG_NUMA, &scx_builtin_idle_per_node))
return pick_idle_cpu_from_node(cpus_allowed, NUMA_NO_NODE, flags);
for_each_numa_node(node, unvisited, N_POSSIBLE)
do_something();
}
Which means you don't need the state at all. Even more, you don't
need to initialize the unvisited mask before checking the static
branch:
s32 scx_pick_idle_cpu(const struct cpumask *cpus_allowed, int node, u64 flags)
{
nodemask_t unvisited;
if (!static_branch_maybe(CONFIG_NUMA, &scx_builtin_idle_per_node))
return pick_idle_cpu_from_node(cpus_allowed, NUMA_NO_NODE, flags);
nodes_clear(unvisited);
for_each_numa_node(node, unvisited)
do_something();
}
If you need some state other than N_POSSIBLE, you can do it similarly:
nodemask_complement(unvisited, N_CPU);
/* Only N_CPU nodes iterated */
for_each_numa_node(node, unvisited)
do_something();
> +{
> + int dist, n, min_dist = INT_MAX, min_node = MAX_NUMNODES;
> +
> + if (node == NUMA_NO_NODE)
> + return MAX_NUMNODES;
> +
> + if (node_state(node, state) && node_isset(node, *mask))
> + return node;
This is correct, but why do we need this special case? If distance to
local node is always 0, and distance to remote node is always greater
than 0, the normal search will return local node, right? Is that a
performance trick? If so, can you put a comment please? Otherwise,
maybe just drop it?
> +
> + for_each_node_state(n, state) {
> + if (!node_isset(n, *mask))
> + continue;
for_each_node_state_and_mask(n, state, mask)
Or if you take the above consideration, just
for_each_node_mask(n, mask)
> + dist = node_distance(node, n);
> + if (dist < min_dist) {
> + min_dist = dist;
> + min_node = n;
> + }
> + }
> +
> + return min_node;
> +}
> +EXPORT_SYMBOL_GPL(numa_nearest_nodemask);
> +
> struct mempolicy *get_task_policy(struct task_struct *p)
> {
> struct mempolicy *pol = p->mempolicy;
> --
> 2.48.1
Powered by blists - more mailing lists