[PATCH v2 0/3] lib/vsprintf: Rework header inclusions

[PATCH v2 0/3] lib/vsprintf: Rework header inclusions

[Andy Shevchenko]

Some patches that reduce the mess with the header inclusions related to
vsprintf.c module. Each patch has its own description, and has no
dependencies to each other, except the collisions over modifications
of the same places. Hence the series.

Changelog v2:
- covered test_printf.c in patches 1 & 2
- do not remove likely implict inclusions (Rasmus)
- declare no_hash_pointers in sprintf.h (Marco, Steven, Rasmus)

Andy Shevchenko (3):
  lib/vsprintf: Sort headers alphabetically
  lib/vsprintf: Split out sprintf() and friends
  lib/vsprintf: Declare no_hash_pointers in sprintf.h

 include/linux/kernel.h  | 30 +-----------------------------
 include/linux/sprintf.h | 27 +++++++++++++++++++++++++++
 lib/test_printf.c       | 20 ++++++++------------
 lib/vsprintf.c          | 39 +++++++++++++++++++++------------------
 mm/kfence/report.c      |  3 +--
 5 files changed, 58 insertions(+), 61 deletions(-)
 create mode 100644 include/linux/sprintf.h

-- 
2.40.0.1.gaa8946217a0b

[PATCH v2 1/3] lib/vsprintf: Sort headers alphabetically

[Andy Shevchenko]

Sorting headers alphabetically helps locating duplicates, and
make it easier to figure out where to insert new headers.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/test_printf.c | 17 +++++++----------
 lib/vsprintf.c    | 38 ++++++++++++++++++++------------------
 2 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7677ebccf3c3..2ab09a0dc841 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -5,24 +5,21 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/bitmap.h>
+#include <linux/dcache.h>
+#include <linux/gfp.h>
+#include <linux/in.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/printk.h>
+#include <linux/property.h>
 #include <linux/random.h>
 #include <linux/rtc.h>
 #include <linux/slab.h>
-#include <linux/string.h>
-
-#include <linux/bitmap.h>
-#include <linux/dcache.h>
 #include <linux/socket.h>
-#include <linux/in.h>
-
-#include <linux/gfp.h>
-#include <linux/mm.h>
-
-#include <linux/property.h>
+#include <linux/string.h>
 
 #include "../tools/testing/selftests/kselftest_module.h"
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 40f560959b16..b17e0744a7bc 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -17,42 +17,44 @@
  * - scnprintf and vscnprintf
  */
 
-#include <linux/stdarg.h>
 #include <linux/build_bug.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
-#include <linux/errname.h>
-#include <linux/module.h>	/* for KSYM_SYMBOL_LEN */
-#include <linux/types.h>
-#include <linux/string.h>
+#include <linux/compiler.h>
+#include <linux/cred.h>
 #include <linux/ctype.h>
-#include <linux/kernel.h>
+#include <linux/dcache.h>
+#include <linux/errname.h>
+#include <linux/ioport.h>
 #include <linux/kallsyms.h>
+#include <linux/kernel.h>
 #include <linux/math64.h>
-#include <linux/uaccess.h>
-#include <linux/ioport.h>
-#include <linux/dcache.h>
-#include <linux/cred.h>
+#include <linux/module.h>	/* for KSYM_SYMBOL_LEN */
+#include <linux/notifier.h>
+#include <linux/of.h>
+#include <linux/property.h>
 #include <linux/rtc.h>
+#include <linux/siphash.h>
+#include <linux/stdarg.h>
+#include <linux/string.h>
+#include <linux/string_helpers.h>
 #include <linux/time.h>
+#include <linux/types.h>
+#include <linux/uaccess.h>
 #include <linux/uuid.h>
-#include <linux/of.h>
-#include <net/addrconf.h>
-#include <linux/siphash.h>
-#include <linux/compiler.h>
-#include <linux/property.h>
-#include <linux/notifier.h>
+
 #ifdef CONFIG_BLOCK
 #include <linux/blkdev.h>
 #endif
 
+#include <net/addrconf.h>
+
 #include "../mm/internal.h"	/* For the trace_print_flags arrays */
 
-#include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/byteorder.h>	/* cpu_to_le16 */
+#include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/unaligned.h>
 
-#include <linux/string_helpers.h>
 #include "kstrtox.h"
 
 /* Disable pointer hashing if requested */
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 1/3] lib/vsprintf: Sort headers alphabetically

[Petr Mladek]

On Sat 2023-08-05 20:50:25, Andy Shevchenko wrote:
> Sorting headers alphabetically helps locating duplicates, and
> make it easier to figure out where to insert new headers.

I agree that includes become a mess after some time. But I am
not persuaded that sorting them alphabetically in random source
files help anything.

Is this part of some grand plan for the entire kernel, please?
Is this outcome from some particular discussion?
Will this become a well know rule checked by checkpatch.pl?

I am personally not going to reject patches because of wrongly
sorted headers unless there is some real plan behind it.

I agree that it might look better. An inverse Christmas' tree
also looks better. But it does not mean that it makes the life
easier. The important things are still hidden in the details
(every single line).

From my POV, this patch would just create a mess in the git
history and complicate backporting.

I am sorry but I will not accept this patch unless there
is a wide consensus that this makes sense.

Best Regards,
Petr

Re: [PATCH v2 1/3] lib/vsprintf: Sort headers alphabetically

[Sergey Senozhatsky]

On (23/08/07 16:31), Petr Mladek wrote:
> 
> I am sorry but I will not accept this patch unless there
> is a wide consensus that this makes sense.

I completely agree with Petr.

I found it a little bit hard to be enthusiastic about
this patch in particular and _probably_ about this series
in general, sorry Andy.

Re: [PATCH v2 1/3] lib/vsprintf: Sort headers alphabetically

[Andy Shevchenko]

On Mon, Aug 07, 2023 at 11:53:02PM +0900, Sergey Senozhatsky wrote:
> On (23/08/07 16:31), Petr Mladek wrote:
> > 
> > I am sorry but I will not accept this patch unless there
> > is a wide consensus that this makes sense.
> 
> I completely agree with Petr.
> 
> I found it a little bit hard to be enthusiastic about
> this patch in particular and _probably_ about this series
> in general, sorry Andy.

What to do with _headers_ that include kernel.h for no reason other than
sprintf.h (as an example)? Your suggestion, please?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] lib/vsprintf: Sort headers alphabetically

[Andy Shevchenko]

On Mon, Aug 07, 2023 at 04:31:37PM +0200, Petr Mladek wrote:
> On Sat 2023-08-05 20:50:25, Andy Shevchenko wrote:
> > Sorting headers alphabetically helps locating duplicates, and
> > make it easier to figure out where to insert new headers.
> 
> I agree that includes become a mess after some time. But I am
> not persuaded that sorting them alphabetically in random source
> files help anything.
> 
> Is this part of some grand plan for the entire kernel, please?
> Is this outcome from some particular discussion?
> Will this become a well know rule checked by checkpatch.pl?
> 
> I am personally not going to reject patches because of wrongly
> sorted headers unless there is some real plan behind it.
> 
> I agree that it might look better. An inverse Christmas' tree
> also looks better. But it does not mean that it makes the life
> easier.

It does from my point of view as maintainability is increased.

> The important things are still hidden in the details
> (every single line).
> 
> From my POV, this patch would just create a mess in the git
> history and complicate backporting.
> 
> I am sorry but I will not accept this patch unless there
> is a wide consensus that this makes sense.

Your choice, of course, But I see in practice dup headers being
added, or some unrelated ones left untouched because header list
mess, and in those cases sorting can help (a bit) in my opinion.

TL;DR: I was tolerating unsorted mess (for really long header
inclusion block) up to the point when I realized how it helps
people to maintain the code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/3] lib/vsprintf: Sort headers alphabetically

[Rasmus Villemoes]

