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

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
To: Petr Mladek <pmladek@suse.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Marco Elver <elver@google.com>,
	linux-kernel@vger.kernel.org, kasan-dev@googlegroups.com,
	linux-mm@kvack.org, Steven Rostedt <rostedt@goodmis.org>,
	Sergey Senozhatsky <senozhatsky@chromium.org>,
	Alexander Potapenko <glider@google.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH v2 2/3] lib/vsprintf: Split out sprintf() and friends
Date: Thu, 10 Aug 2023 11:09:20 +0200	[thread overview]
Message-ID: <67ddbcec-b96f-582c-a38c-259234c3f301@rasmusvillemoes.dk> (raw)
In-Reply-To: <ZNScla_5FXc28k32@alley>

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