Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 10 Oct 2022 17:40:13 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH printk 17/18] printk: Use an output descriptor struct for
 emit
Message-ID: <Y0Q83TZnQcCF/mDi@alley>
References: <20220924000454.3319186-1-john.ogness@linutronix.de>
 <20220924000454.3319186-18-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220924000454.3319186-18-john.ogness@linutronix.de>
Precedence: bulk

On Sat 2022-09-24 02:10:53, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> To prepare for a new console infrastructure that is independent of the
> console BKL, wrap the output mode into a descriptor struct so the new
> infrastrucure can share the emit code that dumps the ringbuffer record
> into the output text buffers.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
>  include/linux/console.h | 15 +++++++
>  kernel/printk/printk.c  | 88 ++++++++++++++++++++++++++++++-----------
>  2 files changed, 79 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 05c7325e98f9..590ab62c01d9 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -187,6 +187,21 @@ struct cons_text_buf {
>  	char		text[CONSOLE_LOG_MAX];
>  };
>  
> +/**
> + * struct cons_outbuf_desc - console output buffer descriptor
> + * @txtbuf:		Pointer to buffer for storing the text
> + * @outbuf:		Pointer to the position in @buffer for
> + *			writing it out to the device

This sounds like this pointer might point into the middle of
some buffer. It sounds scarry without providing the remaining
size of the buffer.

It seems that the pointer is actually used to point to
one of the buffers in txtbuf struct. Then it is again
a bit scarry without the size. I know that it is defined
by @len. But it is not obvious that it is related.

> + * @len:		Message length

It is not clear that it is length of the outbuf.

> + * @extmsg:		Select extended format printing

It would be nice to make it obvious (variable name)
that it is bool and not another buffer.

This actually defines which buffer will be used
in txtbuf.

> + */
> +struct cons_outbuf_desc {
> +	struct cons_text_buf	*txtbuf;
> +	char			*outbuf;
> +	unsigned int		len;
> +	bool			extmsg;
> +};

Sigh, I somehow do not like this structure. I think that the main
problem is that it combines both input and output values.

Also there is too much assignments here and there.

What about?

1. Storing "struct cons_text_buf *txtbuf" into struct console.
   Normal consoles might point to a global txtbuf.
   Atomic consoles might point to the allocated ones.

2. Create structure for writing the next record
   on the console, for example:

   struct console_record {	/* like struct printk_record */
	char *buf;
	int size;
	int len;
   }

Then we could implement:

bool console_get_record(struct console *con,
			struct console_record *cr)
{
	struct cons_text_buf *txtbuf = con->txtbuf;
	struct printk_info info;
	struct printk_record r;
	char *write_text;
	size_t len;

	cr->buf = NULL;
	cr->size = 0;
	cr->len = 0;

	prb_rec_init_rd(&r, &info, txtbuf->text, sizeof(txtbuf->text);

	if (!prb_read_valid(prb, desc->seq, &r))
		return false;

	/* Skip record that has level above the console loglevel. */
	if (suppress_message_printing(r.info->level)) {
		return true;
	}

	if (con->flags & CON_EXTENDED) {
		cr->buf = txtbuf->ext_text;
		cr->size = sizeof(txtbuf->ext_text);
		info_print_ext_header(cr, r.info);
		msg_print_ext_body(cr, &r);
	} else {
		cr->buf = txtbuf->text;
		cr->size = sizeof(txtbuf->text);
		record_print_text(cr, &r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);

		cons_print_dropped(cr, con);
	}

	return true;
}

and

static bool console_emit_next_record(struct console *con,
				     bool *handover)
{
	struct console_record cr;
	unsigned long flags;

	*handover = false;

	if (!console_get_next_record(con, cr))
		return false;

	/* supressed? */
	if (!cr->buf) {
		con->seq++;
		return true;
	}

	/*
	 * While actively printing out messages, if another printk()
	 * were to occur on another CPU, it may wait for this one to
	 * finish. This task can not be preempted if there is a
	 * waiter waiting to take over.
	 *
	 * Interrupts are disabled because the hand over to a waiter
	 * must not be interrupted until the hand over is completed
	 * (@console_waiter is cleared).
	 */
	printk_safe_enter_irqsave(flags);
	console_lock_spinning_enable();

	/* don't trace print latency */
	stop_critical_timings();
	/* Write everything out to the hardware */
	con->write(con, cr->buf, cr->len);
	start_critical_timings();

	con->seq++;

	*handover = console_lock_spinning_disable_and_check();
	printk_safe_exit_irqrestore(flags);

	return true;
}

Advantages:

	+ even less parameters
	+ less assignments (read/write directly in struct console)
	+ struct console_record has just output buffer =>
	  no confusion about the names

How does that sound, please?

Best Regards,
Petr