Re: [PATCH v6 02/11] printk: Use struct console for suppression and extended console state

[Petr Mladek <pmladek@suse.com>]

On Mon 2024-10-28 16:45:34, Chris Down wrote:
> In preparation for supporting per-console loglevels, modify
> printk_get_next_message() to accept the console itself instead of
> individual arguments that mimic its fields. As part of the upcoming
> per-console loglevel support we need the console object here anyway, so
> it makes sense to amortise this now.
> 
> devkmsg_read() has special behaviour here, but all other consoles follow
> the same patterns and can have their extension/suppression states
> determined from their struct console.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2925,20 +2925,19 @@ void console_prepend_replay(struct printk_message *pmsg)
>   * @pmsg will contain the formatted result. @pmsg->pbufs must point to a
>   * struct printk_buffers.
>   *
> + * @con is the console in question. Only @con->flags and @con->level are
> + * guaranteed to be valid at this point. Note especially well that con->seq is
> + * not yet guaranteed to be consistent with @seq.

@con->level does not exist at this point.

But more importantly, the read of @con->flags and @con->level is
sychronized only by SRCU read lock. It means that the word "valid"
is a bit misleading. SRCU just guarantees that the struct console
can't disappear.

I would write something like:

<proposal>
 * @con might point to the console where the read message will be emitted.
 * It is used to determine the format of the message and whether it would get
 * suppressed. The sequence number stored in the struct console is updated
 * by the caller depending on whether the emission succeeds.
 *
 * @con might be NULL when the message is used for another purpose,
 * for example, devkmsg.
</proposal>

> + *
>   * @seq is the record to read and format. If it is not available, the next
>   * valid record is read.
>   *
> - * @is_extended specifies if the message should be formatted for extended
> - * console output.
> - *
> - * @may_supress specifies if records may be skipped based on loglevel.
> - *
>   * Returns false if no record is available. Otherwise true and all fields
>   * of @pmsg are valid. (See the documentation of struct printk_message
>   * for information about the @pmsg fields.)
>   */
> -bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
> -			     bool is_extended, bool may_suppress)
> +bool printk_get_next_message(struct printk_message *pmsg, struct console *con,
> +			     u64 seq)
>  {
>  	struct printk_buffers *pbufs = pmsg->pbufs;
>  	const size_t scratchbuf_sz = sizeof(pbufs->scratchbuf);
> @@ -2948,6 +2947,14 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>  	struct printk_info info;
>  	struct printk_record r;
>  	size_t len = 0;
> +	bool is_extended;
> +
> +	if (con) {
> +		is_extended = console_srcu_read_flags(con) & CON_EXTENDED;

I guess that we would need to implement similar API also for reading
the per-console loglevel. I mean that we should check the SRCU lock
is held. And describe the potential data_race() if the value might
be modified by a sysfs interface in parallel.

Let's discuss this in the patch adding the read. I mention it here
rather just as a mental note. The proposed comment says that it will
be synchronized using SRCU. We need to make sure that it will
be valid also for the loglevel stuff.

> +	} else {
> +		/* Used only by devkmsg_read(). */
> +		is_extended = true;
> +	}
>  
>  	/*
>  	 * Formatting extended messages requires a separate buffer, so use the

Anyway, the change looks fine. With the updated comment:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr