[PATCH 0/2] lib/nodemask: inline wrappers around bitmap

On top of git@github.com:/norov/linux.git bitmap-for-next.

There are just 2 functions in nodemask.c, both are thin wrappers around
bitmap API. 1st patch of this series drops dependency on <asm/machdep.h
for powerpc, which prevents from inlining those nodemask functions, and
2nd patch inlines them and drops nodemask.c.

Yury Norov (2):
  powerpc: drop dependency on <asm/machdep.h> in archrandom.h
  lib/nodemask: inline next_node_in() and node_random()

 MAINTAINERS                           |  1 -
 arch/powerpc/include/asm/archrandom.h |  9 +-------
 arch/powerpc/kernel/setup-common.c    | 11 ++++++++++
 include/linux/nodemask.h              | 27 +++++++++++++++++++-----
 lib/Makefile                          |  2 +-
 lib/nodemask.c                        | 30 ---------------------------
 6 files changed, 35 insertions(+), 45 deletions(-)
 delete mode 100644 lib/nodemask.c

-- 
2.34.1

The functions are pretty thin wrappers around find_bit engine, and
keeping them in c-file prevents compiler from small_const_nbits()
optimization, which must take place for all systems with MAX_NUMNODES
less than BITS_PER_LONG (default is 16 for me).

Moving them to header file doesn't blow up the kernel size:
add/remove: 1/2 grow/shrink: 9/5 up/down: 968/-88 (880)

CC: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 MAINTAINERS              |  1 -
 include/linux/nodemask.h | 27 ++++++++++++++++++++++-----
 lib/Makefile             |  2 +-
 lib/nodemask.c           | 30 ------------------------------
 4 files changed, 23 insertions(+), 37 deletions(-)
 delete mode 100644 lib/nodemask.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7c0b8f28aa25..19c8d0ef1177 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -3540,7 +3540,6 @@ F:	lib/bitmap.c
 F:	lib/cpumask.c
 F:	lib/find_bit.c
 F:	lib/find_bit_benchmark.c
-F:	lib/nodemask.c
 F:	lib/test_bitmap.c
 F:	tools/include/linux/bitmap.h
 F:	tools/include/linux/find.h

diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h
index 0f233b76c9ce..48ebe4007955 100644
--- a/include/linux/nodemask.h
+++ b/include/linux/nodemask.h

@@ -94,6 +94,7 @@
 #include <linux/bitmap.h>
 #include <linux/minmax.h>
 #include <linux/numa.h>
+#include <linux/random.h>
 
 typedef struct { DECLARE_BITMAP(bits, MAX_NUMNODES); } nodemask_t;
 extern nodemask_t _unused_nodemask_arg_;

@@ -276,7 +277,14 @@ static inline unsigned int __next_node(int n, const nodemask_t *srcp)
  * the first node in src if needed.  Returns MAX_NUMNODES if src is empty.
  */
 #define next_node_in(n, src) __next_node_in((n), &(src))
