[PATCH v1 1/3] kernel.h: Don't pollute header with single user macros

[Andy Shevchenko]

The COUNT_ARGS() and CONCATENATE() macros are used by a single user.
Let move them to it.

Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
---
include/linux/kernel.h | 7 -------
include/trace/bpf_probe.h | 7 +++++++
2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 1b2f0a7e00d6..743d3c9a3227 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -476,13 +476,6 @@ ftrace_vprintk(const char *fmt, va_list ap)
static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
#endif /* CONFIG_TRACING */

-/* This counts to 12. Any more, it will return 13th argument. */
-#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _n, X...) _n
-#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
-
-#define __CONCAT(a, b) a ## b
-#define CONCATENATE(a, b) __CONCAT(a, b)
-
/**
* container_of - cast a member of a structure out to the containing structure
* @ptr: the pointer to the member.
diff --git a/include/trace/bpf_probe.h b/include/trace/bpf_probe.h
index a23be89119aa..6f57c96f7dc3 100644
--- a/include/trace/bpf_probe.h
+++ b/include/trace/bpf_probe.h
@@ -27,6 +27,13 @@
#undef __perf_task
#define __perf_task(t) (t)

+/* This counts to 12. Any more, it will return 13th argument. */
+#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _n, X...) _n
+#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0)
+
+#define __CONCAT(a, b) a ## b
+#define CONCATENATE(a, b) __CONCAT(a, b)
+
/* cast any integer, pointer, or small struct to u64 */
#define UINTTYPE(size) \
__typeof__(__builtin_choose_expr(size == 1, (u8)1, \
--
2.30.2

[Andy Shevchenko]

There is no evidence we need kernel.h inclusion in certain headers.
Drop unneeded <linux/kernel.h> inclusion from other headers.

Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
---

include/linux/rwsem.h | 1 -
include/linux/spinlock.h | 1 -
2 files changed, 2 deletions(-)

diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index a66038d88878..1e430a207993 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -11,7 +11,6 @@
#include <linux/linkage.h>

#include <linux/types.h>
-#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/spinlock.h>
#include <linux/atomic.h>
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index a022992725be..159968c89dca 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -53,7 +53,6 @@
#include <linux/compiler.h>
#include <linux/irqflags.h>
#include <linux/thread_info.h>
-#include <linux/kernel.h>
#include <linux/stringify.h>
#include <linux/bottom_half.h>
#include <linux/lockdep.h>
--
2.30.2

[Andy Shevchenko]

kernel.h is being used as a dump for all kinds of stuff for a long time.
Here is the attempt cleaning it up by splitting out container_of() and
typeof_memeber() macros.

At the same time convert users in the header and other folders to use it.
Though for time being include new header back to kernel.h to avoid twisted
indirected includes for existing users.

Note, there are _a lot_ of headers and modules that include kernel.h solely
for one of these macros and this allows to unburden compiler for the twisted
inclusion paths and to make new code cleaner in the future.

Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
---

include/kunit/test.h | 14 ++++++++++++--
include/linux/container_of.h | 37 ++++++++++++++++++++++++++++++++++++
include/linux/kernel.h | 31 +-----------------------------
include/linux/kobject.h | 14 +++++++-------
include/linux/list.h | 6 ++++--
include/linux/llist.h | 4 +++-
include/linux/plist.h | 5 ++++-
include/media/media-entity.h | 3 ++-
lib/radix-tree.c | 6 +++++-
lib/rhashtable.c | 17 +++++++++++------
10 files changed, 86 insertions(+), 51 deletions(-)
create mode 100644 include/linux/container_of.h

diff --git a/include/kunit/test.h b/include/kunit/test.h
index 24b40e5c160b..d88d9f7ead0a 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -11,11 +11,21 @@

#include <kunit/assert.h>
#include <kunit/try-catch.h>
-#include <linux/kernel.h>
+
+#include <linux/compiler_attributes.h>
+#include <linux/container_of.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kconfig.h>
+#include <linux/kref.h>
+#include <linux/list.h>
#include <linux/module.h>
#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
#include <linux/types.h>
-#include <linux/kref.h>
+
+#include <asm/rwonce.h>

struct kunit_resource;

diff --git a/include/linux/container_of.h b/include/linux/container_of.h
new file mode 100644
index 000000000000..f6ee1be0e784
--- /dev/null
+++ b/include/linux/container_of.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_CONTAINER_OF_H
+#define _LINUX_CONTAINER_OF_H
+
+#define typeof_member(T, m) typeof(((T*)0)->m)
+
+/**
+ * container_of - cast a member of a structure out to the containing structure
+ * @ptr: the pointer to the member.
+ * @type: the type of the container struct this is embedded in.
+ * @member: the name of the member within the struct.
+ *
+ */
+#define container_of(ptr, type, member) ({ \
+ void *__mptr = (void *)(ptr); \
+ BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
+ !__same_type(*(ptr), void), \
+ "pointer type mismatch in container_of()"); \
+ ((type *)(__mptr - offsetof(type, member))); })
+
+/**
+ * container_of_safe - cast a member of a structure out to the containing structure
+ * @ptr: the pointer to the member.
+ * @type: the type of the container struct this is embedded in.
+ * @member: the name of the member within the struct.
+ *
+ * If IS_ERR_OR_NULL(ptr), ptr is returned unchanged.
+ */
+#define container_of_safe(ptr, type, member) ({ \
+ void *__mptr = (void *)(ptr); \
+ BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
+ !__same_type(*(ptr), void), \
+ "pointer type mismatch in container_of()"); \
+ IS_ERR_OR_NULL(__mptr) ? ERR_CAST(__mptr) : \
+ ((type *)(__mptr - offsetof(type, member))); })
+
+#endif /* _LINUX_CONTAINER_OF_H */
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 743d3c9a3227..55ea8e6bf31a 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -9,6 +9,7 @@
#include <linux/stddef.h>
#include <linux/types.h>
#include <linux/compiler.h>
+#include <linux/container_of.h>
#include <linux/bitops.h>
#include <linux/kstrtox.h>
#include <linux/log2.h>
@@ -476,36 +477,6 @@ ftrace_vprintk(const char *fmt, va_list ap)

