Re: synchronization model: was: Re: [PATCH printk-rework 09/14] printk: introduce a kmsg_dump iterator

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

On 2021-02-19, Petr Mladek <pmladek@suse.com> wrote:
> This is likely beyond the scope of this patchset.

It would be beyond the scope of this patchset because it is not related
to logbuf_lock removal.

> I am still scratching my head about the synchronization if these dumpers.
>
> There is the "active" flag. It has been introduced by the commit
> e2ae715d66bf4becfb ("kmsg - kmsg_dump() use iterator to receive log
> buffer content"). I do not see any explanation there.
>
> It might prevent some misuse of the API. But the synchronization
> model is not much clear:
>
> 	+ cur_seq and next_seq might be manipulated by
> 	  kmsg_dump_rewind() even when the flag is not set.
>
> 	+ It is possible to use the same dumper more times in parallel.
> 	  The API will fill the provided buffer of all callers
> 	  as long as the active flag is set.
>
> 	+ The "active" flag does not synchronize other operations with
> 	  the provided buffer. The "dump" callback is responsible
> 	  to provide some synchronization on its own.
>
> In fact, it is not much clear how struct kmsg_dumper_iter, struct kmsg_dumper,
> and the used buffers are connected with each other and synchronized.

With this series applied, there is no connection between them. And
actually you have made me realize that the iterator should be named
"kmsg_dump_iter" instead. I will change that for v3.

> It might some sense to have the iterator in a separate structure.
> But the only safe scenario seems to be when all these three things
> (both structures and the buffer) are connected together and
> synchronized by the same lock. Also the "active" flag does not look
> much helpful and can be removed.

The @active flag is useless. It should be removed.

We have kmsg_dump_get_line(), kmsg_dump_get_buffer(), kmsg_dump_rewind()
as an in-kernel interface to allow retrieving the kernel buffer
contents. To use these interfaces, the caller only needs to have an
iterator that is initialized using kmsg_dump_rewind(). These functions
can be (and are) used, regardless if a dumper has been registered. And I
think that is OK.

The used buffers (like the iterator) are local to the caller. So there
is no need for the kmsg_dump_*() functions to be concerned about any
synchronization there.

Then we have kmsg_dump_register() and kmsg_dump_unregister() to allow
for registration of a dump() callback, to be called when the kernel does
panic/oops/emergency/shutdown. Presumably the registered callback would
use the kmsg_dump_*() functions to access the kernel buffer. Again, no
need for kmsg_dump_*() functions to be concerned about synchronization
because the buffers are provided by the callbacks.

> As I said, this is likely beyond this patchset. This patch does more
> or less just a refactoring and helps to understand the dependencies.

Aside from removing the useless @active flag, I am not sure what else
you would want to change. Perhaps just fixup the comments/documentation
to clarify these interfaces and what their purpose is.

John Ogness