Re: [PATCH printk v1 07/13] printk: move buffer definitions into console_emit_next_record() caller

[John Ogness <john.ogness@linutronix.de>]

On 2022-02-16, Petr Mladek <pmladek@suse.com> wrote:
>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index 822b7b6ad6d1..02bde45c1149 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -2597,13 +2611,13 @@ static bool console_emit_next_record(struct console *con, bool *handover)
>>  		goto skip;
>>  	}
>>  
>> -	if (con->flags & CON_EXTENDED) {
>> -		write_text = &ext_text[0];
>> -		len = info_print_ext_header(ext_text, sizeof(ext_text), r.info);
>> -		len += msg_print_ext_body(ext_text + len, sizeof(ext_text) - len,
>> +	if (ext_text) {
>> +		write_text = ext_text;
>> +		len = info_print_ext_header(ext_text, CONSOLE_EXT_LOG_MAX, r.info);
>> +		len += msg_print_ext_body(ext_text + len, CONSOLE_EXT_LOG_MAX - len,
>>  					  &r.text_buf[0], r.info->text_len, &r.info->dev_info);
>>  	} else {
>> -		write_text = &text[0];
>> +		write_text = text;
>>  		len = record_print_text(&r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);
>
> @text and @ext_text buffers are never used at the same time. It might
> be enough to use a single text[CONSOLE_EXT_LOG_MAX] buffer. It would
> even slightly simplify the code.

No, they _are_ used at the same time.

r.text_buf is @text. msg_print_ext_body() takes @ext_text and
&r.text_buf[0]. Unfortunately msg_print_ext_body() does not work "in
place" like record_print_text() does.

>> @@ -2650,6 +2664,9 @@ static bool console_emit_next_record(struct console *con, bool *handover)
>>   */
>>  static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handover)
>>  {
>> +	static char dropped_text[DROPPED_TEXT_MAX];
>> +	static char ext_text[CONSOLE_EXT_LOG_MAX];
>> +	static char text[CONSOLE_LOG_MAX];
>
> These buffers are for printing from console_unlock(). The same buffers
> will need to be allocated for each console in the kthreads.
>
> It might make sense to allocate these buffers in register_console()
> and store the pointers in struct console.
>
> Well, we might need extra buffers for atomic console drivers and
> diffent contexts that would be used during panic. But maybe
> they can be allocated in register_console() as well.

register_console() happens quite early. But my plan for v2 is to make
them global static variables and allocate them on the first
register_console().

>>  	bool any_usable = false;
>>  	struct console *con;
>>  	bool any_progress;
>> @@ -2667,7 +2684,16 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
>>  				continue;
>>  			any_usable = true;
>>  
>> -			progress = console_emit_next_record(con, handover);
>> +			if (con->flags & CON_EXTENDED) {
>> +				/* Extended consoles do not print "dropped messages". */
>> +				progress = console_emit_next_record(con, &text[0],
>
> IMHO, &text[0] buffer is not used for extended consoles.

Yes. msg_print_ext_body() needs it.

>> +								    &ext_text[0], NULL,
>> +								    handover);
>> +			} else {
>> +				progress = console_emit_next_record(con, &text[0],
>> +								    NULL, &dropped_text[0],
>> +								    handover);
>> +			}
>>  			if (*handover)
>>  				return true;
>
> I do not resist on allocating the buffers in register_console(). I am
> not sure if it would really makes things easier.

I'll give it a try for v2.

John