static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
#endif /* CONFIG_TRACING */

-/**
- * container_of - cast a member of a structure out to the containing structure
- * @ptr: the pointer to the member.
- * @type: the type of the container struct this is embedded in.
- * @member: the name of the member within the struct.
- *
- */
-#define container_of(ptr, type, member) ({ \
- void *__mptr = (void *)(ptr); \
- BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
- !__same_type(*(ptr), void), \
- "pointer type mismatch in container_of()"); \
- ((type *)(__mptr - offsetof(type, member))); })
-
-/**
- * container_of_safe - cast a member of a structure out to the containing structure
- * @ptr: the pointer to the member.
- * @type: the type of the container struct this is embedded in.
- * @member: the name of the member within the struct.
- *
- * If IS_ERR_OR_NULL(ptr), ptr is returned unchanged.
- */
-#define container_of_safe(ptr, type, member) ({ \
- void *__mptr = (void *)(ptr); \
- BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
- !__same_type(*(ptr), void), \
- "pointer type mismatch in container_of()"); \
- IS_ERR_OR_NULL(__mptr) ? ERR_CAST(__mptr) : \
- ((type *)(__mptr - offsetof(type, member))); })
-
/* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
#ifdef CONFIG_FTRACE_MCOUNT_RECORD
# define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
diff --git a/include/linux/kobject.h b/include/linux/kobject.h
index ea30529fba08..c7284418a6d0 100644
--- a/include/linux/kobject.h
+++ b/include/linux/kobject.h
@@ -15,18 +15,18 @@
#ifndef _KOBJECT_H_
#define _KOBJECT_H_

-#include <linux/types.h>
-#include <linux/list.h>
-#include <linux/sysfs.h>
+#include <linux/atomic.h>
#include <linux/compiler.h>
-#include <linux/spinlock.h>
+#include <linux/container_of.h>
+#include <linux/list.h>
#include <linux/kref.h>
#include <linux/kobject_ns.h>
-#include <linux/kernel.h>
#include <linux/wait.h>
-#include <linux/atomic.h>
-#include <linux/workqueue.h>
+#include <linux/spinlock.h>
+#include <linux/sysfs.h>
+#include <linux/types.h>
#include <linux/uidgid.h>
+#include <linux/workqueue.h>

#define UEVENT_HELPER_PATH_LEN 256
#define UEVENT_NUM_ENVP 64 /* number of env pointers */
diff --git a/include/linux/list.h b/include/linux/list.h
index f2af4b4aa4e9..5dc679b373da 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -2,11 +2,13 @@
#ifndef _LINUX_LIST_H
#define _LINUX_LIST_H