On 07/08/2023 16.58, Andy Shevchenko wrote:
> On Mon, Aug 07, 2023 at 04:31:37PM +0200, Petr Mladek wrote:
>> On Sat 2023-08-05 20:50:25, Andy Shevchenko wrote:
>>> Sorting headers alphabetically helps locating duplicates, and
>>> make it easier to figure out where to insert new headers.
>>
>> I agree that includes become a mess after some time. But I am
>> not persuaded that sorting them alphabetically in random source
>> files help anything.
>>
>> Is this part of some grand plan for the entire kernel, please?
>> Is this outcome from some particular discussion?
>> Will this become a well know rule checked by checkpatch.pl?
>>
>> I am personally not going to reject patches because of wrongly
>> sorted headers unless there is some real plan behind it.
>>
>> I agree that it might look better. An inverse Christmas' tree
>> also looks better. But it does not mean that it makes the life
>> easier.
> 
> It does from my point of view as maintainability is increased.
> 
>> The important things are still hidden in the details
>> (every single line).
>>
>> From my POV, this patch would just create a mess in the git
>> history and complicate backporting.
>>
>> I am sorry but I will not accept this patch unless there
>> is a wide consensus that this makes sense.
> 
> Your choice, of course, But I see in practice dup headers being
> added, or some unrelated ones left untouched because header list
> mess, and in those cases sorting can help (a bit) in my opinion.

I agree with Andy on this one. There doesn't need to be some grand
master plan to apply this to the entire kernel, but doing it to
individual files bit by bit does increase the maintainability. And I
really don't buy the backporting argument. Sure, backporting some patch
across the release that does the sorting is harder - but then,
backporting the sorting patch itself is entirely trivial (maybe not the
textual part, but redoing the semantics of it is). _However_,
backporting a patch from release z to release y, both of which being
later than the release x that did the sorting, is going to be _easier_.
It also reduces merge conflicts - that's also why lots of Makefiles are
kept sorted.

It's of course entirely unrelated to moving the declarations of the
provided functions to a separate header file, but IMO both are worth doing.

Rasmus

Re: [PATCH v2 1/3] lib/vsprintf: Sort headers alphabetically

[Petr Mladek]

On Mon 2023-08-07 21:47:17, Rasmus Villemoes wrote:
> On 07/08/2023 16.58, Andy Shevchenko wrote:
> > On Mon, Aug 07, 2023 at 04:31:37PM +0200, Petr Mladek wrote:
> >> On Sat 2023-08-05 20:50:25, Andy Shevchenko wrote:
> >>> Sorting headers alphabetically helps locating duplicates, and
> >>> make it easier to figure out where to insert new headers.
> >>
> >> I agree that includes become a mess after some time. But I am
> >> not persuaded that sorting them alphabetically in random source
> >> files help anything.
> >>
> >> Is this part of some grand plan for the entire kernel, please?
> >> Is this outcome from some particular discussion?
> >> Will this become a well know rule checked by checkpatch.pl?
> >>
> >> I am personally not going to reject patches because of wrongly
> >> sorted headers unless there is some real plan behind it.
> >>
> >> I agree that it might look better. An inverse Christmas' tree
> >> also looks better. But it does not mean that it makes the life
> >> easier.
> > 
> > It does from my point of view as maintainability is increased.
> > 
> >> The important things are still hidden in the details
> >> (every single line).
> >>
> >> From my POV, this patch would just create a mess in the git
> >> history and complicate backporting.
> >>
> >> I am sorry but I will not accept this patch unless there
> >> is a wide consensus that this makes sense.
> > 
> > Your choice, of course, But I see in practice dup headers being
> > added, or some unrelated ones left untouched because header list
> > mess, and in those cases sorting can help (a bit) in my opinion.
> 
> I agree with Andy on this one. There doesn't need to be some grand
> master plan to apply this to the entire kernel, but doing it to
> individual files bit by bit does increase the maintainability. And I
> really don't buy the backporting argument. Sure, backporting some patch
> across the release that does the sorting is harder - but then,
> backporting the sorting patch itself is entirely trivial (maybe not the
> textual part, but redoing the semantics of it is). _However_,
> backporting a patch from release z to release y, both of which being
> later than the release x that did the sorting, is going to be _easier_.
> It also reduces merge conflicts - that's also why lots of Makefiles are
> kept sorted.

I am afraid that we will not find a consensus here. I agree that
the sorting has some advantage.

But I would still like to get some wider agreement on this move
from other subsystem. It is a good candidate for a mass change
which would be part of some plan.

I want to avoid reshuffling this more times according to personal
preferences. And I do not want to add this cosmetic subsystem
specific requirement.

Best Regards,
Petr

[PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Andy Shevchenko]

kernel.h is being used as a dump for all kinds of stuff for a long time.
sprintf() and friends are used in many drivers without need of the full
kernel.h dependency train with it.

Here is the attempt on cleaning it up by splitting out sprintf() and
friends.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/kernel.h  | 30 +-----------------------------
 include/linux/sprintf.h | 25 +++++++++++++++++++++++++
 lib/test_printf.c       |  1 +
 lib/vsprintf.c          |  1 +
 4 files changed, 28 insertions(+), 29 deletions(-)
 create mode 100644 include/linux/sprintf.h

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index b9e76f717a7e..cee8fe87e9f4 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -29,6 +29,7 @@
 #include <linux/panic.h>
 #include <linux/printk.h>
 #include <linux/build_bug.h>
+#include <linux/sprintf.h>
 #include <linux/static_call_types.h>
 #include <linux/instruction_pointer.h>
 #include <asm/byteorder.h>
@@ -203,35 +204,6 @@ static inline void might_fault(void) { }
 
 void do_exit(long error_code) __noreturn;
 
-extern int num_to_str(char *buf, int size,
-		      unsigned long long num, unsigned int width);
-
-/* lib/printf utilities */
-
-extern __printf(2, 3) int sprintf(char *buf, const char * fmt, ...);
-extern __printf(2, 0) int vsprintf(char *buf, const char *, va_list);
-extern __printf(3, 4)
-int snprintf(char *buf, size_t size, const char *fmt, ...);
-extern __printf(3, 0)
-int vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
-extern __printf(3, 4)
-int scnprintf(char *buf, size_t size, const char *fmt, ...);
-extern __printf(3, 0)
-int vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
-extern __printf(2, 3) __malloc
-char *kasprintf(gfp_t gfp, const char *fmt, ...);
-extern __printf(2, 0) __malloc
-char *kvasprintf(gfp_t gfp, const char *fmt, va_list args);
-extern __printf(2, 0)
-const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
-
-extern __scanf(2, 3)
-int sscanf(const char *, const char *, ...);
-extern __scanf(2, 0)
-int vsscanf(const char *, const char *, va_list);
-
-extern int no_hash_pointers_enable(char *str);
-
 extern int get_option(char **str, int *pint);
 extern char *get_options(const char *str, int nints, int *ints);
 extern unsigned long long memparse(const char *ptr, char **retptr);
diff --git a/include/linux/sprintf.h b/include/linux/sprintf.h
new file mode 100644
index 000000000000..9ca23bcf9f42
--- /dev/null
+++ b/include/linux/sprintf.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_KERNEL_SPRINTF_H_
+#define _LINUX_KERNEL_SPRINTF_H_
+
+#include <linux/compiler_attributes.h>
+#include <linux/types.h>
+
+int num_to_str(char *buf, int size, unsigned long long num, unsigned int width);
+
+__printf(2, 3) int sprintf(char *buf, const char * fmt, ...);
+__printf(2, 0) int vsprintf(char *buf, const char *, va_list);
+__printf(3, 4) int snprintf(char *buf, size_t size, const char *fmt, ...);
+__printf(3, 0) int vsnprintf(char *buf, size_t size, const char *fmt, va_list args);
+__printf(3, 4) int scnprintf(char *buf, size_t size, const char *fmt, ...);
+__printf(3, 0) int vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
+__printf(2, 3) __malloc char *kasprintf(gfp_t gfp, const char *fmt, ...);
+__printf(2, 0) __malloc char *kvasprintf(gfp_t gfp, const char *fmt, va_list args);
+__printf(2, 0) const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list args);
+
+__scanf(2, 3) int sscanf(const char *, const char *, ...);
+__scanf(2, 0) int vsscanf(const char *, const char *, va_list);
+
+int no_hash_pointers_enable(char *str);
+
+#endif	/* _LINUX_KERNEL_SPRINTF_H */
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 2ab09a0dc841..5adca19d34e2 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -19,6 +19,7 @@
 #include <linux/rtc.h>
 #include <linux/slab.h>
 #include <linux/socket.h>
