[PATCH v4 0/7] kernel.h further split

[PATCH v4 0/7] kernel.h further split

[Andy Shevchenko]

v3: https://lore.kernel.org/linux-media/20211007150339.28910-1-andriy.shevchenko@linux.intel.com/T/#u
v2: https://lore.kernel.org/linux-media/20211007095129.22037-1-andriy.shevchenko@linux.intel.com/T/#u

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.

The build speedup is
	1.83% (ccache approach, see v2 cover letter for the details)
	0.5%  (kcbench approach, see v3 cover letter for the details)

In v4:
- dropped kobject.h change (Greg)
- Cc'ed more people (as per v1)

In v3:
- split patch 2 to more patches (Greg)
- excluded C changes (Herbert, Greg)
- measured with kcbench, see below (Greg)

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/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 ++-
 9 files changed, 63 insertions(+), 39 deletions(-)
 create mode 100644 include/linux/container_of.h

-- 
2.33.0

[PATCH v4 1/7] kernel.h: Drop unneeded <linux/kernel.h> inclusion from other headers

[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.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>
-- 
2.33.0

[PATCH v4 2/7] kernel.h: Split out container_of() and typeof_member() macros

[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_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 +-----------------------------
 2 files changed, 38 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
-- 
2.33.0

[PATCH v4 3/7] kunit: Replace kernel.h with the necessary inclusions

[Andy Shevchenko]

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;
 
-- 
2.33.0

[PATCH v4 4/7] list.h: Replace kernel.h with the necessary inclusions

[Andy Shevchenko]

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.
-- 
2.33.0

[PATCH v4 5/7] llist: Replace kernel.h with the necessary inclusions

[Andy Shevchenko]

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;
-- 
2.33.0

[PATCH v4 6/7] plist: Replace kernel.h with the necessary inclusions

[Andy Shevchenko]

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;
-- 
2.33.0

[PATCH v4 7/7] media: entity: Replace kernel.h with the necessary inclusions

[Andy Shevchenko]

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 */
 
-- 
2.33.0

Re: [PATCH v4 2/7] kernel.h: Split out container_of() and typeof_member() macros

[Miguel Ojeda]

On Thu, Oct 7, 2021 at 5:44 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> +#define typeof_member(T, m)    typeof(((T*)0)->m)

Is the patch missing the removal from the other place?

Cheers,
Miguel

Re: [PATCH v4 2/7] kernel.h: Split out container_of() and typeof_member() macros

[Andy Shevchenko]

On Thu, Oct 07, 2021 at 05:54:32PM +0200, Miguel Ojeda wrote:
> On Thu, Oct 7, 2021 at 5:44 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > +#define typeof_member(T, m)    typeof(((T*)0)->m)
> 
> Is the patch missing the removal from the other place?

Possibly, but I leave it for now for builders to place with the series and
people having chance to comment.

Thanks for review!

Maybe Andrew can fix this when applying?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/7] list.h: Replace kernel.h with the necessary inclusions

[Jonathan Cameron]

On Thu,  7 Oct 2021 18:44:04 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> 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>

Is there a reason you didn't quite sort this into alphabetical order?

> -#include <linux/const.h>
> -#include <linux/kernel.h>
> +
> +#include <asm/barrier.h>
>  
>  /*
>   * Circular doubly linked list implementation.

Re: [PATCH v4 4/7] list.h: Replace kernel.h with the necessary inclusions

[Andy Shevchenko]

On Thu, Oct 07, 2021 at 05:16:35PM +0100, Jonathan Cameron wrote:
> On Thu,  7 Oct 2021 18:44:04 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> Is there a reason you didn't quite sort this into alphabetical order?

Glad you asked! Yes. Greg and possibly others will come and tell me that
I mustn't do two things in one change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/7] list.h: Replace kernel.h with the necessary inclusions

[Andy Shevchenko]

On Thu, Oct 07, 2021 at 07:21:18PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 07, 2021 at 05:16:35PM +0100, Jonathan Cameron wrote:
> > On Thu,  7 Oct 2021 18:44:04 +0300
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> ...
> 
> > Is there a reason you didn't quite sort this into alphabetical order?
> 
> Glad you asked! Yes. Greg and possibly others will come and tell me that
> I mustn't do two things in one change.

Actually I have to avoid moving const.h. I will change this in v5.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/7] kernel.h: Split out container_of() and typeof_member() macros

[Joe Perches]

On Thu, 2021-10-07 at 18:44 +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.

IMO: this new file is missing 2 #include directives.

> diff --git a/include/linux/container_of.h b/include/linux/container_of.h
[]
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

And trivially: I'd prefer GPL-2.0-only

> +#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()");	\

This is not a self-contained header as it requires
#include <linux/build_bug.h>
which should be at the top of this file.

> +	((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))); })

And this requires

#include <linux/err.h>

> +
> +#endif	/* _LINUX_CONTAINER_OF_H */

Re: [PATCH v4 2/7] kernel.h: Split out container_of() and typeof_member() macros

[Andy Shevchenko]

On Thu, Oct 07, 2021 at 09:27:38AM -0700, Joe Perches wrote:
> On Thu, 2021-10-07 at 18:44 +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.
> 
> IMO: this new file is missing 2 #include directives.

Thanks, Joe, I'll address this in v5.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 7/7] media: entity: Replace kernel.h with the necessary inclusions

[Sakari Ailus]

On Thu, Oct 07, 2021 at 06:44:07PM +0300, Andy Shevchenko wrote:
> 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>

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

Re: [PATCH v4 6/7] plist: Replace kernel.h with the necessary inclusions

[Joe Perches]

On Thu, 2021-10-07 at 18:44 +0300, Andy Shevchenko wrote:
> 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.
[]
> diff --git a/include/linux/plist.h b/include/linux/plist.h
[]
> @@ -73,8 +73,11 @@
[]
> +#include <asm/bug.h>

why asm/bug.h and not linux/bug.h ?

Re: [PATCH v4 6/7] plist: Replace kernel.h with the necessary inclusions

[Andy Shevchenko]

On Thu, Oct 07, 2021 at 10:12:56AM -0700, Joe Perches wrote:
> On Thu, 2021-10-07 at 18:44 +0300, Andy Shevchenko wrote:
> > 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.
> []
> > diff --git a/include/linux/plist.h b/include/linux/plist.h
> []
> > @@ -73,8 +73,11 @@
> []
> > +#include <asm/bug.h>
> 
> why asm/bug.h and not linux/bug.h ?

The direct inclusion is from that one. Why linux/bug? What are we going
to use from it?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 6/7] plist: Replace kernel.h with the necessary inclusions

