| Message ID | 20210713084541.7958-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v1,1/3] kernel.h: Don't pollute header with single user macros | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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.shevchenko@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
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. > > 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.shevchenko@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. Fair enough. I will remove these conversions from the patch in v2. Thanks for review!
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
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,
On Tue, Jul 13, 2021 at 3:21 PM Herbert Xu <herbert@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.
> 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)
On Tue, Jul 13, 2021 at 1:23 PM Greg Kroah-Hartman <gregkh@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
On Tue, Jul 13, 2021 at 08:39:22PM +0200, Miguel Ojeda wrote: > On Tue, Jul 13, 2021 at 1:23 PM Greg Kroah-Hartman > <gregkh@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`). It does almost 2% (steady) speedup. I will send a v2 with methodology and numbers of testing.
On Thu, Oct 07, 2021 at 12:20:58PM +0300, Andy Shevchenko wrote: > On Tue, Jul 13, 2021 at 08:39:22PM +0200, Miguel Ojeda wrote: > > On Tue, Jul 13, 2021 at 1:23 PM Greg Kroah-Hartman > > <gregkh@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`). > > It does almost 2% (steady) speedup. I will send a v2 with methodology > and numbers of testing. Seems it's slightly different Cc list, so TWIMC the v2 is here: https://lore.kernel.org/linux-media/20211007095129.22037-5-andriy.shevchenko@linux.intel.com/T/#u
On Thu, Oct 7, 2021 at 11:21 AM Andy Shevchenko <andriy.shevchenko@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
On Thu, Oct 07, 2021 at 05:39:56PM +0200, Miguel Ojeda wrote: > On Thu, Oct 7, 2021 at 11:21 AM Andy Shevchenko > <andriy.shevchenko@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! There is a v4 out, you are among others in Cc list.
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
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.shevchenko@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