+#include <linux/sprintf.h>
 #include <linux/string.h>
 
 #include "../tools/testing/selftests/kselftest_module.h"
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b17e0744a7bc..c89719586d0c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -35,6 +35,7 @@
 #include <linux/property.h>
 #include <linux/rtc.h>
 #include <linux/siphash.h>
+#include <linux/sprintf.h>
 #include <linux/stdarg.h>
 #include <linux/string.h>
 #include <linux/string_helpers.h>
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Andrew Morton]

On Sat,  5 Aug 2023 20:50:26 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> kernel.h is being used as a dump for all kinds of stuff for a long time.
> sprintf() and friends are used in many drivers without need of the full
> kernel.h dependency train with it.

There seems little point in this unless someone signs up to convert
lots of code to include sprintf.h instead of kernel.h?

And such conversions will presumably cause all sorts of nasties
which require additional work?

So... what's the plan here?

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Andy Shevchenko]

On Sat, Aug 05, 2023 at 11:43:04AM -0700, Andrew Morton wrote:
> On Sat,  5 Aug 2023 20:50:26 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > kernel.h is being used as a dump for all kinds of stuff for a long time.
> > sprintf() and friends are used in many drivers without need of the full
> > kernel.h dependency train with it.
> 
> There seems little point in this unless someone signs up to convert
> lots of code to include sprintf.h instead of kernel.h?
> 
> And such conversions will presumably cause all sorts of nasties
> which require additional work?
> 
> So... what's the plan here?

My main plan is to clean _headers_ from kernel.h.
The rest of the code may do that gradually.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Andy Shevchenko]

On Sat, Aug 05, 2023 at 11:43:04AM -0700, Andrew Morton wrote:
> On Sat,  5 Aug 2023 20:50:26 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > kernel.h is being used as a dump for all kinds of stuff for a long time.
> > sprintf() and friends are used in many drivers without need of the full
> > kernel.h dependency train with it.
> 
> There seems little point in this unless someone signs up to convert
> lots of code to include sprintf.h instead of kernel.h?

You can say it to any cleanup work that starts from the baby steps.

> And such conversions will presumably cause all sorts of nasties
> which require additional work?
> 
> So... what's the plan here?

My main goal is to get rid from kernel.h in the _headers_ first.
The secondary goal as discussed with Jonathan to have IIO subsystem
be cleaned up from kernel.h (meaning C files as well) at some point.

FWIW, I have started kernel.h cleanup due to impossibility to
make bitmap_*alloc() being static inline.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Petr Mladek]

On Sat 2023-08-05 20:50:26, Andy Shevchenko wrote:
> kernel.h is being used as a dump for all kinds of stuff for a long time.
> sprintf() and friends are used in many drivers without need of the full
> kernel.h dependency train with it.
> 
> Here is the attempt on cleaning it up by splitting out sprintf() and
> friends.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/kernel.h  | 30 +-----------------------------
>  include/linux/sprintf.h | 25 +++++++++++++++++++++++++
>  lib/test_printf.c       |  1 +
>  lib/vsprintf.c          |  1 +
>  4 files changed, 28 insertions(+), 29 deletions(-)
>  create mode 100644 include/linux/sprintf.h

I agree that kernel.h is not the right place. But are there any
numbers how much separate sprintf.h might safe?

Maybe, we should not reinvent the wheel and get inspired by
userspace.

sprintf() and friends are basic functions which most people know
from userspace. And it is pretty handy that the kernel variants
are are mostly compatible as well.

IMHO, it might be handful when they are also included similar way
as in userspace. From my POV printk.h is like stdio.h. And we already
have include/linux/stdarg.h where the v*print*() function might
fit nicely.

How does this sound, please?

Best Regards,
Petr

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Andy Shevchenko]

On Mon, Aug 07, 2023 at 05:03:19PM +0200, Petr Mladek wrote:
> On Sat 2023-08-05 20:50:26, Andy Shevchenko wrote:
> > kernel.h is being used as a dump for all kinds of stuff for a long time.
> > sprintf() and friends are used in many drivers without need of the full
> > kernel.h dependency train with it.
> > 
> > Here is the attempt on cleaning it up by splitting out sprintf() and
> > friends.

...

> I agree that kernel.h is not the right place. But are there any
> numbers how much separate sprintf.h might safe?
> Maybe, we should not reinvent the wheel and get inspired by
> userspace.
> 
> sprintf() and friends are basic functions which most people know
> from userspace. And it is pretty handy that the kernel variants
> are are mostly compatible as well.
> 
> IMHO, it might be handful when they are also included similar way
> as in userspace. From my POV printk.h is like stdio.h. And we already
> have include/linux/stdarg.h where the v*print*() function might
> fit nicely.
> 
> How does this sound, please?

Not every user (especially _header_) wants to have printk.h included just for
sprintf.h that may have nothing to do with real output. So, same reasoning
from me as keeping that in kernel.h, i.e. printk.h no better.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Andy Shevchenko]

On Mon, Aug 07, 2023 at 06:09:54PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 07, 2023 at 05:03:19PM +0200, Petr Mladek wrote:
> > On Sat 2023-08-05 20:50:26, Andy Shevchenko wrote:

...

> > I agree that kernel.h is not the right place. But are there any
> > numbers how much separate sprintf.h might safe?
> > Maybe, we should not reinvent the wheel and get inspired by
> > userspace.
> > 
> > sprintf() and friends are basic functions which most people know
> > from userspace. And it is pretty handy that the kernel variants
> > are are mostly compatible as well.
> > 
> > IMHO, it might be handful when they are also included similar way
> > as in userspace. From my POV printk.h is like stdio.h. And we already
> > have include/linux/stdarg.h where the v*print*() function might
> > fit nicely.
> > 
> > How does this sound, please?
> 
> Not every user (especially _header_) wants to have printk.h included just for
> sprintf.h that may have nothing to do with real output. So, same reasoning
> from me as keeping that in kernel.h, i.e. printk.h no better.

(haven't check these, just to show how many _headers_ uses sprintf() call)

$ git grep -lw s.*printf -- include/linux/
include/linux/acpi.h
include/linux/audit.h
include/linux/btf.h
include/linux/dev_printk.h
include/linux/device-mapper.h
include/linux/efi.h
include/linux/fortify-string.h
include/linux/fs.h
include/linux/gameport.h
include/linux/kdb.h
include/linux/kdev_t.h
include/linux/kernel.h
include/linux/mmiotrace.h
include/linux/netlink.h
include/linux/pci-p2pdma.h
include/linux/perf_event.h
include/linux/printk.h
include/linux/seq_buf.h
include/linux/seq_file.h
include/linux/shrinker.h
include/linux/string.h
include/linux/sunrpc/svc_xprt.h
include/linux/tnum.h
include/linux/trace_seq.h
include/linux/usb.h
include/linux/usb/gadget_configfs.h

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Andy Shevchenko]

On Mon, Aug 07, 2023 at 06:11:24PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 07, 2023 at 06:09:54PM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 07, 2023 at 05:03:19PM +0200, Petr Mladek wrote:
> > > On Sat 2023-08-05 20:50:26, Andy Shevchenko wrote:

...

> > > How does this sound, please?
> > 
> > Not every user (especially _header_) wants to have printk.h included just for
> > sprintf.h that may have nothing to do with real output. So, same reasoning
> > from me as keeping that in kernel.h, i.e. printk.h no better.
> 
> (haven't check these, just to show how many _headers_ uses sprintf() call)
> 
> $ git grep -lw s.*printf -- include/linux/
> include/linux/acpi.h
> include/linux/audit.h
> include/linux/btf.h
> include/linux/dev_printk.h
> include/linux/device-mapper.h
> include/linux/efi.h
> include/linux/fortify-string.h
> include/linux/fs.h
> include/linux/gameport.h
> include/linux/kdb.h
> include/linux/kdev_t.h
> include/linux/kernel.h
> include/linux/mmiotrace.h
> include/linux/netlink.h
> include/linux/pci-p2pdma.h
> include/linux/perf_event.h
> include/linux/printk.h
> include/linux/seq_buf.h
> include/linux/seq_file.h
> include/linux/shrinker.h
> include/linux/string.h
> include/linux/sunrpc/svc_xprt.h
> include/linux/tnum.h
> include/linux/trace_seq.h
> include/linux/usb.h
> include/linux/usb/gadget_configfs.h

Okay, revised as my regexp was too lazy

$ git grep -lw s[^[:space:]_]*printf -- include/linux/
include/linux/btf.h
include/linux/device-mapper.h
include/linux/efi.h
include/linux/fortify-string.h
include/linux/kdev_t.h
include/linux/kernel.h
include/linux/netlink.h
include/linux/pci-p2pdma.h
include/linux/perf_event.h
include/linux/sunrpc/svc_xprt.h
include/linux/tnum.h
include/linux/usb.h
include/linux/usb/gadget_configfs.h


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Petr Mladek]

