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

[Petr Mladek <pmladek@suse.com>]

On Thu 2021-02-18 09:18:12, John Ogness wrote:
> Rather than store the iterator information into the registered
> kmsg_dump structure, create a separate iterator structure. The
> kmsg_dump_iter structure can reside on the stack of the caller,
> thus allowing lockless use of the kmsg_dump functions.
> 
> This is in preparation for removal of @logbuf_lock.
> 
> diff --git a/include/linux/kmsg_dump.h b/include/linux/kmsg_dump.h
> index 4095a34db0fa..2fdb10ab1799 100644
> --- a/include/linux/kmsg_dump.h
> +++ b/include/linux/kmsg_dump.h
> @@ -29,6 +29,18 @@ enum kmsg_dump_reason {
>  	KMSG_DUMP_MAX
>  };
>  
> +/**
> + * struct kmsg_dumper_iter - iterator for kernel crash message dumper
> + * @active:	Flag that specifies if this is currently dumping
> + * @cur_seq:	Points to the oldest message to dump (private)
> + * @next_seq:	Points after the newest message to dump (private)
> + */
> +struct kmsg_dumper_iter {
> +	bool	active;
> +	u64	cur_seq;
> +	u64	next_seq;
> +};
> +

This is likely beyond the scope of this patchset.

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.

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.

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

It is possible that it will be more clear the following week
with a fresh mind.

Best Regards,
Petr