From: Thomas Gleixner <[email protected]>
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 <[email protected]>
Signed-off-by: John Ogness <[email protected]>
---
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
+
/*
* The interface for a console, or any other device that wants to capture
* console messages (printer driver?)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 8c81806c2e99..8ef499ab3c1e 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -44,8 +44,6 @@ static inline const char *printk_skip_headers(const char *buffer)
return buffer;
}
-#define CONSOLE_EXT_LOG_MAX 8192
-
/* printk's without a loglevel use this.. */
#define MESSAGE_LOGLEVEL_DEFAULT CONFIG_MESSAGE_LOGLEVEL_DEFAULT
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9ec101766471..a4854a60e6d8 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -471,9 +471,6 @@ static struct latched_seq clear_seq = {
#define PREFIX_MAX 32
#endif
-/* the maximum size of a formatted record (i.e. with prefix added per line) */
-#define CONSOLE_LOG_MAX 1024
-
/* the maximum size for a dropped text message */
#define DROPPED_TEXT_MAX 64
@@ -2387,7 +2384,6 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
#else /* CONFIG_PRINTK */
-#define CONSOLE_LOG_MAX 0
#define DROPPED_TEXT_MAX 0
#define printk_time false
--
2.30.2
On Thu 2022-11-24 00:19:54, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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