On Mon 2023-08-07 18:13:57, Andy Shevchenko wrote:
> On Mon, Aug 07, 2023 at 06:11:24PM +0300, Andy Shevchenko wrote:
> > On Mon, Aug 07, 2023 at 06:09:54PM +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 07, 2023 at 05:03:19PM +0200, Petr Mladek wrote:
> > > > On Sat 2023-08-05 20:50:26, Andy Shevchenko wrote:
> 
> ...
> 
> > > > How does this sound, please?
> > > 
> > > Not every user (especially _header_) wants to have printk.h included just for
> > > sprintf.h that may have nothing to do with real output. So, same reasoning
> > > from me as keeping that in kernel.h, i.e. printk.h no better.
> > 
> > (haven't check these, just to show how many _headers_ uses sprintf() call)
> > 
> > $ git grep -lw s.*printf -- include/linux/
> > include/linux/acpi.h
> > include/linux/audit.h
> > include/linux/btf.h
> > include/linux/dev_printk.h
> > include/linux/device-mapper.h
> > include/linux/efi.h
> > include/linux/fortify-string.h
> > include/linux/fs.h
> > include/linux/gameport.h
> > include/linux/kdb.h
> > include/linux/kdev_t.h
> > include/linux/kernel.h
> > include/linux/mmiotrace.h
> > include/linux/netlink.h
> > include/linux/pci-p2pdma.h
> > include/linux/perf_event.h
> > include/linux/printk.h
> > include/linux/seq_buf.h
> > include/linux/seq_file.h
> > include/linux/shrinker.h
> > include/linux/string.h
> > include/linux/sunrpc/svc_xprt.h
> > include/linux/tnum.h
> > include/linux/trace_seq.h
> > include/linux/usb.h
> > include/linux/usb/gadget_configfs.h
> 
> Okay, revised as my regexp was too lazy
> 
> $ git grep -lw s[^[:space:]_]*printf -- include/linux/
> include/linux/btf.h
> include/linux/device-mapper.h
> include/linux/efi.h
> include/linux/fortify-string.h
> include/linux/kdev_t.h
> include/linux/kernel.h
> include/linux/netlink.h
> include/linux/pci-p2pdma.h
> include/linux/perf_event.h
> include/linux/sunrpc/svc_xprt.h
> include/linux/tnum.h
> include/linux/usb.h
> include/linux/usb/gadget_configfs.h

This is only a tiny part of the picture.

$> git grep sc*n*printf | cut -d : -f1 | uniq | grep "\.c$" | wc -l
5254
$> find . -name  "*.c" | wc -l
32319

It means that the vsprintf() family is used in 1/6 of all kernel
source files. They would need to include one extra header.

If you split headers into so many small pieces then all
source files will start with 3 screens of includes. I do not see
how this helps with maintainability.

Best Regards,
Petr

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Andy Shevchenko]

On Tue, Aug 08, 2023 at 08:41:49AM +0200, Petr Mladek wrote:
> On Mon 2023-08-07 18:13:57, Andy Shevchenko wrote:
> > On Mon, Aug 07, 2023 at 06:11:24PM +0300, Andy Shevchenko wrote:
> > > On Mon, Aug 07, 2023 at 06:09:54PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Aug 07, 2023 at 05:03:19PM +0200, Petr Mladek wrote:
> > > > > On Sat 2023-08-05 20:50:26, Andy Shevchenko wrote:

...

> > > > > How does this sound, please?
> > > > 
> > > > Not every user (especially _header_) wants to have printk.h included just for
> > > > sprintf.h that may have nothing to do with real output. So, same reasoning
> > > > from me as keeping that in kernel.h, i.e. printk.h no better.
> > > 
> > > (haven't check these, just to show how many _headers_ uses sprintf() call)
> > > 
> > > $ git grep -lw s.*printf -- include/linux/
> > > include/linux/acpi.h
> > > include/linux/audit.h
> > > include/linux/btf.h
> > > include/linux/dev_printk.h
> > > include/linux/device-mapper.h
> > > include/linux/efi.h
> > > include/linux/fortify-string.h
> > > include/linux/fs.h
> > > include/linux/gameport.h
> > > include/linux/kdb.h
> > > include/linux/kdev_t.h
> > > include/linux/kernel.h
> > > include/linux/mmiotrace.h
> > > include/linux/netlink.h
> > > include/linux/pci-p2pdma.h
> > > include/linux/perf_event.h
> > > include/linux/printk.h
> > > include/linux/seq_buf.h
> > > include/linux/seq_file.h
> > > include/linux/shrinker.h
> > > include/linux/string.h
> > > include/linux/sunrpc/svc_xprt.h
> > > include/linux/tnum.h
> > > include/linux/trace_seq.h
> > > include/linux/usb.h
> > > include/linux/usb/gadget_configfs.h
> > 
> > Okay, revised as my regexp was too lazy
> > 
> > $ git grep -lw s[^[:space:]_]*printf -- include/linux/
> > include/linux/btf.h
> > include/linux/device-mapper.h
> > include/linux/efi.h
> > include/linux/fortify-string.h
> > include/linux/kdev_t.h
> > include/linux/kernel.h
> > include/linux/netlink.h
> > include/linux/pci-p2pdma.h
> > include/linux/perf_event.h
> > include/linux/sunrpc/svc_xprt.h
> > include/linux/tnum.h
> > include/linux/usb.h
> > include/linux/usb/gadget_configfs.h
> 
> This is only a tiny part of the picture.
> 
> $> git grep sc*n*printf | cut -d : -f1 | uniq | grep "\.c$" | wc -l
> 5254
> $> find . -name  "*.c" | wc -l
> 32319
> 
> It means that the vsprintf() family is used in 1/6 of all kernel
> source files. They would need to include one extra header.

No, not only one. more, but the outcome of this is not using what is not used
and unwinding the header dependency hell.

But hey, I am not talking about C files right now, it's secondary, however
in IIO we want to get rid of kernel.h in the C files as well.

Also, please, go through all of them and tell, how many of them are using
stuff from kernel.h besides sprintf.h and ARRAY_SIZE() (which I plan
for a long time to split from kernel.h)?

> If you split headers into so many small pieces then all
> source files will start with 3 screens of includes. I do not see
> how this helps with maintainability.

It should be a compromise. But including kernel.h mess into a file
(**especially** into header) for let's say a single sprintf() call
or use ARRAY_SIZE() macro is a bad idea. _This_ is not maintainable
code and developers definitely haven't put their brains to what they
are doing with the header inclusion block in their code.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Petr Mladek]

