| 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: <Z6osTMrXVv54cES9@thinkpad>
Date: Mon, 10 Feb 2025 11:41:48 -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 Mon, Feb 10, 2025 at 09:28:36AM +0100, Andrea Righi wrote:
> Hi Yury,
>
> On Sun, Feb 09, 2025 at 12:40:15PM -0500, Yury Norov wrote:
> > 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.
>
> Hm... nodemask_types.h needs to include numa.h to resolve MAX_NUMNODES,
> Maybe we can move numa_nearest_nodemask() to linux/nodemask.h?
Maybe yes, but it's generally wrong. nodemask.h is a library, and
numa.h (generally, NUMA code) is one user. The right way to go is when
client code includes all necessary libs, not vise-versa.
Regarding MAX_NUMNODES, it's a natural property of nodemasks, and
should be declared in nodemask.h. The attached patch reverts the
inclusion paths dependency. I build-tested it against today's master
and x86_64 defconfig. Can you consider taking it and prepending your
series?
> > > #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.
>
> Personally I think adding "find_" to the name would be a bit redundant, as
> it seems quite obvious that it's finding the nearest node. It sounds a bit
> like the get_something() discussion and we can just use something().
>
> About adding "_state" to the name, it'd make sense since we have
> for_each_node_state/for_each_node(), but we would need to change
> numa_nearest_node() -> numa_nearest_node_state((), that is beyond the scope
> of this patch set.
>
> If I had to design this completely from scratch I'd probably propose
> something like this:
> - nearest_node_state(node, state)
> - nearest_node(node) -> nearest_node_state(node, N_POSSIBLE)
> - nearest_node_nodemask(node, nodemask) -> here the state can be managed
> with the nodemask (as you suggested below)
>
> But again, this is probably a more generic discussion that can be addressed
> in a separate thread.
Yes, it's not related to your series. Please just introduce
nearest_node_nodemask(node, nodemask) as your minimal required change.
I will do a necessary cleanup later, if needed.
Thanks,
Yury
>From 3ad589b371d671485d61d7debcb7526283a2f703 Mon Sep 17 00:00:00 2001
From: Yury Norov <yury.norov@...il.com>
Date: Mon, 10 Feb 2025 10:56:04 -0500
Subject: [PATCH] nodemask: numa: reorganize inclusion path
Nodemasks now pull linux/numa.h for MAX_NUMNODES and NUMA_NO_NODE
macros. This series makes numa.h depending on nodemasks, so we hit
a circular dependency.
Nodemasks library is highly employed by NUMA code, and it would be
logical to resolve the circular dependency by making NUMA headers
dependent nodemask.h.
Signed-off-by: Yury Norov <yury.norov@...il.com>
---
include/linux/nodemask.h | 1 -
include/linux/nodemask_types.h | 11 ++++++++++-
include/linux/numa.h | 10 +---------
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h
index 9fd7a0ce9c1a..27644a6edc6e 100644
--- a/include/linux/nodemask.h
+++ b/include/linux/nodemask.h
@@ -94,7 +94,6 @@
#include <linux/bitmap.h>
#include <linux/minmax.h>
#include <linux/nodemask_types.h>
-#include <linux/numa.h>
#include <linux/random.h>
extern nodemask_t _unused_nodemask_arg_;
diff --git a/include/linux/nodemask_types.h b/include/linux/nodemask_types.h
index 6b28d97ea6ed..f850a48742f1 100644
--- a/include/linux/nodemask_types.h
+++ b/include/linux/nodemask_types.h
@@ -3,7 +3,16 @@
#define __LINUX_NODEMASK_TYPES_H
#include <linux/bitops.h>
-#include <linux/numa.h>
+
+#ifdef CONFIG_NODES_SHIFT
+#define NODES_SHIFT CONFIG_NODES_SHIFT
+#else
+#define NODES_SHIFT 0
+#endif
+
+#define MAX_NUMNODES (1 << NODES_SHIFT)
+
+#define NUMA_NO_NODE (-1)
typedef struct { DECLARE_BITMAP(bits, MAX_NUMNODES); } nodemask_t;
diff --git a/include/linux/numa.h b/include/linux/numa.h
index 3567e40329eb..31d8bf8a951a 100644
--- a/include/linux/numa.h
+++ b/include/linux/numa.h
@@ -3,16 +3,8 @@
#define _LINUX_NUMA_H
#include <linux/init.h>
#include <linux/types.h>
+#include <linux/nodemask.h>
-#ifdef CONFIG_NODES_SHIFT
-#define NODES_SHIFT CONFIG_NODES_SHIFT
-#else
-#define NODES_SHIFT 0
-#endif
-
-#define MAX_NUMNODES (1 << NODES_SHIFT)
-
-#define NUMA_NO_NODE (-1)
#define NUMA_NO_MEMBLK (-1)
static inline bool numa_valid_node(int nid)
--
2.43.0
Powered by blists - more mailing lists