Messages in this thread | | | Date | Fri, 19 Feb 2021 18:57:08 +0100 | From | Petr Mladek <> | Subject | synchronization model: was: Re: [PATCH printk-rework 09/14] printk: introduce a kmsg_dump iterator |
| |
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
| |