On Tue 2023-08-08 15:47:59, Andy Shevchenko wrote:
> On Tue, Aug 08, 2023 at 08:41:49AM +0200, Petr Mladek wrote:
> > On Mon 2023-08-07 18:13:57, Andy Shevchenko wrote:
> > > On Mon, Aug 07, 2023 at 06:11:24PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Aug 07, 2023 at 06:09:54PM +0300, Andy Shevchenko wrote:
> > > > > On Mon, Aug 07, 2023 at 05:03:19PM +0200, Petr Mladek wrote:
> > > > > > On Sat 2023-08-05 20:50:26, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > > How does this sound, please?
> > > > > 
> > > > > Not every user (especially _header_) wants to have printk.h included just for
> > > > > sprintf.h that may have nothing to do with real output. So, same reasoning
> > > > > from me as keeping that in kernel.h, i.e. printk.h no better.
> > > > 
> > > > (haven't check these, just to show how many _headers_ uses sprintf() call)
> > > > 
> > > > $ git grep -lw s.*printf -- include/linux/
> > > > include/linux/acpi.h
> > > > include/linux/audit.h
> > > > include/linux/btf.h
> > > > include/linux/dev_printk.h
> > > > include/linux/device-mapper.h
> > > > include/linux/efi.h
> > > > include/linux/fortify-string.h
> > > > include/linux/fs.h
> > > > include/linux/gameport.h
> > > > include/linux/kdb.h
> > > > include/linux/kdev_t.h
> > > > include/linux/kernel.h
> > > > include/linux/mmiotrace.h
> > > > include/linux/netlink.h
> > > > include/linux/pci-p2pdma.h
> > > > include/linux/perf_event.h
> > > > include/linux/printk.h
> > > > include/linux/seq_buf.h
> > > > include/linux/seq_file.h
> > > > include/linux/shrinker.h
> > > > include/linux/string.h
> > > > include/linux/sunrpc/svc_xprt.h
> > > > include/linux/tnum.h
> > > > include/linux/trace_seq.h
> > > > include/linux/usb.h
> > > > include/linux/usb/gadget_configfs.h
> > > 
> > > Okay, revised as my regexp was too lazy
> > > 
> > > $ git grep -lw s[^[:space:]_]*printf -- include/linux/
> > > include/linux/btf.h
> > > include/linux/device-mapper.h
> > > include/linux/efi.h
> > > include/linux/fortify-string.h
> > > include/linux/kdev_t.h
> > > include/linux/kernel.h
> > > include/linux/netlink.h
> > > include/linux/pci-p2pdma.h
> > > include/linux/perf_event.h
> > > include/linux/sunrpc/svc_xprt.h
> > > include/linux/tnum.h
> > > include/linux/usb.h
> > > include/linux/usb/gadget_configfs.h
> > 
> > This is only a tiny part of the picture.
> > 
> > $> git grep sc*n*printf | cut -d : -f1 | uniq | grep "\.c$" | wc -l
> > 5254
> > $> find . -name  "*.c" | wc -l
> > 32319
> > 
> > It means that the vsprintf() family is used in 1/6 of all kernel
> > source files. They would need to include one extra header.
> 
> No, not only one. more, but the outcome of this is not using what is not used
> and unwinding the header dependency hell.
> 
> But hey, I am not talking about C files right now, it's secondary, however
> in IIO we want to get rid of kernel.h in the C files as well.

This sounds scary. Headers and C files are closely related. IMHO, it
does not makes sense to split header files without looking how
the functions are used.

Everyone agrees that kernel.h should be removed. But there are always
more possibilities where to move the definitions. For this, the use
in C files must be considered. Otherwise, it is just a try&hope approach.

> Also, please, go through all of them and tell, how many of them are using
> stuff from kernel.h besides sprintf.h and ARRAY_SIZE() (which I plan
> for a long time to split from kernel.h)?

I am all for removing vsprintf declarations from linux.h.

I provided the above numbers to support the idea of moving them
into printk.h.

The numbers show that the vsprintf function famility is used
quite frequently. IMHO, creating an extra tiny include file
will create more harm then good. By the harm I mean:

    + churn when updating 1/6 of source files

    + prolonging the list of #include lines in .c file. It will
      not help with maintainability which was one of the motivation
      in this patchset.

    + an extra work for people using vsprintf function family in
      new .c files. People are used to get them for free,
      together with printk().

Best Regards,
Petr

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Rasmus Villemoes]

On 10/08/2023 10.15, Petr Mladek wrote:

> Everyone agrees that kernel.h should be removed. But there are always
> more possibilities where to move the definitions. For this, the use
> in C files must be considered. Otherwise, it is just a try&hope approach.
> 
>> Also, please, go through all of them and tell, how many of them are using
>> stuff from kernel.h besides sprintf.h and ARRAY_SIZE() (which I plan
>> for a long time to split from kernel.h)?
> 
> I am all for removing vsprintf declarations from linux.h.
> 
> I provided the above numbers to support the idea of moving them
> into printk.h.
> 
> The numbers show that the vsprintf function famility is used
> quite frequently. IMHO, creating an extra tiny include file
> will create more harm then good. By the harm I mean:
> 
>     + churn when updating 1/6 of source files

Well, we probably shouldn't do 5000 single-line patches to add that
sprintf.h include, and another 10000 to add an array-macros.h include
(just as an example). Some tooling and reasonable batching would
probably be required. Churn it will be, but how many thousands of
patches were done to make i2c drivers' probe methods lose a parameter
(first converting them all to .probe_new, then another round to again
assign to .probe when that prototype was changed). That's just the cost
of any tree-wide change in a tree our size.

>     + prolonging the list of #include lines in .c file. It will
>       not help with maintainability which was one of the motivation
>       in this patchset.

We really have to stop pretending it's ok to rely on header a.h
automatically pulling in b.h, if a .c file actually uses something
declared in b.h. [Of course, the reality is more complicated; e.g. we
have many cases where one must include linux/foo.h, not asm/foo.h, but
the actual declarations are in the appropriate arch-specific file.
However, we should not rely on linux/bar.h pulling in linux/foo.h.]

>     + an extra work for people using vsprintf function family in
>       new .c files. People are used to get them for free,
>       together with printk().

This is flawed. Not every C source file does a printk, or uses anything
else from printk.h. E.g. a lot of drivers only do the dev_err() family,
some subsystems have their own wrappers, etc. So by moving the
declarations to printk.h you just replace the kernel.h with something
equally bad (essentially all existing headers are bad because they all
include each other recursively). Also, by not moving the declarations to
a separate header, you're ignoring the fact that your own numbers show
that 5/6 of the kernel's TUs would become _smaller_ by not having to
parse those declarations. And the 1/6 that do use sprintf() may become
smaller by thousands of lines once they can avoid kernel.h and all that
that includes recursively.

But those gains can never be achieved if we don't start somewhere, and
if every such baby step results in 20+ message threads.

Rasmus

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Andy Shevchenko]

On Thu, Aug 10, 2023 at 11:09:20AM +0200, Rasmus Villemoes wrote:
> On 10/08/2023 10.15, Petr Mladek wrote:

...

> >     + prolonging the list of #include lines in .c file. It will
> >       not help with maintainability which was one of the motivation
> >       in this patchset.
> 
> We really have to stop pretending it's ok to rely on header a.h
> automatically pulling in b.h, if a .c file actually uses something
> declared in b.h. [Of course, the reality is more complicated; e.g. we
> have many cases where one must include linux/foo.h, not asm/foo.h, but
> the actual declarations are in the appropriate arch-specific file.
> However, we should not rely on linux/bar.h pulling in linux/foo.h.]

Btw, it's easy to enforce IIUC, i.e. by dropping

  #ifndef _FOO_H
  #define _FOO_H
  #endif

mantra from the headers.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Rasmus Villemoes]

On 10/08/2023 15.17, Andy Shevchenko wrote:
> On Thu, Aug 10, 2023 at 11:09:20AM +0200, Rasmus Villemoes wrote:
>> On 10/08/2023 10.15, Petr Mladek wrote:
> 
> ...
> 
>>>     + prolonging the list of #include lines in .c file. It will
>>>       not help with maintainability which was one of the motivation
>>>       in this patchset.
>>
>> We really have to stop pretending it's ok to rely on header a.h
>> automatically pulling in b.h, if a .c file actually uses something
>> declared in b.h. [Of course, the reality is more complicated; e.g. we
>> have many cases where one must include linux/foo.h, not asm/foo.h, but
>> the actual declarations are in the appropriate arch-specific file.
>> However, we should not rely on linux/bar.h pulling in linux/foo.h.]
> 
> Btw, it's easy to enforce IIUC, i.e. by dropping
> 
>   #ifndef _FOO_H
>   #define _FOO_H
>   #endif
> 
> mantra from the headers.
> 

No, you can't do that, because some headers legitimately include other
headers, often for type definitions. Say some struct definition where
one of the members is another struct (struct list_head being an obvious
example). Or a static inline function.

We _also_ don't want to force everybody who includes a.h to ensure that
they first include b.h because something in a.h needs stuff from b.h.

So include guards must be used. They are a so well-known idiom that gcc
even has special code for handling them: If everything in a foo.h file
except comments is inside an ifndef/define/endif, gcc remembers that
that foo.h file has such an include guard, so when gcc then encounters
some #include directive that would again resolve to that same foo.h, and
the include guard hasn't been #undef'ed, it doesn't even do the syscalls
to open/read/close the file again.

Rasmus

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Steven Rostedt]