+#include <linux/container_of.h>
+#include <linux/const.h>
#include <linux/types.h>
#include <linux/stddef.h>
#include <linux/poison.h>
-#include <linux/const.h>
-#include <linux/kernel.h>
+
+#include <asm/barrier.h>

/*
* Circular doubly linked list implementation.
diff --git a/include/linux/llist.h b/include/linux/llist.h
index 24f207b0190b..85bda2d02d65 100644
--- a/include/linux/llist.h
+++ b/include/linux/llist.h
@@ -49,7 +49,9 @@
*/

#include <linux/atomic.h>
-#include <linux/kernel.h>
+#include <linux/container_of.h>
+#include <linux/stddef.h>
+#include <linux/types.h>

struct llist_head {
struct llist_node *first;
diff --git a/include/linux/plist.h b/include/linux/plist.h
index 66bab1bca35c..0f352c1d3c80 100644
--- a/include/linux/plist.h
+++ b/include/linux/plist.h
@@ -73,8 +73,11 @@
#ifndef _LINUX_PLIST_H_
#define _LINUX_PLIST_H_

-#include <linux/kernel.h>
+#include <linux/container_of.h>
#include <linux/list.h>
+#include <linux/types.h>
+
+#include <asm/bug.h>

struct plist_head {
struct list_head node_list;
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 09737b47881f..fea489f03d57 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -13,10 +13,11 @@

#include <linux/bitmap.h>
#include <linux/bug.h>
+#include <linux/container_of.h>
#include <linux/fwnode.h>
-#include <linux/kernel.h>
#include <linux/list.h>
#include <linux/media.h>
+#include <linux/types.h>

/* Enums used internally at the media controller to represent graphs */

diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index b3afafe46fff..a0f346a095df 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -12,19 +12,21 @@
#include <linux/bitmap.h>
#include <linux/bitops.h>
#include <linux/bug.h>
+#include <linux/container_of.h>
#include <linux/cpu.h>
#include <linux/errno.h>
#include <linux/export.h>
#include <linux/idr.h>
#include <linux/init.h>
-#include <linux/kernel.h>
#include <linux/kmemleak.h>
+#include <linux/math.h>
#include <linux/percpu.h>
#include <linux/preempt.h> /* in_interrupt() */
#include <linux/radix-tree.h>
#include <linux/rcupdate.h>
#include <linux/slab.h>
#include <linux/string.h>
+#include <linux/types.h>
#include <linux/xarray.h>

/*
@@ -285,6 +287,8 @@ radix_tree_node_alloc(gfp_t gfp_mask, struct radix_tree_node *parent,
return ret;
}

+extern void radix_tree_node_rcu_free(struct rcu_head *head);
+
void radix_tree_node_rcu_free(struct rcu_head *head)
{
struct radix_tree_node *node =
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index e12bbfb240b8..64823476bb53 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -12,19 +12,24 @@
*/

#include <linux/atomic.h>
-#include <linux/kernel.h>
+#include <linux/bit_spinlock.h>
+#include <linux/container_of.h>
+#include <linux/err.h>
+#include <linux/export.h>
#include <linux/init.h>
+#include <linux/jhash.h>
+#include <linux/lockdep.h>
#include <linux/log2.h>
#include <linux/sched.h>
-#include <linux/rculist.h>
#include <linux/slab.h>
-#include <linux/vmalloc.h>
#include <linux/mm.h>
-#include <linux/jhash.h>
+#include <linux/mutex.h>
#include <linux/random.h>
+#include <linux/rculist.h>
#include <linux/rhashtable.h>
-#include <linux/err.h>
-#include <linux/export.h>
+#include <linux/types.h>
+#include <linux/vmalloc.h>
+#include <linux/workqueue.h>

#define HASH_DEFAULT_SIZE 64UL
#define HASH_MIN_SIZE 4U
--
2.30.2

[Greg Kroah-Hartman]

On Tue, Jul 13, 2021 at 11:45:41AM +0300, Andy Shevchenko wrote:
> kernel.h is being used as a dump for all kinds of stuff for a long time.
> Here is the attempt cleaning it up by splitting out container_of() and
> typeof_memeber() macros.

That feels messy, why? Reading one .h file for these common
macros/defines is fine, why are container_of and typeof somehow
deserving of their own .h files? What speedups are you seeing by
splitting this up?

> At the same time convert users in the header and other folders to use it.
> Though for time being include new header back to kernel.h to avoid twisted
> indirected includes for existing users.
>
> Note, there are _a lot_ of headers and modules that include kernel.h solely
> for one of these macros and this allows to unburden compiler for the twisted
> inclusion paths and to make new code cleaner in the future.
>
> Signed-off-by: Andy Shevchenko <andriy.s...@linux.intel.com>
> ---
> include/kunit/test.h | 14 ++++++++++++--
> include/linux/container_of.h | 37 ++++++++++++++++++++++++++++++++++++
> include/linux/kernel.h | 31 +-----------------------------
> include/linux/kobject.h | 14 +++++++-------

Why are all of these changes needed to kobject.h for this one change?
This diff:

> --- a/include/linux/kobject.h
> +++ b/include/linux/kobject.h
> @@ -15,18 +15,18 @@
> #ifndef _KOBJECT_H_
> #define _KOBJECT_H_
>
> -#include <linux/types.h>
> -#include <linux/list.h>
> -#include <linux/sysfs.h>
> +#include <linux/atomic.h>
> #include <linux/compiler.h>
> -#include <linux/spinlock.h>
> +#include <linux/container_of.h>
> +#include <linux/list.h>
> #include <linux/kref.h>
> #include <linux/kobject_ns.h>
> -#include <linux/kernel.h>
> #include <linux/wait.h>
> -#include <linux/atomic.h>
> -#include <linux/workqueue.h>
> +#include <linux/spinlock.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> #include <linux/uidgid.h>
> +#include <linux/workqueue.h>

Is a lot more changes than the "split the macros out" deserves.

Please make this a separate change, remember to only do one thing at a
time (this patch is at least 2 changes...)

so NAK, this change isn't ok as-is.

greg k-h

[Andy Shevchenko]

On Tue, Jul 13, 2021 at 12:37:46PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Jul 13, 2021 at 11:45:41AM +0300, Andy Shevchenko wrote:
> > kernel.h is being used as a dump for all kinds of stuff for a long time.
> > Here is the attempt cleaning it up by splitting out container_of() and
> > typeof_memeber() macros.
>
> That feels messy, why?

Because the headers in the kernel are messy.

> Reading one .h file for these common
> macros/defines is fine, why are container_of and typeof somehow
> deserving of their own .h files?

It's explained here. There are tons of drivers that includes kernel.h for only
a few or even solely for container_of() macro.

> What speedups are you seeing by
> splitting this up?

C preprocessing.

Fair enough. I will remove these conversions from the patch in v2.

Thanks for review!

--
With Best Regards,
Andy Shevchenko

[Greg Kroah-Hartman]

On Tue, Jul 13, 2021 at 02:16:17PM +0300, Andy Shevchenko wrote:
> On Tue, Jul 13, 2021 at 12:37:46PM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jul 13, 2021 at 11:45:41AM +0300, Andy Shevchenko wrote:
> > > kernel.h is being used as a dump for all kinds of stuff for a long time.
> > > Here is the attempt cleaning it up by splitting out container_of() and
> > > typeof_memeber() macros.
> >
> > That feels messy, why?
>
> Because the headers in the kernel are messy.

Life is messy and can not easily be partitioned into tiny pieces. That
way usually ends up being even messier in the end...

> > Reading one .h file for these common
> > macros/defines is fine, why are container_of and typeof somehow
> > deserving of their own .h files?
>
> It's explained here. There are tons of drivers that includes kernel.h for only
> a few or even solely for container_of() macro.

And why is that really a problem? kernel.h is in the cache and I would
be amazed if splitting this out actually made anything build faster.

> > What speedups are you seeing by
> > splitting this up?
>
> C preprocessing.

Numbers please.

greg k-h

[Herbert Xu]

On Tue, Jul 13, 2021 at 01:23:01PM +0200, Greg Kroah-Hartman wrote:
>
> Life is messy and can not easily be partitioned into tiny pieces. That
> way usually ends up being even messier in the end...

One advantage is less chance of header loops which very often
involve kernel.h and one of the most common reasons for other
header files to include kernel.h is to access container_of.

However, I don't see much point in touching *.c files that include
kernel.h.

Cheers,
--
Email: Herbert Xu <her...@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[Peter Zijlstra]

On Tue, Jul 13, 2021 at 11:45:39AM +0300, Andy Shevchenko wrote:
> The COUNT_ARGS() and CONCATENATE() macros are used by a single user.
> Let move them to it.

That seems to be because people like re-implementing it instead of
reusing existing ones:

arch/x86/include/asm/efi.h:#define __efi_nargs__(_0, _1, _2, _3, _4, _5, _6, _7, n, ...) \
arch/x86/include/asm/rmwcc.h:#define __RMWcc_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _n, X...) _n
include/linux/arm-smccc.h:#define ___count_args(_0, _1, _2, _3, _4, _5, _6, _7, _8, x, ...) x
include/linux/kernel.h:#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _n, X...) _n

[Andy Shevchenko]

Good catch!

I will redo this, thanks!

[Andy Shevchenko]

On Tue, Jul 13, 2021 at 3:21 PM Herbert Xu <her...@gondor.apana.org.au> wrote:
> On Tue, Jul 13, 2021 at 01:23:01PM +0200, Greg Kroah-Hartman wrote:
> >
> > Life is messy and can not easily be partitioned into tiny pieces. That
> > way usually ends up being even messier in the end...
>
> One advantage is less chance of header loops which very often
> involve kernel.h and one of the most common reasons for other
> header files to include kernel.h is to access container_of.

Thanks, yes, that's also one important point.

> However, I don't see much point in touching *.c files that include
> kernel.h.

The whole idea came when discussing drivers, esp. IIO ones. They often
are using ARRAY_SIZE() + container_of(). kernel.h is a big overkill
there.

[David Laight]

> The whole idea came when discussing drivers, esp. IIO ones. They often
> are using ARRAY_SIZE() + container_of(). kernel.h is a big overkill
> there.

It probably makes no difference.
Even apparently trivial includes pull in most of the world.

I managed to get a compile error from one of the defines
in sys/ioctl.h - the include sequence the compiler gave
me was about 20 deep.
I've forgotten where it started, but it meandered through
some arch/x86 directories.

I suspect that some files have a #include where just a
'struct foo' would suffice.

There will also be .h files which include both the 'public'
interface and 'private' definitions.
Sometimes splitting those can reduce the number of includes.
This is most noticeable compiling trivial drivers.

The other way to speed up compilations is to reduce the -I
path list to the compiler.
This is particularly significant if compiling over NFS.
(Well NFS might have changed, but...)

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[Miguel Ojeda]

On Tue, Jul 13, 2021 at 1:23 PM Greg Kroah-Hartman
<gre...@linuxfoundation.org> wrote:
>
> Life is messy and can not easily be partitioned into tiny pieces. That
> way usually ends up being even messier in the end...

I agree measurements would be ideal.

Having said that, even if it makes no performance difference, I think
it is reasonable to split things (within reason) and makes a bunch of
other things easier, plus sometimes one can enforce particular
conventions in the separate header (like I did when introducing
`compiler_attributes.h`).

Cheers,
Miguel

[Andy Shevchenko]

It does almost 2% (steady) speedup. I will send a v2 with methodology
and numbers of testing.

[Andy Shevchenko]

Seems it's slightly different Cc list, so TWIMC the v2 is here:
https://lore.kernel.org/linux-media/20211007095129.22037...@linux.intel.com/T/#u

[Miguel Ojeda]

On Thu, Oct 7, 2021 at 11:21 AM Andy Shevchenko
<andriy.s...@linux.intel.com> wrote:
>
> It does almost 2% (steady) speedup. I will send a v2 with methodology
> and numbers of testing.

Thanks for taking the time to get the numbers!

Cheers,
Miguel

[Andy Shevchenko]

There is a v4 out, you are among others in Cc list.