Re: [PATCH printk v3 5/6] printk: introduce console_get_next_message() and console_message

[Petr Mladek]

On Tue 2023-01-03 16:03:17, John Ogness wrote:
> On 2023-01-03, Petr Mladek <pmladek@xxxxxxxx> wrote:
> > On Thu 2022-12-22 16:47:39, John Ogness wrote:
> >> On 2022-12-21, John Ogness <john.ogness@xxxxxxxxxxxxx> wrote:
> >> > +static bool console_emit_next_record(struct console *con, bool *handover, int cookie)
> >> > +{
> >> > +	bool is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
> >> > +	static char dropped_text[DROPPED_TEXT_MAX];
> >> > +	static struct console_buffers cbufs;
> >> > +	static struct console_message cmsg = {
> >> > +		.cbufs = &cbufs,
> >> > +	};
> >> 
> >> @cmsg should not be static. The whole point of the console_message
> >> wrapper struct is so that it can sit on the stack.
> >
> > Well, it might actually be better to keep it static for now.
> > It always points to static struct console_buffers cbufs anyway.
> >
> > It would make sense to have it on stack only when it uses
> > different buffers.
> 
> I think we should avoid making things static when it serves no purpose.

I agree. Well, I still feel that both structures are tightly
connected and there should be 1:1 relation. See below.

> > Which brings the question. Does it makes sense to use
> > the same buffers by different struct console_message?
> > Will it be safe in any situation?
> >
> > I did not want to complicate it yesterday. I think that
> > I have already proposed this. But this brings back
> > the question whether it makes sense to have two structures
> > at all.
> >
> > I still think that it would be easier and even more safe
> > to put everything into struct console_message.
> >
> > I mean to have:
> >
> > struct console_message {
> > 	char			buf[CONSOLE_EXT_LOG_MAX];
> > 	char			scratch_buf[LOG_LINE_MAX];
> > 	unsigned int		len;
> > 	u64			seq;
> > 	unsigned long		dropped;
> > };
> 
> The current atomic console proposal allocates 1x cbuf per-cpu and 4x
> meta-data per-cpu. Different contexts of a cpu will have different
> meta-data, but all the contexts of a cpu will share the same cbuf.
>
> If cbufs become embedded in cmsg, then we would allocate 1x cmsg
> per-cpu. But the atomic consoles would still need their own 4x per-cpu
> meta-data.

Do we really need 4x the meta data?

The metadata describe the state of the buffer. Using the buffer in one
context invalidates the metadata in the other context.

By other words, if one context reuses the buffer it might also
reuse the metadata. We just need to inform the original context
that it would need to break or re-start the operation.  But we would
need to do this anyway.

This is my intuitive view. It is possible that I miss some
important detail.

> When looking at the proposal code, it looks wrong to have meta-data
> fields in the cmsg struct that are not being used. But maybe that is
> acceptable during the "transition phase" until all legacy consoles are
> gone.

I am not sure what you mean by "not being used".

When I try to find "struct console_buffers" with cscope
than I see it in:

	+ struct console_message definition:
	    + part of the structure

	+ console_prepend_dropped()
	+ console_get_next_message()
	    + both read the pointer from the given struct message

	+ console_emit_next_record()
	    + static definition
	    + the pointer is stored into struct console_message

In all situations, struct console_message is needed when
struct console_buffers is manipulated. By other words,
the metadata are always needed when the buffers are used.

> For v4 I will drop the console_buffers struct. I will use your
> suggestion.

Please, do not do it just to make me happy. My intention
is to keep things simple. And keeping the two structures
synced needs an extra code.

If you are sure that the separation will really be needed
in the future then feel free to keep the two structures.

Best Regards,
Petr