On Thu, 10 Aug 2023 16:17:57 +0200
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> > Btw, it's easy to enforce IIUC, i.e. by dropping
> > 
> >   #ifndef _FOO_H
> >   #define _FOO_H
> >   #endif
> > 
> > mantra from the headers.
> >   
> 
> No, you can't do that, because some headers legitimately include other
> headers, often for type definitions. Say some struct definition where
> one of the members is another struct (struct list_head being an obvious
> example). Or a static inline function.
> 
> We _also_ don't want to force everybody who includes a.h to ensure that
> they first include b.h because something in a.h needs stuff from b.h.
> 
> So include guards must be used. They are a so well-known idiom that gcc
> even has special code for handling them: If everything in a foo.h file
> except comments is inside an ifndef/define/endif, gcc remembers that
> that foo.h file has such an include guard, so when gcc then encounters
> some #include directive that would again resolve to that same foo.h, and
> the include guard hasn't been #undef'ed, it doesn't even do the syscalls
> to open/read/close the file again.

I hope Andy was just joking with that recommendation.

-- Steve

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Andy Shevchenko]

On Fri, Aug 11, 2023 at 03:28:17PM -0400, Steven Rostedt wrote:
> On Thu, 10 Aug 2023 16:17:57 +0200
> Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
> > > Btw, it's easy to enforce IIUC, i.e. by dropping
> > > 
> > >   #ifndef _FOO_H
> > >   #define _FOO_H
> > >   #endif
> > > 
> > > mantra from the headers.
> > 
> > No, you can't do that, because some headers legitimately include other
> > headers, often for type definitions. Say some struct definition where
> > one of the members is another struct (struct list_head being an obvious
> > example). Or a static inline function.
> > 
> > We _also_ don't want to force everybody who includes a.h to ensure that
> > they first include b.h because something in a.h needs stuff from b.h.
> > 
> > So include guards must be used. They are a so well-known idiom that gcc
> > even has special code for handling them: If everything in a foo.h file
> > except comments is inside an ifndef/define/endif, gcc remembers that
> > that foo.h file has such an include guard, so when gcc then encounters
> > some #include directive that would again resolve to that same foo.h, and
> > the include guard hasn't been #undef'ed, it doesn't even do the syscalls
> > to open/read/close the file again.
> 
> I hope Andy was just joking with that recommendation.

Too radical to be true to implement. But it's always good to have a rationale
(thanks Rasmus) behind existing approach.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Petr Mladek]

On Thu 2023-08-10 11:09:20, Rasmus Villemoes wrote:
> On 10/08/2023 10.15, Petr Mladek wrote:
> 
> > Everyone agrees that kernel.h should be removed. But there are always
> > more possibilities where to move the definitions. For this, the use
> > in C files must be considered. Otherwise, it is just a try&hope approach.
> > 
> >> Also, please, go through all of them and tell, how many of them are using
> >> stuff from kernel.h besides sprintf.h and ARRAY_SIZE() (which I plan
> >> for a long time to split from kernel.h)?
> > 
> > I am all for removing vsprintf declarations from linux.h.
> > 
> > I provided the above numbers to support the idea of moving them
> > into printk.h.
> > 
> > The numbers show that the vsprintf function famility is used
> > quite frequently. IMHO, creating an extra tiny include file
> > will create more harm then good. By the harm I mean:
> > 
> >     + churn when updating 1/6 of source files
> 
> Well, we probably shouldn't do 5000 single-line patches to add that
> sprintf.h include, and another 10000 to add an array-macros.h include
> (just as an example). Some tooling and reasonable batching would
> probably be required. Churn it will be, but how many thousands of
> patches were done to make i2c drivers' probe methods lose a parameter
> (first converting them all to .probe_new, then another round to again
> assign to .probe when that prototype was changed). That's just the cost
> of any tree-wide change in a tree our size.

OK.

> >     + prolonging the list of #include lines in .c file. It will
> >       not help with maintainability which was one of the motivation
> >       in this patchset.
> 
> We really have to stop pretending it's ok to rely on header a.h
> automatically pulling in b.h, if a .c file actually uses something
> declared in b.h.

Yes, we need to find some ballance.

> >     + an extra work for people using vsprintf function family in
> >       new .c files. People are used to get them for free,
> >       together with printk().
> 
> This is flawed. Not every C source file does a printk, or uses anything
> else from printk.h. E.g. a lot of drivers only do the dev_err() family,
> some subsystems have their own wrappers, etc. So by moving the
> declarations to printk.h you just replace the kernel.h with something
> equally bad (essentially all existing headers are bad because they all
> include each other recursively). Also, by not moving the declarations to
> a separate header, you're ignoring the fact that your own numbers show
> that 5/6 of the kernel's TUs would become _smaller_ by not having to
> parse those declarations. And the 1/6 that do use sprintf() may become
> smaller by thousands of lines once they can avoid kernel.h and all that
> that includes recursively.

OK, I did some grepping:

## total number of .c files
pmladek@alley:/prace/kernel/linux> find . -name *.c | wc -l
32319

# printk() usage:

## .c files with printk() calls:
$> git grep  "printk(\|pr_\(emerg\|alert\|crit\|err\|warn\|notice\|info\|cont\|debug\)(" | cut -d ":" -f 1 | uniq | grep "\.c$" | wc -l
8966

    => 28% .c files use printk() directly

## .h files with printk() calls:
$> git grep  "printk(\|pr_\(emerg\|alert\|crit\|err\|warn\|notice\|info\|cont\|debug\)(" | cut -d ":" -f 1 | uniq | grep "\.h$" | wc -l
1006

   => the number is probably much higher because it is also used
      in 1000+ header files.


# vprintf() usage:

## .c files where printk() functions are use without vprintf() functions
$> grep -f printf.list -v  printk.list | wc -l
6725

  => 21% .c files use vprintf() functions directly


# unique usage:

## .c files where vprintf() family functions are used directly
$> git grep sc*n*printf | cut -d : -f1 | uniq | grep "\.c$" | wc -l
5254

  => 75% .c of files using printk() are not using vprintf()

## .c files where vprintf() functions are use without printk() functions
$> grep -f printk.list -v  printf.list | wc -l
3045

  => 45% .c of files using vprintf() are not using printk()


My view:

The overlap will likely be bigger because vprintk() family is often
used directly in .c files but printk() is quite frequently used
indirectly via .h files.

But still, there seems to be non-trivial number of .c files which use
vprintf() and not printk().

=> The split might help after all.

In each case, I do not want to discuss this to the death. And will
not block this patch.

Best Regards,
Petr

RE: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[David Laight]

From: Rasmus Villemoes
> Sent: 10 August 2023 10:09
...
> We really have to stop pretending it's ok to rely on header a.h
> automatically pulling in b.h, if a .c file actually uses something
> declared in b.h. [Of course, the reality is more complicated; e.g. we
> have many cases where one must include linux/foo.h, not asm/foo.h, but
> the actual declarations are in the appropriate arch-specific file.
> However, we should not rely on linux/bar.h pulling in linux/foo.h.]

IMHO (for what it matters) it would be better to focus on why
#include <cdev.h> pulls in around 350 other headers (look at
a .d file) that worry about moving a few files into a new
'leaf' header from somewhere that pretty much everything has
to include anyway.

	David

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

RE: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[David Laight]

...
> If you split headers into so many small pieces then all
> source files will start with 3 screens of includes. I do not see
> how this helps with maintainability.

You also slow down compilations.

A few extra definitions in a 'leaf' header (one without any
#includes) don't really matter.
If a header includes other 'leaf' headers that doesn't matter
much.

But the deep include chains caused by a low level header
including a main header are what causes pretty much every
header to get included in every compilation.

Breaking the deep chains is probably more useful than
adding leaf headers for things that are in a header pretty
much everything in going to include anyway.

The is probably scope for counting the depth of header
includes by looking at what each header includes.

	David

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

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Andy Shevchenko]

On Wed, Aug 09, 2023 at 08:48:54AM +0000, David Laight wrote:
> ...
> > If you split headers into so many small pieces then all
> > source files will start with 3 screens of includes. I do not see
> > how this helps with maintainability.
> 
> You also slow down compilations.

Ingo's patches showed the opposite. Do you have actual try and numbers?