[Joe Perches]

On Thu, 2021-10-07 at 20:19 +0300, Andy Shevchenko wrote:
> On Thu, Oct 07, 2021 at 10:12:56AM -0700, Joe Perches wrote:
> > On Thu, 2021-10-07 at 18:44 +0300, Andy Shevchenko wrote:
> > > 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.
> > []
> > > diff --git a/include/linux/plist.h b/include/linux/plist.h
> > []
> > > @@ -73,8 +73,11 @@
> > []
> > > +#include <asm/bug.h>
> > 
> > why asm/bug.h and not linux/bug.h ?
> 
> The direct inclusion is from that one. Why linux/bug?

A general guideline is to avoid asm includes.

Re: [PATCH v4 4/7] list.h: Replace kernel.h with the necessary inclusions

[Laurent Pinchart]

On Thu, Oct 07, 2021 at 05:16:35PM +0100, Jonathan Cameron wrote:
> On Thu,  7 Oct 2021 18:44:04 +0300 Andy Shevchenko wrote:
> 
> > 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>
> 
> Is there a reason you didn't quite sort this into alphabetical order?

On a side note, if someone with perle knowledge could add a checkpatch
warning for this, I think it would be very nice. I'm a bit tired of
asking for alphabetical order in reviews :-)

> > -#include <linux/const.h>
> > -#include <linux/kernel.h>
> > +
> > +#include <asm/barrier.h>
> >  
> >  /*
> >   * Circular doubly linked list implementation.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 6/7] plist: Replace kernel.h with the necessary inclusions

[Andy Shevchenko]

On Thu, Oct 07, 2021 at 10:26:05AM -0700, Joe Perches wrote:
> On Thu, 2021-10-07 at 20:19 +0300, Andy Shevchenko wrote:
> > On Thu, Oct 07, 2021 at 10:12:56AM -0700, Joe Perches wrote:
> > > On Thu, 2021-10-07 at 18:44 +0300, Andy Shevchenko wrote:
> > > > 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.
> > > []
> > > > diff --git a/include/linux/plist.h b/include/linux/plist.h
> > > []
> > > > @@ -73,8 +73,11 @@
> > > []
> > > > +#include <asm/bug.h>
> > > 
> > > why asm/bug.h and not linux/bug.h ?
> > 
> > The direct inclusion is from that one. Why linux/bug?
> 
> A general guideline is to avoid asm includes.

It's fine when it has any useful background. Doing cargo cult is not always
a good idea. I plead for common sense.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 6/7] plist: Replace kernel.h with the necessary inclusions

[Joe Perches]

On Thu, 2021-10-07 at 20:32 +0300, Andy Shevchenko wrote:
> On Thu, Oct 07, 2021 at 10:26:05AM -0700, Joe Perches wrote:
> > On Thu, 2021-10-07 at 20:19 +0300, Andy Shevchenko wrote:
> > > On Thu, Oct 07, 2021 at 10:12:56AM -0700, Joe Perches wrote:
> > > > On Thu, 2021-10-07 at 18:44 +0300, Andy Shevchenko wrote:
> > > > > 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.
> > > > []
> > > > > diff --git a/include/linux/plist.h b/include/linux/plist.h
> > > > []
> > > > > @@ -73,8 +73,11 @@
> > > > []
> > > > > +#include <asm/bug.h>
> > > > 
> > > > why asm/bug.h and not linux/bug.h ?
> > > 
> > > The direct inclusion is from that one. Why linux/bug?
> > 
> > A general guideline is to avoid asm includes.
> 
> It's fine when it has any useful background. Doing cargo cult is not always
> a good idea. I plead for common sense.

Common sense isn't particularly common.
Someone is going to run checkpatch on it and submit a patch one day.
Maybe add a comment for the less common.

cheers, Joe

Re: [PATCH v4 4/7] list.h: Replace kernel.h with the necessary inclusions

[Joe Perches]

On Thu, 2021-10-07 at 20:29 +0300, Laurent Pinchart wrote:
> On Thu, Oct 07, 2021 at 05:16:35PM +0100, Jonathan Cameron wrote:
> > On Thu,  7 Oct 2021 18:44:04 +0300 Andy Shevchenko wrote:
> > > 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.
[]
> > > diff --git 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>
> > 
> > Is there a reason you didn't quite sort this into alphabetical order?
> 
> On a side note, if someone with perle knowledge could add a checkpatch
> warning for this, I think it would be very nice. I'm a bit tired of
> asking for alphabetical order in reviews :-)

As are people that want reverse christmas tree.
Neither of which I will do as I think both are poor form at best.

If you want, this was a checkpatch reverse christmas tree attempt,
as that was more common to some.

https://lore.kernel.org/lkml/1478242438.1924.31.camel@perches.com/

Re: [PATCH v4 4/7] list.h: Replace kernel.h with the necessary inclusions

[Laurent Pinchart]

On Fri, Oct 08, 2021 at 05:59:18PM -0700, Joe Perches wrote:
> On Thu, 2021-10-07 at 20:29 +0300, Laurent Pinchart wrote:
> > On Thu, Oct 07, 2021 at 05:16:35PM +0100, Jonathan Cameron wrote:
> > > On Thu,  7 Oct 2021 18:44:04 +0300 Andy Shevchenko wrote:
> > > > 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.
> []
> > > > diff --git 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>
> > > 
> > > Is there a reason you didn't quite sort this into alphabetical order?
> > 
> > On a side note, if someone with perle knowledge could add a checkpatch
> > warning for this, I think it would be very nice. I'm a bit tired of
> > asking for alphabetical order in reviews :-)
> 
> As are people that want reverse christmas tree.
> Neither of which I will do as I think both are poor form at best.

Reverse xmas tree order is just a matter of style, while alphabetical
ordering of headers helps catching duplicate, including when merging
branches that both add the same header in different locations. I thus
think there's a technical value to it.

> If you want, this was a checkpatch reverse christmas tree attempt,
> as that was more common to some.
> 
> https://lore.kernel.org/lkml/1478242438.1924.31.camel@perches.com/

-- 
Regards,

Laurent Pinchart

RE: [PATCH v4 2/7] kernel.h: Split out container_of() and typeof_member() macros

[David Laight]

From: Joe Perches
> Sent: 07 October 2021 17:28
...
> IMO: this new file is missing 2 #include directives.
...
> This is not a self-contained header as it requires
> #include <linux/build_bug.h>
> which should be at the top of this file.
...
> And this requires
> 
> #include <linux/err.h>

And I bet the biggest problem is the time spent by the compiler
searching down the -I path for headers.

If you count system calls during a build I suspect that
failed opens of .h files dominate.

To see how much this really costs try running a build with
a (traditional) NFS mounted source tree - where every directory
name in a filename requires an NFS file handle lookup.

	David

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