v2: https://lore.kernel.org/linux-media/20211007095129.22037-1-andriy.shevchenko...
The kernel.h is a set of something which is not related to each other and often used in non-crossed compilation units, especially when drivers need only one or two macro definitions from it.
Here is the split of container_of(). The goals are the following: - untwist the dependency hell a bit - drop kernel.h inclusion where it's only used for container_of() - speed up C preprocessing.
In v3: - split patch 2 to more patches (Greg) - exclude C changes (Herbert, Greg) - measure with kcbench, see below (Greg)
Cc: Thorsten Leemhuis regressions@leemhuis.info
People, like Greg KH and Miguel Ojeda, were asking about the latter. My methodology an testing has been provided in cover letter for v2 (see above) and here is what Greg KH insisted to have which is speedup of the kernel build.
$ kcbench -i 3 -j 64 -o $O -s $PWD --no-download -m Processor: Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz [88 CPUs] Cpufreq; Memory: powersave [intel_pstate]; 128823 MiB Linux running: 5.6.0-2-amd64 [x86_64] Compiler: gcc (Debian 10.3.0-11) 10.3.0 Linux compiled: 5.15.0-rc4 Config; Environment: allmodconfig; CCACHE_DISABLE="1" Build command: make vmlinux modules Filling caches: This might take a while... Done Run 1 (-j 64): 464.07 seconds / 7.76 kernels/hour [P:6001%] Run 2 (-j 64): 464.64 seconds / 7.75 kernels/hour [P:6000%] Run 3 (-j 64): 486.41 seconds / 7.40 kernels/hour [P:5727%]
$ kcbench -i 3 -j 64 -o $O -s $PWD --no-download -m Processor: Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz [88 CPUs] Cpufreq; Memory: powersave [intel_pstate]; 128823 MiB Linux running: 5.6.0-2-amd64 [x86_64] Compiler: gcc (Debian 10.3.0-11) 10.3.0 Linux compiled: 5.15.0-rc4 Config; Environment: allmodconfig; CCACHE_DISABLE="1" Build command: make vmlinux modules Filling caches: This might take a while... Done Run 1 (-j 64): 462.32 seconds / 7.79 kernels/hour [P:6009%] Run 2 (-j 64): 462.33 seconds / 7.79 kernels/hour [P:6006%] Run 3 (-j 64): 465.45 seconds / 7.73 kernels/hour [P:5999%]
Median values 464.64 before 462.33 after
Speedup: +0.5%
This supports and in align with my own approach, but shows lower numbers due to additional big take in the measurements (compilation without ccache).
Andy Shevchenko (7): kernel.h: Drop unneeded <linux/kernel.h> inclusion from other headers kernel.h: Split out container_of() and typeof_member() macros kunit: Replace kernel.h with the necessary inclusions list.h: Replace kernel.h with the necessary inclusions llist: Replace kernel.h with the necessary inclusions plist: Replace kernel.h with the necessary inclusions media: entity: Replace kernel.h with the necessary inclusions
include/kunit/test.h | 14 ++++++++++++-- include/linux/container_of.h | 37 ++++++++++++++++++++++++++++++++++++ include/linux/kernel.h | 31 +----------------------------- include/linux/kobject.h | 1 + include/linux/list.h | 6 ++++-- include/linux/llist.h | 4 +++- include/linux/plist.h | 5 ++++- include/linux/rwsem.h | 1 - include/linux/spinlock.h | 1 - include/media/media-entity.h | 3 ++- 10 files changed, 64 insertions(+), 39 deletions(-) create mode 100644 include/linux/container_of.h
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.shevchenko@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 352c6127cb90..f9348769e558 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 76a855b3ecde..c04e99edfe92 100644 --- a/include/linux/spinlock.h +++ b/include/linux/spinlock.h @@ -57,7 +57,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>
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_member() macros.
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.shevchenko@linux.intel.com --- include/linux/container_of.h | 37 ++++++++++++++++++++++++++++++++++++ include/linux/kernel.h | 31 +----------------------------- include/linux/kobject.h | 1 + 3 files changed, 39 insertions(+), 30 deletions(-) create mode 100644 include/linux/container_of.h
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 d416fe3165cb..ad9fdcce9dcf 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> @@ -482,36 +483,6 @@ static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { } #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. - * @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 efd56f990a46..bf8371e58b17 100644 --- a/include/linux/kobject.h +++ b/include/linux/kobject.h @@ -15,6 +15,7 @@ #ifndef _KOBJECT_H_ #define _KOBJECT_H_
+#include <linux/container_of.h> #include <linux/types.h> #include <linux/list.h> #include <linux/sysfs.h>
When kernel.h is used in the headers it adds a lot into dependency hell, especially when there are circular dependencies are involved.
Replace kernel.h inclusion with the list of what is really being used.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- include/kunit/test.h | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
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;
When kernel.h is used in the headers it adds a lot into dependency hell, especially when there are circular dependencies are involved.
Replace kernel.h inclusion with the list of what is really being used.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- include/linux/list.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
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.
When kernel.h is used in the headers it adds a lot into dependency hell, especially when there are circular dependencies are involved.
Replace kernel.h inclusion with the list of what is really being used.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- include/linux/llist.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
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;
When kernel.h is used in the headers it adds a lot into dependency hell, especially when there are circular dependencies are involved.
Replace kernel.h inclusion with the list of what is really being used.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- include/linux/plist.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
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;
When kernel.h is used in the headers it adds a lot into dependency hell, especially when there are circular dependencies are involved.
Replace kernel.h inclusion with the list of what is really being used.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- include/media/media-entity.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
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 */
On Thu, Oct 07, 2021 at 06:03:34PM +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_member() macros.
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.shevchenko@linux.intel.com
include/linux/container_of.h | 37 ++++++++++++++++++++++++++++++++++++ include/linux/kernel.h | 31 +----------------------------- include/linux/kobject.h | 1 +
Why did you touch kobject.h here?
It shouldn't have been needed to change for this commit.
Anyway, I really don't think this is all worth any more work at all, as I'm not going to be the one taking it...
thanks,
greg k-h
On Thu, Oct 07, 2021 at 05:09:09PM +0200, Greg Kroah-Hartman wrote:
On Thu, Oct 07, 2021 at 06:03:34PM +0300, Andy Shevchenko wrote:
...
Why did you touch kobject.h here?
Because it uses it, but indirectly. Now we may include it directly.
It shouldn't have been needed to change for this commit.
OK. I will split it to a separate change. Would it be okay?
Anyway, I really don't think this is all worth any more work at all, as I'm not going to be the one taking it...
It's fine. There are more people in favour of this change anyway.
Thanks for review!
linux-kselftest-mirror@lists.linaro.org
-
Andy Shevchenko -
Greg Kroah-Hartman