> A few extra definitions in a 'leaf' header (one without any
> #includes) don't really matter.
> If a header includes other 'leaf' headers that doesn't matter
> much.
> 
> But the deep include chains caused by a low level header
> including a main header are what causes pretty much every
> header to get included in every compilation.
> 
> Breaking the deep chains is probably more useful than
> adding leaf headers for things that are in a header pretty
> much everything in going to include anyway.
> 
> The is probably scope for counting the depth of header
> includes by looking at what each header includes.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[David Laight]

From: Andy Shevchenko
> Sent: 10 August 2023 14:14
> 
> On Wed, Aug 09, 2023 at 08:48:54AM +0000, David Laight wrote:
> > ...
> > > If you split headers into so many small pieces then all
> > > source files will start with 3 screens of includes. I do not see
> > > how this helps with maintainability.
> >
> > You also slow down compilations.
> 
> Ingo's patches showed the opposite. Do you have actual try and numbers?

The compiler has to open the extra file on every compile.
If you include it from lots of different places it has to open
it for each one (to find the include guard).
Any attempted compiler optimisations have the same much the
same problem as #pragma once.

With a long -I list even finding the file can take a while.

Probably most obvious when using NFS mounted filesystems.
Especially the 'traditional' NFS protocol that required a
message 'round trip' for each element of the directory path.

	David

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

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

['Andy Shevchenko']

On Mon, Aug 14, 2023 at 08:12:55AM +0000, David Laight wrote:
> From: Andy Shevchenko
> > Sent: 10 August 2023 14:14
> > On Wed, Aug 09, 2023 at 08:48:54AM +0000, David Laight wrote:

...

> > > > If you split headers into so many small pieces then all
> > > > source files will start with 3 screens of includes. I do not see
> > > > how this helps with maintainability.
> > >
> > > You also slow down compilations.
> > 
> > Ingo's patches showed the opposite. Do you have actual try and numbers?
> 
> The compiler has to open the extra file on every compile.
> If you include it from lots of different places it has to open
> it for each one (to find the include guard).
> Any attempted compiler optimisations have the same much the
> same problem as #pragma once.
> 
> With a long -I list even finding the file can take a while.
> 
> Probably most obvious when using NFS mounted filesystems.
> Especially the 'traditional' NFS protocol that required a
> message 'round trip' for each element of the directory path.

Right, as I said come up with numbers. Ingo did that, so can you.
His numbers shows _increase_ of build speed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Steven Rostedt]

On Mon, 7 Aug 2023 18:09:54 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Aug 07, 2023 at 05:03:19PM +0200, Petr Mladek wrote:
> > On Sat 2023-08-05 20:50:26, Andy Shevchenko wrote:  
> > > kernel.h is being used as a dump for all kinds of stuff for a long time.
> > > sprintf() and friends are used in many drivers without need of the full
> > > kernel.h dependency train with it.
> > > 
> > > Here is the attempt on cleaning it up by splitting out sprintf() and
> > > friends.  
> 
> ...
> 
> > I agree that kernel.h is not the right place. But are there any
> > numbers how much separate sprintf.h might safe?
> > Maybe, we should not reinvent the wheel and get inspired by
> > userspace.
> > 
> > sprintf() and friends are basic functions which most people know
> > from userspace. And it is pretty handy that the kernel variants
> > are are mostly compatible as well.
> > 
> > IMHO, it might be handful when they are also included similar way
> > as in userspace. From my POV printk.h is like stdio.h. And we already
> > have include/linux/stdarg.h where the v*print*() function might
> > fit nicely.
> > 
> > How does this sound, please?  
> 
> Not every user (especially _header_) wants to have printk.h included just for
> sprintf.h that may have nothing to do with real output. So, same reasoning
> from me as keeping that in kernel.h, i.e. printk.h no better.
> 

If you separate out the sprintf() into its own header and still include
that in kernel.h, then for what you said in the other email:

> What to do with _headers_ that include kernel.h for no reason other than
> sprintf.h (as an example)? Your suggestion, please?

It can include sprintf.h (or printk.h or stdio.h, whatever) instead of kernel.h.

What's the issue?

-- Steve

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Andy Shevchenko]

On Mon, Aug 07, 2023 at 10:24:55PM -0400, Steven Rostedt wrote:
> On Mon, 7 Aug 2023 18:09:54 +0300
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Aug 07, 2023 at 05:03:19PM +0200, Petr Mladek wrote:
> > > On Sat 2023-08-05 20:50:26, Andy Shevchenko wrote:  
> > > > kernel.h is being used as a dump for all kinds of stuff for a long time.
> > > > sprintf() and friends are used in many drivers without need of the full
> > > > kernel.h dependency train with it.
> > > > 
> > > > Here is the attempt on cleaning it up by splitting out sprintf() and
> > > > friends.  

...

> > > I agree that kernel.h is not the right place. But are there any
> > > numbers how much separate sprintf.h might safe?
> > > Maybe, we should not reinvent the wheel and get inspired by
> > > userspace.
> > > 
> > > sprintf() and friends are basic functions which most people know
> > > from userspace. And it is pretty handy that the kernel variants
> > > are are mostly compatible as well.
> > > 
> > > IMHO, it might be handful when they are also included similar way
> > > as in userspace. From my POV printk.h is like stdio.h. And we already
> > > have include/linux/stdarg.h where the v*print*() function might
> > > fit nicely.
> > > 
> > > How does this sound, please?  
> > 
> > Not every user (especially _header_) wants to have printk.h included just for
> > sprintf.h that may have nothing to do with real output. So, same reasoning
> > from me as keeping that in kernel.h, i.e. printk.h no better.
> 
> If you separate out the sprintf() into its own header and still include
> that in kernel.h, then for what you said in the other email:
> 
> > What to do with _headers_ that include kernel.h for no reason other than
> > sprintf.h (as an example)? Your suggestion, please?
> 
> It can include sprintf.h (or printk.h or stdio.h, whatever) instead of kernel.h.
> 
> What's the issue?

The issue is the same, printk.h brings a lot more than just s*printf().
Why should I include it for a, let's say, single sprintf() call?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[Rasmus Villemoes]

On 07/08/2023 17.03, Petr Mladek wrote:

> I agree that kernel.h is not the right place. But are there any
> numbers how much separate sprintf.h might safe?
> 
> Maybe, we should not reinvent the wheel and get inspired by
> userspace.
> 
> sprintf() and friends are basic functions which most people know
> from userspace. And it is pretty handy that the kernel variants
> are are mostly compatible as well.
> 
> IMHO, it might be handful when they are also included similar way
> as in userspace. From my POV printk.h is like stdio.h. And we already
> have include/linux/stdarg.h where the v*print*() function might
> fit nicely.
> 
> How does this sound, please?

No, please. Let's have a separate header for the functions defined in
vsprintf.c. We really need to trim our headers down to something more
manageable, and stop including everything from everywhere just because
$this little macro needs $that little inline function.

I did https://wildmoose.dk/header-bloat/ many moons ago, I'm sure it
looks even worse today. I also did some sparse-hackery to let it tell me
which macros/functions/types were declared/defined in a given .h file,
and then if some .c file included that .h file but didn't use any of
those, the #include could go away.

Sure, individually, moving the sprintf family out of kernel.h won't save
much (and, of course, nothing at all initially when we're forced to add
an include of that new header from kernel.h). But this technical debt
has crept in over many years, it's not going away in one or two
releases. And use of the sprintf family is very easy to grep for, so
it's a good low-hanging fruit where we should be able to make everybody
who needs one of them include the proper header, and then drop the
include from kernel.h.

Rasmus

RE: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends

[David Laight]

From: Rasmus Villemoes
> Sent: 07 August 2023 20:32
...
> No, please. Let's have a separate header for the functions defined in
> vsprintf.c. We really need to trim our headers down to something more
> manageable, and stop including everything from everywhere just because
> $this little macro needs $that little inline function.

The problem I see isn't things like kernel.h defining a few 'library'
functions, but deep nested includes that means that pretty much all
of the headers get pulled into all the compiles.

Some nested includes sequences can go through an "asm" header
that you might expect to be architecture specific stuff and then
include something like ioctl.h.

Add something like #define IO_WR @@@ to the top a C file
and then see where the compiler finds the duplicate definition.

	David

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

[PATCH v2 3/3] lib/vsprintf: Declare no_hash_pointers in sprintf.h

[Andy Shevchenko]