-unsigned int __next_node_in(int node, const nodemask_t *srcp);
+static inline unsigned int __next_node_in(int node, const nodemask_t *srcp)
+{
+	unsigned int ret = __next_node(node, srcp);
+
+	if (ret == MAX_NUMNODES)
+		ret = __first_node(srcp);
+	return ret;
+}
 
 static inline void init_nodemask_of_node(nodemask_t *mask, int node)
 {

@@ -493,14 +501,23 @@ static inline int num_node_state(enum node_states state)
 
 #endif
 
+/*
+ * Return the bit number of a random bit set in the nodemask.
+ * (returns NUMA_NO_NODE if nodemask is empty)
+ */
+static inline int node_random(const nodemask_t *maskp)
+{
 #if defined(CONFIG_NUMA) && (MAX_NUMNODES > 1)
-extern int node_random(const nodemask_t *maskp);
+	int w, bit = NUMA_NO_NODE;
+
+	w = nodes_weight(*maskp);
+	if (w)
+		bit = find_nth_bit(maskp->bits, MAX_NUMNODES, get_random_int() % w);
+	return bit;
 #else
-static inline int node_random(const nodemask_t *mask)
-{
 	return 0;
-}
 #endif
+}
 
 #define node_online_map 	node_states[N_ONLINE]
 #define node_possible_map 	node_states[N_POSSIBLE]

diff --git a/lib/Makefile b/lib/Makefile
index f99bf61f8bbc..731cea0342d1 100644
--- a/lib/Makefile
+++ b/lib/Makefile

@@ -33,7 +33,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
 	 flex_proportions.o ratelimit.o show_mem.o \
 	 is_single_threaded.o plist.o decompress.o kobject_uevent.o \
 	 earlycpio.o seq_buf.o siphash.o dec_and_lock.o \
-	 nmi_backtrace.o nodemask.o win_minmax.o memcat_p.o \
+	 nmi_backtrace.o win_minmax.o memcat_p.o \
 	 buildid.o
 
 lib-$(CONFIG_PRINTK) += dump_stack.o

diff --git a/lib/nodemask.c b/lib/nodemask.c
deleted file mode 100644
index 7dad4ce8ff59..000000000000
--- a/lib/nodemask.c
+++ /dev/null

@@ -1,30 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-#include <linux/nodemask.h>
-#include <linux/module.h>
-#include <linux/random.h>
-
-unsigned int __next_node_in(int node, const nodemask_t *srcp)
-{
-	unsigned int ret = __next_node(node, srcp);
-
-	if (ret == MAX_NUMNODES)
-		ret = __first_node(srcp);
-	return ret;
-}
-EXPORT_SYMBOL(__next_node_in);
-
-#ifdef CONFIG_NUMA
-/*
- * Return the bit number of a random bit set in the nodemask.
- * (returns NUMA_NO_NODE if nodemask is empty)
- */
-int node_random(const nodemask_t *maskp)
-{
-	int w, bit = NUMA_NO_NODE;
-
-	w = nodes_weight(*maskp);
-	if (w)
-		bit = find_nth_bit(maskp->bits, MAX_NUMNODES, get_random_int() % w);
-	return bit;
-}
-#endif

-- 
2.34.1

archrandom.h includes <asm/machdep.h> to refer ppc_md. This causes
circular header dependency, if generic nodemask.h  includes random.h:

In file included from include/linux/cred.h:16,
                 from include/linux/seq_file.h:13,
                 from arch/powerpc/include/asm/machdep.h:6,
                 from arch/powerpc/include/asm/archrandom.h:5,
                 from include/linux/random.h:109,
                 from include/linux/nodemask.h:97,
                 from include/linux/list_lru.h:12,
                 from include/linux/fs.h:13,
                 from include/linux/compat.h:17,
                 from arch/powerpc/kernel/asm-offsets.c:12:
include/linux/sched.h:1203:9: error: unknown type name 'nodemask_t'
 1203 |         nodemask_t                      mems_allowed;
      |         ^~~~~~~~~~

Fix it by removing <asm/machdep.h> dependency from archrandom.h

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 arch/powerpc/include/asm/archrandom.h |  9 +--------
 arch/powerpc/kernel/setup-common.c    | 11 +++++++++++
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/archrandom.h b/arch/powerpc/include/asm/archrandom.h
index 9a53e29680f4..21def59ef1a6 100644
--- a/arch/powerpc/include/asm/archrandom.h
+++ b/arch/powerpc/include/asm/archrandom.h

@@ -4,7 +4,7 @@
 
 #ifdef CONFIG_ARCH_RANDOM
 
-#include <asm/machdep.h>
+bool __must_check arch_get_random_seed_long(unsigned long *v);
 
 static inline bool __must_check arch_get_random_long(unsigned long *v)
 {

@@ -16,13 +16,6 @@ static inline bool __must_check arch_get_random_int(unsigned int *v)
 	return false;
 }
 
-static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
-{
-	if (ppc_md.get_random_seed)
-		return ppc_md.get_random_seed(v);
-
-	return false;
-}
 
 static inline bool __must_check arch_get_random_seed_int(unsigned int *v)
 {

diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index eb0077b302e2..18c5fa5918bf 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c

@@ -171,6 +171,17 @@ EXPORT_SYMBOL_GPL(machine_power_off);
 void (*pm_power_off)(void);
 EXPORT_SYMBOL_GPL(pm_power_off);
 
+#ifdef CONFIG_ARCH_RANDOM
+bool __must_check arch_get_random_seed_long(unsigned long *v)
+{
+	if (ppc_md.get_random_seed)
+		return ppc_md.get_random_seed(v);
+
+	return false;
+}
+EXPORT_SYMBOL(arch_get_random_seed_long);
+#endif
+
 void machine_halt(void)
 {
 	machine_shutdown();

-- 
2.34.1

On Sun, Jul 24, 2022 at 12:19 AM Yury Norov [off-list ref] wrote:

archrandom.h includes <asm/machdep.h> to refer ppc_md. This causes
circular header dependency, if generic nodemask.h  includes random.h:

In file included from include/linux/cred.h:16,
                 from include/linux/seq_file.h:13,
                 from arch/powerpc/include/asm/machdep.h:6,
                 from arch/powerpc/include/asm/archrandom.h:5,
                 from include/linux/random.h:109,
                 from include/linux/nodemask.h:97,
                 from include/linux/list_lru.h:12,
                 from include/linux/fs.h:13,
                 from include/linux/compat.h:17,
                 from arch/powerpc/kernel/asm-offsets.c:12:
include/linux/sched.h:1203:9: error: unknown type name 'nodemask_t'
 1203 |         nodemask_t                      mems_allowed;
      |         ^~~~~~~~~~

Fix it by removing <asm/machdep.h> dependency from archrandom.h

...

 EXPORT_SYMBOL_GPL(pm_power_off);

^^^ (Note this and read below)

...

+EXPORT_SYMBOL(arch_get_random_seed_long);

It can't be like this. Brief browsing of the callees shows that.

-- 
With Best Regards,
Andy Shevchenko

Yury Norov [off-list ref] writes:

I'd rather we didn't have to force this out of line.

I think I see a different way to fix it, I'll just do some more build
tests.

cheers

Michael Ellerman [off-list ref] writes:

Yury Norov [off-list ref] writes:

quoted

archrandom.h includes <asm/machdep.h> to refer ppc_md. This causes
circular header dependency, if generic nodemask.h  includes random.h:

In file included from include/linux/cred.h:16,
                 from include/linux/seq_file.h:13,
                 from arch/powerpc/include/asm/machdep.h:6,
                 from arch/powerpc/include/asm/archrandom.h:5,
                 from include/linux/random.h:109,
                 from include/linux/nodemask.h:97,
                 from include/linux/list_lru.h:12,
                 from include/linux/fs.h:13,
                 from include/linux/compat.h:17,
                 from arch/powerpc/kernel/asm-offsets.c:12:
include/linux/sched.h:1203:9: error: unknown type name 'nodemask_t'
 1203 |         nodemask_t                      mems_allowed;
      |         ^~~~~~~~~~

Fix it by removing <asm/machdep.h> dependency from archrandom.h

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 arch/powerpc/include/asm/archrandom.h |  9 +--------
 arch/powerpc/kernel/setup-common.c    | 11 +++++++++++
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/archrandom.h b/arch/powerpc/include/asm/archrandom.h
index 9a53e29680f4..21def59ef1a6 100644
--- a/arch/powerpc/include/asm/archrandom.h
+++ b/arch/powerpc/include/asm/archrandom.h

@@ -4,7 +4,7 @@
 
 #ifdef CONFIG_ARCH_RANDOM
 
-#include <asm/machdep.h>
+bool __must_check arch_get_random_seed_long(unsigned long *v);
 
 static inline bool __must_check arch_get_random_long(unsigned long *v)
 {

@@ -16,13 +16,6 @@ static inline bool __must_check arch_get_random_int(unsigned int *v)
 	return false;
 }
 
-static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
-{
-	if (ppc_md.get_random_seed)
-		return ppc_md.get_random_seed(v);
-
-	return false;
-}

I'd rather we didn't have to force this out of line.

I think I see a different way to fix it, I'll just do some more build
tests.

Of course my idea didn't work :}

So I'll just ack your patch for now, and maybe I can get the headers
cleaned up in future to allow it to be out-of-line again.

cheers

Yury Norov [off-list ref] writes:

archrandom.h includes <asm/machdep.h> to refer ppc_md. This causes
circular header dependency, if generic nodemask.h  includes random.h:

In file included from include/linux/cred.h:16,
                 from include/linux/seq_file.h:13,
                 from arch/powerpc/include/asm/machdep.h:6,
                 from arch/powerpc/include/asm/archrandom.h:5,
                 from include/linux/random.h:109,
                 from include/linux/nodemask.h:97,
                 from include/linux/list_lru.h:12,
                 from include/linux/fs.h:13,
                 from include/linux/compat.h:17,
                 from arch/powerpc/kernel/asm-offsets.c:12:
include/linux/sched.h:1203:9: error: unknown type name 'nodemask_t'
 1203 |         nodemask_t                      mems_allowed;
      |         ^~~~~~~~~~

Fix it by removing <asm/machdep.h> dependency from archrandom.h

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 arch/powerpc/include/asm/archrandom.h |  9 +--------
 arch/powerpc/kernel/setup-common.c    | 11 +++++++++++
 2 files changed, 12 insertions(+), 8 deletions(-)

Acked-by: Michael Ellerman <mpe@ellerman.id.au>

cheers

On Mon, Jul 25, 2022 at 09:28:12AM +0200, Andy Shevchenko wrote:

On Sun, Jul 24, 2022 at 12:19 AM Yury Norov [off-list ref] wrote:

quoted

archrandom.h includes <asm/machdep.h> to refer ppc_md. This causes
circular header dependency, if generic nodemask.h  includes random.h:

In file included from include/linux/cred.h:16,
                 from include/linux/seq_file.h:13,
                 from arch/powerpc/include/asm/machdep.h:6,
                 from arch/powerpc/include/asm/archrandom.h:5,
                 from include/linux/random.h:109,
                 from include/linux/nodemask.h:97,
                 from include/linux/list_lru.h:12,
                 from include/linux/fs.h:13,
                 from include/linux/compat.h:17,
                 from arch/powerpc/kernel/asm-offsets.c:12:
include/linux/sched.h:1203:9: error: unknown type name 'nodemask_t'
 1203 |         nodemask_t                      mems_allowed;
      |         ^~~~~~~~~~

Fix it by removing <asm/machdep.h> dependency from archrandom.h

...

quoted

 EXPORT_SYMBOL_GPL(pm_power_off);

^^^ (Note this and read below)

...

quoted

+EXPORT_SYMBOL(arch_get_random_seed_long);

It can't be like this. Brief browsing of the callees shows that.

Is my understanding correct that you're suggesting to make it GPL?

ppc_md is exported with EXPORT_SYMBOL(), and the function is in header,
so it's available for non-GPL code now. I don't want to change it.

On Mon, Jul 25, 2022 at 10:22:13PM +1000, Michael Ellerman wrote:

Michael Ellerman [off-list ref] writes:

quoted

Yury Norov [off-list ref] writes:

quoted

archrandom.h includes <asm/machdep.h> to refer ppc_md. This causes
circular header dependency, if generic nodemask.h  includes random.h:

In file included from include/linux/cred.h:16,
                 from include/linux/seq_file.h:13,
                 from arch/powerpc/include/asm/machdep.h:6,
                 from arch/powerpc/include/asm/archrandom.h:5,
                 from include/linux/random.h:109,
                 from include/linux/nodemask.h:97,
                 from include/linux/list_lru.h:12,
                 from include/linux/fs.h:13,
                 from include/linux/compat.h:17,
                 from arch/powerpc/kernel/asm-offsets.c:12:
include/linux/sched.h:1203:9: error: unknown type name 'nodemask_t'
 1203 |         nodemask_t                      mems_allowed;
      |         ^~~~~~~~~~

Fix it by removing <asm/machdep.h> dependency from archrandom.h

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 arch/powerpc/include/asm/archrandom.h |  9 +--------
 arch/powerpc/kernel/setup-common.c    | 11 +++++++++++
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/archrandom.h b/arch/powerpc/include/asm/archrandom.h
index 9a53e29680f4..21def59ef1a6 100644
--- a/arch/powerpc/include/asm/archrandom.h
+++ b/arch/powerpc/include/asm/archrandom.h

@@ -4,7 +4,7 @@
 
 #ifdef CONFIG_ARCH_RANDOM
 
-#include <asm/machdep.h>
+bool __must_check arch_get_random_seed_long(unsigned long *v);
 
 static inline bool __must_check arch_get_random_long(unsigned long *v)
 {

@@ -16,13 +16,6 @@ static inline bool __must_check arch_get_random_int(unsigned int *v)
 	return false;
 }
 
-static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
-{
-	if (ppc_md.get_random_seed)
-		return ppc_md.get_random_seed(v);
-
-	return false;
-}

I'd rather we didn't have to force this out of line.

I think I see a different way to fix it, I'll just do some more build
tests.

Of course my idea didn't work :}

So I'll just ack your patch for now, and maybe I can get the headers
cleaned up in future to allow it to be out-of-line again.

I understand that it looks like a tradeoff - we inline a couple of small
functions with the cost of uninlining an almost innocent victim. 

The complete solution would be probably a splitting ppc_md declaration
out of asm/machdep.h. I wanted to do that, but I'm not a PPC guy, and
just don't know how to split the header correctly.

Anyways, thanks for the ack. Applied on bitmap-for-next.

Thanks,
Yury

On Mon, Jul 25, 2022 at 6:19 PM Yury Norov [off-list ref] wrote:

On Mon, Jul 25, 2022 at 09:28:12AM +0200, Andy Shevchenko wrote:

quoted

On Sun, Jul 24, 2022 at 12:19 AM Yury Norov [off-list ref] wrote:

...

quoted

quoted

 EXPORT_SYMBOL_GPL(pm_power_off);

^^^ (Note this and read below)

...

quoted

+EXPORT_SYMBOL(arch_get_random_seed_long);

It can't be like this. Brief browsing of the callees shows that.

Is my understanding correct that you're suggesting to make it GPL?

ppc_md is exported with EXPORT_SYMBOL(), and the function is in header,
so it's available for non-GPL code now. I don't want to change it.

The symbols your function calls are GPL. As far as I understand (not a
lawyer!) it logically one may not call GPL and pretend to be non-GPL.

-- 
With Best Regards,
Andy Shevchenko

On Mon, Jul 25, 2022 at 11:39:39PM +0200, Andy Shevchenko wrote:

On Mon, Jul 25, 2022 at 6:19 PM Yury Norov [off-list ref] wrote:

quoted

On Mon, Jul 25, 2022 at 09:28:12AM +0200, Andy Shevchenko wrote:

quoted

On Sun, Jul 24, 2022 at 12:19 AM Yury Norov [off-list ref] wrote:

...

quoted

quoted

quoted

 EXPORT_SYMBOL_GPL(pm_power_off);

^^^ (Note this and read below)

...

quoted

+EXPORT_SYMBOL(arch_get_random_seed_long);

It can't be like this. Brief browsing of the callees shows that.

Is my understanding correct that you're suggesting to make it GPL?

ppc_md is exported with EXPORT_SYMBOL(), and the function is in header,
so it's available for non-GPL code now. I don't want to change it.

The symbols your function calls are GPL. As far as I understand (not a
lawyer!) it logically one may not call GPL and pretend to be non-GPL.

Can you explain what you mean in details?

The function is:
        static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
        {
               if (ppc_md.get_random_seed)
                       return ppc_md.get_random_seed(v);

               return false;
        }

ppc_md is non-GPL:
 77 /* The main machine-dep calls structure
 78  */
 79 struct machdep_calls ppc_md;
 80 EXPORT_SYMBOL(ppc_md);

And get_random_seed is initialized in in arch/powerpc/platforms/powernv/rng.c
with different functions that are static and not exported at all. 

I don't understand where arch_get_random_seed_long calls GPL...

Yury Norov [off-list ref] writes:

On Mon, Jul 25, 2022 at 10:22:13PM +1000, Michael Ellerman wrote:

quoted

Michael Ellerman [off-list ref] writes:

quoted

Yury Norov [off-list ref] writes:

quoted

archrandom.h includes <asm/machdep.h> to refer ppc_md. This causes
circular header dependency, if generic nodemask.h  includes random.h:

In file included from include/linux/cred.h:16,
                 from include/linux/seq_file.h:13,
                 from arch/powerpc/include/asm/machdep.h:6,
                 from arch/powerpc/include/asm/archrandom.h:5,
                 from include/linux/random.h:109,
                 from include/linux/nodemask.h:97,
                 from include/linux/list_lru.h:12,
                 from include/linux/fs.h:13,
                 from include/linux/compat.h:17,
                 from arch/powerpc/kernel/asm-offsets.c:12:
include/linux/sched.h:1203:9: error: unknown type name 'nodemask_t'
 1203 |         nodemask_t                      mems_allowed;
      |         ^~~~~~~~~~

Fix it by removing <asm/machdep.h> dependency from archrandom.h

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 arch/powerpc/include/asm/archrandom.h |  9 +--------
 arch/powerpc/kernel/setup-common.c    | 11 +++++++++++
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/archrandom.h b/arch/powerpc/include/asm/archrandom.h
index 9a53e29680f4..21def59ef1a6 100644
--- a/arch/powerpc/include/asm/archrandom.h
+++ b/arch/powerpc/include/asm/archrandom.h

@@ -4,7 +4,7 @@
 
 #ifdef CONFIG_ARCH_RANDOM
 
-#include <asm/machdep.h>
+bool __must_check arch_get_random_seed_long(unsigned long *v);
 
 static inline bool __must_check arch_get_random_long(unsigned long *v)
 {

@@ -16,13 +16,6 @@ static inline bool __must_check arch_get_random_int(unsigned int *v)
 	return false;
 }
 
-static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
-{
-	if (ppc_md.get_random_seed)
-		return ppc_md.get_random_seed(v);
-
-	return false;
-}

I'd rather we didn't have to force this out of line.

I think I see a different way to fix it, I'll just do some more build
tests.

Of course my idea didn't work :}

So I'll just ack your patch for now, and maybe I can get the headers
cleaned up in future to allow it to be out-of-line again.

I understand that it looks like a tradeoff - we inline a couple of small
functions with the cost of uninlining an almost innocent victim. 

Yeah. The truth is the cost to access the RNG will far outweigh the cost
of that out-of-line call, so there's no real issue. But it's also such a
small function that it just cries out to be inlined :)

The complete solution would be probably a splitting ppc_md declaration
out of asm/machdep.h. I wanted to do that, but I'm not a PPC guy, and
just don't know how to split the header correctly.

I managed to drop the includes of seq_file.h and dma-mapping.h, which
seemed to fix the circular include problem, but there's a bit of fall
out in unrelated files. I think I can get that sorted though eventually.

Anyways, thanks for the ack. Applied on bitmap-for-next.

No worries.

cheers

On Tue, Jul 26, 2022 at 1:35 AM Yury Norov [off-list ref] wrote:

On Mon, Jul 25, 2022 at 11:39:39PM +0200, Andy Shevchenko wrote:

quoted

On Mon, Jul 25, 2022 at 6:19 PM Yury Norov [off-list ref] wrote:

quoted

On Mon, Jul 25, 2022 at 09:28:12AM +0200, Andy Shevchenko wrote:

quoted

On Sun, Jul 24, 2022 at 12:19 AM Yury Norov [off-list ref] wrote:

...

quoted

quoted

quoted

quoted

 EXPORT_SYMBOL_GPL(pm_power_off);

^^^ (Note this and read below)

...

quoted

+EXPORT_SYMBOL(arch_get_random_seed_long);

It can't be like this. Brief browsing of the callees shows that.

Is my understanding correct that you're suggesting to make it GPL?

ppc_md is exported with EXPORT_SYMBOL(), and the function is in header,
so it's available for non-GPL code now. I don't want to change it.

The symbols your function calls are GPL. As far as I understand (not a
lawyer!) it logically one may not call GPL and pretend to be non-GPL.

Can you explain what you mean in details?

The function is:
        static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
        {
               if (ppc_md.get_random_seed)
                       return ppc_md.get_random_seed(v);

               return false;
        }

ppc_md is non-GPL:
 77 /* The main machine-dep calls structure
 78  */
 79 struct machdep_calls ppc_md;
 80 EXPORT_SYMBOL(ppc_md);

What a mess...

And get_random_seed is initialized in in arch/powerpc/platforms/powernv/rng.c
with different functions that are static and not exported at all.

I don't understand where arch_get_random_seed_long calls GPL...

The ->get_random_seed() (aka "callees" in my previous mail) are all
GPL (maybe I missed one out of five which is non-GPL, but then it's
even more of a mess).

-- 
With Best Regards,
Andy Shevchenko

On Tue, Jul 26, 2022 at 8:13 AM Andy Shevchenko
[off-list ref] wrote:

On Tue, Jul 26, 2022 at 1:35 AM Yury Norov [off-list ref] wrote:

quoted

On Mon, Jul 25, 2022 at 11:39:39PM +0200, Andy Shevchenko wrote:

quoted

On Mon, Jul 25, 2022 at 6:19 PM Yury Norov [off-list ref] wrote:

quoted

On Mon, Jul 25, 2022 at 09:28:12AM +0200, Andy Shevchenko wrote:

quoted

On Sun, Jul 24, 2022 at 12:19 AM Yury Norov [off-list ref] wrote:

...

quoted

quoted

quoted

quoted

quoted

 EXPORT_SYMBOL_GPL(pm_power_off);

^^^ (Note this and read below)

...

quoted

+EXPORT_SYMBOL(arch_get_random_seed_long);

It can't be like this. Brief browsing of the callees shows that.

Is my understanding correct that you're suggesting to make it GPL?

ppc_md is exported with EXPORT_SYMBOL(), and the function is in header,
so it's available for non-GPL code now. I don't want to change it.

The symbols your function calls are GPL. As far as I understand (not a
lawyer!) it logically one may not call GPL and pretend to be non-GPL.

Can you explain what you mean in details?

The function is:
        static inline bool __must_check arch_get_random_seed_long(unsigned long *v)
        {
               if (ppc_md.get_random_seed)
                       return ppc_md.get_random_seed(v);

               return false;
        }

ppc_md is non-GPL:
 77 /* The main machine-dep calls structure
 78  */
 79 struct machdep_calls ppc_md;
 80 EXPORT_SYMBOL(ppc_md);

What a mess...

quoted

And get_random_seed is initialized in in arch/powerpc/platforms/powernv/rng.c
with different functions that are static and not exported at all.

To be clear, their license is defined in the file: "GPL-2.0-or-later".
But again, not a lawyer, just using my common sense.

quoted

I don't understand where arch_get_random_seed_long calls GPL...

The ->get_random_seed() (aka "callees" in my previous mail) are all
GPL (maybe I missed one out of five which is non-GPL, but then it's
even more of a mess).

-- 
With Best Regards,
Andy Shevchenko

Yury Norov [off-list ref] writes:

On Mon, Jul 25, 2022 at 09:28:12AM +0200, Andy Shevchenko wrote:

quoted

On Sun, Jul 24, 2022 at 12:19 AM Yury Norov [off-list ref] wrote:

quoted

archrandom.h includes <asm/machdep.h> to refer ppc_md. This causes
circular header dependency, if generic nodemask.h  includes random.h:

In file included from include/linux/cred.h:16,
                 from include/linux/seq_file.h:13,
                 from arch/powerpc/include/asm/machdep.h:6,
                 from arch/powerpc/include/asm/archrandom.h:5,
                 from include/linux/random.h:109,
                 from include/linux/nodemask.h:97,
                 from include/linux/list_lru.h:12,
                 from include/linux/fs.h:13,
                 from include/linux/compat.h:17,
                 from arch/powerpc/kernel/asm-offsets.c:12:
include/linux/sched.h:1203:9: error: unknown type name 'nodemask_t'
 1203 |         nodemask_t                      mems_allowed;
      |         ^~~~~~~~~~

Fix it by removing <asm/machdep.h> dependency from archrandom.h

...

quoted

 EXPORT_SYMBOL_GPL(pm_power_off);

^^^ (Note this and read below)

...

quoted

+EXPORT_SYMBOL(arch_get_random_seed_long);

It can't be like this. Brief browsing of the callees shows that.

Is my understanding correct that you're suggesting to make it GPL?

ppc_md is exported with EXPORT_SYMBOL(), and the function is in header,
so it's available for non-GPL code now. I don't want to change it.

That's true, your change maintains the status quo.

But I think we actually don't need it exported to modules, I think it's
a private detail of the RNG <-> architecture interface, not something
that modules should be calling.

So I think it's OK to drop the EXPORT_SYMBOL, either in this patch or a
subsequent one if you don't want to rebase.

cheers

On Tue, Jul 26, 2022 at 04:57:38PM +1000, Michael Ellerman wrote:

Yury Norov [off-list ref] writes:

quoted

On Mon, Jul 25, 2022 at 09:28:12AM +0200, Andy Shevchenko wrote:

quoted

On Sun, Jul 24, 2022 at 12:19 AM Yury Norov [off-list ref] wrote:

quoted

archrandom.h includes <asm/machdep.h> to refer ppc_md. This causes
circular header dependency, if generic nodemask.h  includes random.h:

In file included from include/linux/cred.h:16,
                 from include/linux/seq_file.h:13,
                 from arch/powerpc/include/asm/machdep.h:6,
                 from arch/powerpc/include/asm/archrandom.h:5,
                 from include/linux/random.h:109,
                 from include/linux/nodemask.h:97,
                 from include/linux/list_lru.h:12,
                 from include/linux/fs.h:13,
                 from include/linux/compat.h:17,
                 from arch/powerpc/kernel/asm-offsets.c:12:
include/linux/sched.h:1203:9: error: unknown type name 'nodemask_t'
 1203 |         nodemask_t                      mems_allowed;
      |         ^~~~~~~~~~

Fix it by removing <asm/machdep.h> dependency from archrandom.h

...

quoted

 EXPORT_SYMBOL_GPL(pm_power_off);

^^^ (Note this and read below)

...

quoted

+EXPORT_SYMBOL(arch_get_random_seed_long);

It can't be like this. Brief browsing of the callees shows that.

Is my understanding correct that you're suggesting to make it GPL?

ppc_md is exported with EXPORT_SYMBOL(), and the function is in header,
so it's available for non-GPL code now. I don't want to change it.

That's true, your change maintains the status quo.

But I think we actually don't need it exported to modules, I think it's
a private detail of the RNG <-> architecture interface, not something
that modules should be calling.

So I think it's OK to drop the EXPORT_SYMBOL, either in this patch or a
subsequent one if you don't want to rebase.

OK, changed.

Yury Norov [off-list ref] writes:

The patch that got merged (36d4b36b69590fed99356a4426c940a253a93800) still have lib/nodemask.c

On Fri, Aug 12, 2022 at 10:46:57AM +0530, Aneesh Kumar K.V wrote:

Yury Norov [off-list ref] writes:

quoted

The functions are pretty thin wrappers around find_bit engine, and
keeping them in c-file prevents compiler from small_const_nbits()
optimization, which must take place for all systems with MAX_NUMNODES
less than BITS_PER_LONG (default is 16 for me).

Moving them to header file doesn't blow up the kernel size:
add/remove: 1/2 grow/shrink: 9/5 up/down: 968/-88 (880)

CC: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 MAINTAINERS              |  1 -
 include/linux/nodemask.h | 27 ++++++++++++++++++++++-----
 lib/Makefile             |  2 +-
 lib/nodemask.c           | 30 ------------------------------
 4 files changed, 23 insertions(+), 37 deletions(-)
 delete mode 100644 lib/nodemask.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7c0b8f28aa25..19c8d0ef1177 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -3540,7 +3540,6 @@ F:	lib/bitmap.c
 F:	lib/cpumask.c
 F:	lib/find_bit.c
 F:	lib/find_bit_benchmark.c
-F:	lib/nodemask.c
 F:	lib/test_bitmap.c
 F:	tools/include/linux/bitmap.h
 F:	tools/include/linux/find.h

diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h
index 0f233b76c9ce..48ebe4007955 100644
--- a/include/linux/nodemask.h
+++ b/include/linux/nodemask.h

@@ -94,6 +94,7 @@
 #include <linux/bitmap.h>
 #include <linux/minmax.h>
 #include <linux/numa.h>
+#include <linux/random.h>
 
 typedef struct { DECLARE_BITMAP(bits, MAX_NUMNODES); } nodemask_t;
 extern nodemask_t _unused_nodemask_arg_;

@@ -276,7 +277,14 @@ static inline unsigned int __next_node(int n, const nodemask_t *srcp)
  * the first node in src if needed.  Returns MAX_NUMNODES if src is empty.
  */
 #define next_node_in(n, src) __next_node_in((n), &(src))
-unsigned int __next_node_in(int node, const nodemask_t *srcp);
+static inline unsigned int __next_node_in(int node, const nodemask_t *srcp)
+{
+	unsigned int ret = __next_node(node, srcp);
+
+	if (ret == MAX_NUMNODES)
+		ret = __first_node(srcp);
+	return ret;
+}
 
 static inline void init_nodemask_of_node(nodemask_t *mask, int node)
 {

@@ -493,14 +501,23 @@ static inline int num_node_state(enum node_states state)
 
 #endif
 
+/*
+ * Return the bit number of a random bit set in the nodemask.
+ * (returns NUMA_NO_NODE if nodemask is empty)
+ */
+static inline int node_random(const nodemask_t *maskp)
+{
 #if defined(CONFIG_NUMA) && (MAX_NUMNODES > 1)
-extern int node_random(const nodemask_t *maskp);
+	int w, bit = NUMA_NO_NODE;
+
+	w = nodes_weight(*maskp);
+	if (w)
+		bit = find_nth_bit(maskp->bits, MAX_NUMNODES, get_random_int() % w);
+	return bit;
 #else
-static inline int node_random(const nodemask_t *mask)
-{
 	return 0;
-}
 #endif
+}
 
 #define node_online_map 	node_states[N_ONLINE]
 #define node_possible_map 	node_states[N_POSSIBLE]

diff --git a/lib/Makefile b/lib/Makefile
index f99bf61f8bbc..731cea0342d1 100644
--- a/lib/Makefile
+++ b/lib/Makefile

@@ -33,7 +33,7 @@ lib-y := ctype.o string.o vsprintf.o cmdline.o \
 	 flex_proportions.o ratelimit.o show_mem.o \
 	 is_single_threaded.o plist.o decompress.o kobject_uevent.o \
 	 earlycpio.o seq_buf.o siphash.o dec_and_lock.o \
-	 nmi_backtrace.o nodemask.o win_minmax.o memcat_p.o \
+	 nmi_backtrace.o win_minmax.o memcat_p.o \
 	 buildid.o
 
 lib-$(CONFIG_PRINTK) += dump_stack.o

diff --git a/lib/nodemask.c b/lib/nodemask.c
deleted file mode 100644
index 7dad4ce8ff59..000000000000
--- a/lib/nodemask.c
+++ /dev/null

@@ -1,30 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-#include <linux/nodemask.h>
-#include <linux/module.h>
-#include <linux/random.h>
-
-unsigned int __next_node_in(int node, const nodemask_t *srcp)
-{
-	unsigned int ret = __next_node(node, srcp);
-
-	if (ret == MAX_NUMNODES)
-		ret = __first_node(srcp);
-	return ret;
-}
-EXPORT_SYMBOL(__next_node_in);
-
-#ifdef CONFIG_NUMA
-/*
- * Return the bit number of a random bit set in the nodemask.
- * (returns NUMA_NO_NODE if nodemask is empty)
- */
-int node_random(const nodemask_t *maskp)
-{
-	int w, bit = NUMA_NO_NODE;
-
-	w = nodes_weight(*maskp);
-	if (w)
-		bit = find_nth_bit(maskp->bits, MAX_NUMNODES, get_random_int() % w);
-	return bit;
-}
-#endif

-- 
2.34.1

The patch that got merged (36d4b36b69590fed99356a4426c940a253a93800) still have lib/nodemask.c

Thanks Aneesh. I'll send a fix shortly.