[printk,v2,1/7] printk: Move buffer size defines

  From: Thomas Gleixner <tglx@linutronix.de>

Move the buffer size defines to console.h in preparation of adding a
buffer structure. The new buffer structure will be embedded within
struct console. Therefore console.h was chosen as the new home for
these defines.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/console.h | 14 ++++++++++++++
 include/linux/printk.h  |  2 --
 kernel/printk/printk.c  |  4 ----
 3 files changed, 14 insertions(+), 6 deletions(-)
  

On Thu 2022-11-24 00:19:54, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Move the buffer size defines to console.h in preparation of adding a
> buffer structure. The new buffer structure will be embedded within
> struct console. Therefore console.h was chosen as the new home for
> these defines.

The buffers are not embedded into struct console in this patchset.
Are they going to be added directly or via pointer, please?

IMHO, it is always better to hide these implementation details
in an internal header or source file. It will be possible
if struct console contained on a pointer to the buffers.


> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  include/linux/console.h | 14 ++++++++++++++
>  include/linux/printk.h  |  2 --
>  kernel/printk/printk.c  |  4 ----
>  3 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 9cea254b34b8..799fc3216aad 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -122,6 +122,20 @@ static inline int con_debug_leave(void)
>  #define CM_ERASE    (2)
>  #define CM_MOVE     (3)
>  
> +#ifdef CONFIG_PRINTK
> +
> +/* The maximum size of a formatted record (i.e. with prefix added per line) */
> +#define CONSOLE_LOG_MAX		1024
> +
> +#else
> +
> +#define CONSOLE_LOG_MAX		0
> +
> +#endif
> +
> +/* The maximum size of a formatted extended record */
> +#define CONSOLE_EXT_LOG_MAX	8192

It looks strange that we need the buffer for extended messages
and not the normal one when !CONFIG_PRINTK. I can't find any reason
for this. It looks like a historic inconsistency.

CONSOLE_EXT_LOG_MAX was added into printk.h in the commit
d43ff430f434d862db59 ("printk: guard the amount written per line
by devkmsg_read()"). It didn't have to be in include/linux/printk.h
at that time. But nobody cared, including me, I was a greenhorn ;-)

Well, it would be nice to fix it when we are moving the definition
next to each other.

Best Regards,
Petr
  

On 2022-11-24, Petr Mladek <pmladek@suse.com> wrote:
>> Move the buffer size defines to console.h in preparation of adding a
>> buffer structure. The new buffer structure will be embedded within
>> struct console. Therefore console.h was chosen as the new home for
>> these defines.
>
> The buffers are not embedded into struct console in this patchset.
> Are they going to be added directly or via pointer, please?

By "embedded" I mean added directly. The buffers need to be available
immediately and cannot be allocated or assigned dynamically. The console
struct is generally defined by drivers with:

static struct console my_console = {
   ...
};

I could think of no way to statically define the buffers but keep their
sizes hidden.

> IMHO, it is always better to hide these implementation details
> in an internal header or source file. It will be possible
> if struct console contained on a pointer to the buffers.

The problem is not pointers, it is static definition (without knowing
the size of the thing that is statically defined). The new thread/atomic
consoles run in parallel, so they cannot share the single static buffer
like we do now.

>> --- a/include/linux/console.h
>> +++ b/include/linux/console.h
>> @@ -122,6 +122,20 @@ static inline int con_debug_leave(void)
>>  #define CM_ERASE    (2)
>>  #define CM_MOVE     (3)
>>  
>> +#ifdef CONFIG_PRINTK
>> +
>> +/* The maximum size of a formatted record (i.e. with prefix added per line) */
>> +#define CONSOLE_LOG_MAX		1024
>> +
>> +#else
>> +
>> +#define CONSOLE_LOG_MAX		0
>> +
>> +#endif
>> +
>> +/* The maximum size of a formatted extended record */
>> +#define CONSOLE_EXT_LOG_MAX	8192
>
> It looks strange that we need the buffer for extended messages
> and not the normal one when !CONFIG_PRINTK. I can't find any reason
> for this. It looks like a historic inconsistency.

Yes, it looked that way to me as well. I will move it under
CONFIG_PRINTK for v3.

John
  

On Thu 2022-11-24 13:44:29, John Ogness wrote:
> On 2022-11-24, Petr Mladek <pmladek@suse.com> wrote:
> >> Move the buffer size defines to console.h in preparation of adding a
> >> buffer structure. The new buffer structure will be embedded within
> >> struct console. Therefore console.h was chosen as the new home for
> >> these defines.
> >
> > The buffers are not embedded into struct console in this patchset.
> > Are they going to be added directly or via pointer, please?
> 
> By "embedded" I mean added directly. The buffers need to be available
> immediately and cannot be allocated or assigned dynamically. The console
> struct is generally defined by drivers with:
> 
> static struct console my_console = {
>    ...
> };
> 
> I could think of no way to statically define the buffers but keep their
> sizes hidden.
> 
> > IMHO, it is always better to hide these implementation details
> > in an internal header or source file. It will be possible
> > if struct console contained on a pointer to the buffers.
> 
> The problem is not pointers, it is static definition (without knowing
> the size of the thing that is statically defined). The new thread/atomic
> consoles run in parallel, so they cannot share the single static buffer
> like we do now.

Let me to play a devil advocate first:

Well, allocation is possible long before scheduling is
possible. It is actually available even before early parameters
are proceed where the boot consoles are registered. At least
it is used when setup_log_buf(1) is called in setup_arch() on x86.

The motivation is that only thread/atomic consoles would need
the console-specific buffer. The other consoles might share
the global one.

It would be useful even for atomic consoles. IMHO, most users
use generic kernels that support a variety of hardware. They
would provide static buffers for many console drivers but
only one or two would be used in the end.

Also the atomic consoles would need these buffers for each context.
It might be even more useful to allocate them dynamically.

Or do I miss something, please?


That said:

I do not have any numbers at hands to show how this
is important. Also I do not know if the early allocations
have some limits.

The static buffers might be acceptable for simplicity and
reliability after all.

Feel free to keep them static. I would ack the next version
of this patch after making the CONSOLE_EXT_LOG_MAX dependent
on CONFIG_PRINTK.

Best Regards,
Petr
  

On 2022-11-24, Petr Mladek <pmladek@suse.com> wrote:
> The motivation is that only thread/atomic consoles would need
> the console-specific buffer. The other consoles might share
> the global one.

I understand what you are saying. I will change it to a pointer and
assign it to an internal shared global static buffer on
register_console(). Then we can keep the size defines private.

For the upcoming thread/atomic consoles, I will setup the sprint-buffers
differently.

> Also the atomic consoles would need these buffers for each context.
> It might be even more useful to allocate them dynamically.

Yes, atomic consoles need dedicated per-console, per-cpu, per-context
buffers. Some of these are allocated dynamically. I will revisit this
with the idea of minimizing static buffers.

John