Sparse is not happy to see non-static variable without declaration:
lib/vsprintf.c:61:6: warning: symbol 'no_hash_pointers' was not declared. Should it be static?

Declare respective variable in the sprintf.h. With this, add a comment
to discourage its use if no real need.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/sprintf.h | 2 ++
 lib/test_printf.c       | 2 --
 mm/kfence/report.c      | 3 +--
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/include/linux/sprintf.h b/include/linux/sprintf.h
index 9ca23bcf9f42..33dcbec71925 100644
--- a/include/linux/sprintf.h
+++ b/include/linux/sprintf.h
@@ -20,6 +20,8 @@ __printf(2, 0) const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list
 __scanf(2, 3) int sscanf(const char *, const char *, ...);
 __scanf(2, 0) int vsscanf(const char *, const char *, va_list);
 
+/* These are for specific cases, do not use without real need */
+extern bool no_hash_pointers;
 int no_hash_pointers_enable(char *str);
 
 #endif	/* _LINUX_KERNEL_SPRINTF_H */
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 5adca19d34e2..cf861dc22169 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -39,8 +39,6 @@ KSTM_MODULE_GLOBALS();
 static char *test_buffer __initdata;
 static char *alloced_buffer __initdata;
 
-extern bool no_hash_pointers;
-
 static int __printf(4, 0) __init
 do_test(int bufsize, const char *expect, int elen,
 	const char *fmt, va_list ap)
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index 197430a5be4a..c509aed326ce 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -13,6 +13,7 @@
 #include <linux/printk.h>
 #include <linux/sched/debug.h>
 #include <linux/seq_file.h>
+#include <linux/sprintf.h>
 #include <linux/stacktrace.h>
 #include <linux/string.h>
 #include <trace/events/error_report.h>
@@ -26,8 +27,6 @@
 #define ARCH_FUNC_PREFIX ""
 #endif
 
-extern bool no_hash_pointers;
-
 /* Helper function to either print to a seq_file or to console. */
 __printf(2, 3)
 static void seq_con_printf(struct seq_file *seq, const char *fmt, ...)
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v2 3/3] lib/vsprintf: Declare no_hash_pointers in sprintf.h

[Marco Elver]

On Sat, 5 Aug 2023 at 19:49, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Sparse is not happy to see non-static variable without declaration:
> lib/vsprintf.c:61:6: warning: symbol 'no_hash_pointers' was not declared. Should it be static?
>
> Declare respective variable in the sprintf.h. With this, add a comment
> to discourage its use if no real need.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Marco Elver <elver@google.com>

> ---
>  include/linux/sprintf.h | 2 ++
>  lib/test_printf.c       | 2 --
>  mm/kfence/report.c      | 3 +--
>  3 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/sprintf.h b/include/linux/sprintf.h
> index 9ca23bcf9f42..33dcbec71925 100644
> --- a/include/linux/sprintf.h
> +++ b/include/linux/sprintf.h
> @@ -20,6 +20,8 @@ __printf(2, 0) const char *kvasprintf_const(gfp_t gfp, const char *fmt, va_list
>  __scanf(2, 3) int sscanf(const char *, const char *, ...);
>  __scanf(2, 0) int vsscanf(const char *, const char *, va_list);
>
> +/* These are for specific cases, do not use without real need */
> +extern bool no_hash_pointers;
>  int no_hash_pointers_enable(char *str);
>
>  #endif /* _LINUX_KERNEL_SPRINTF_H */
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 5adca19d34e2..cf861dc22169 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -39,8 +39,6 @@ KSTM_MODULE_GLOBALS();
>  static char *test_buffer __initdata;
>  static char *alloced_buffer __initdata;
>
> -extern bool no_hash_pointers;
> -
>  static int __printf(4, 0) __init
>  do_test(int bufsize, const char *expect, int elen,
>         const char *fmt, va_list ap)
> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> index 197430a5be4a..c509aed326ce 100644
> --- a/mm/kfence/report.c
> +++ b/mm/kfence/report.c
> @@ -13,6 +13,7 @@
>  #include <linux/printk.h>
>  #include <linux/sched/debug.h>
>  #include <linux/seq_file.h>
> +#include <linux/sprintf.h>
>  #include <linux/stacktrace.h>
>  #include <linux/string.h>
>  #include <trace/events/error_report.h>
> @@ -26,8 +27,6 @@
>  #define ARCH_FUNC_PREFIX ""
>  #endif
>
> -extern bool no_hash_pointers;
> -
>  /* Helper function to either print to a seq_file or to console. */
>  __printf(2, 3)
>  static void seq_con_printf(struct seq_file *seq, const char *fmt, ...)
> --
> 2.40.0.1.gaa8946217a0b
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20230805175027.50029-4-andriy.shevchenko%40linux.intel.com.

Re: [PATCH v2 3/3] lib/vsprintf: Declare no_hash_pointers in sprintf.h

[Petr Mladek]

On Sat 2023-08-05 20:50:27, Andy Shevchenko wrote:
> Sparse is not happy to see non-static variable without declaration:
> lib/vsprintf.c:61:6: warning: symbol 'no_hash_pointers' was not declared. Should it be static?
> 
> Declare respective variable in the sprintf.h. With this, add a comment
> to discourage its use if no real need.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/sprintf.h | 2 ++
>  lib/test_printf.c       | 2 --
>  mm/kfence/report.c      | 3 +--

If we agreed to move sprintf() declarations into printk.h
then this might go to printk.h as well.

Best Regards,
Petr

Re: [PATCH v2 3/3] lib/vsprintf: Declare no_hash_pointers in sprintf.h

[Andy Shevchenko]

On Mon, Aug 07, 2023 at 05:06:33PM +0200, Petr Mladek wrote:
> On Sat 2023-08-05 20:50:27, Andy Shevchenko wrote:
> > Sparse is not happy to see non-static variable without declaration:
> > lib/vsprintf.c:61:6: warning: symbol 'no_hash_pointers' was not declared. Should it be static?
> > 
> > Declare respective variable in the sprintf.h. With this, add a comment
> > to discourage its use if no real need.

> If we agreed to move sprintf() declarations into printk.h
> then this might go to printk.h as well.

Sure, but I disagree with printk.h approach (as I explained why in the reply
to the suggestion).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/3] lib/vsprintf: Rework header inclusions

[Petr Mladek]

On Sat 2023-08-05 20:50:24, Andy Shevchenko wrote:
> Some patches that reduce the mess with the header inclusions related to
> vsprintf.c module. Each patch has its own description, and has no
> dependencies to each other, except the collisions over modifications
> of the same places. Hence the series.
> 
> Changelog v2:
> - covered test_printf.c in patches 1 & 2
> - do not remove likely implict inclusions (Rasmus)
> - declare no_hash_pointers in sprintf.h (Marco, Steven, Rasmus)
> 
> Andy Shevchenko (3):
>   lib/vsprintf: Sort headers alphabetically

I am sorry but I am still against this patch?

>   lib/vsprintf: Split out sprintf() and friends
>   lib/vsprintf: Declare no_hash_pointers in sprintf.h

I am fine with these two.

Would you mind preparing v3 without the sorting patch, please?

Best Regards,
Petr

Re: [PATCH v2 0/3] lib/vsprintf: Rework header inclusions

[Andy Shevchenko]

On Mon, Aug 14, 2023 at 05:38:17PM +0200, Petr Mladek wrote:
> On Sat 2023-08-05 20:50:24, Andy Shevchenko wrote:
> > Some patches that reduce the mess with the header inclusions related to
> > vsprintf.c module. Each patch has its own description, and has no
> > dependencies to each other, except the collisions over modifications
> > of the same places. Hence the series.
> > 
> > Changelog v2:
> > - covered test_printf.c in patches 1 & 2
> > - do not remove likely implict inclusions (Rasmus)
> > - declare no_hash_pointers in sprintf.h (Marco, Steven, Rasmus)
> > 
> > Andy Shevchenko (3):
> >   lib/vsprintf: Sort headers alphabetically
> 
> I am sorry but I am still against this patch?
> 
> >   lib/vsprintf: Split out sprintf() and friends
> >   lib/vsprintf: Declare no_hash_pointers in sprintf.h
> 
> I am fine with these two.
> 
> Would you mind preparing v3 without the sorting patch, please?

Yes. Thank you for the review.

-- 
With Best Regards,
Andy Shevchenko