Messages in this thread | | | Date | Mon, 7 Nov 2022 17:10:09 +0100 | From | Petr Mladek <> | Subject | cosmetic: was: Re: [patch RFC 19/29] printk: Add basic infrastructure for non-BKL consoles |
| |
On Sun 2022-09-11 00:28:01, Thomas Gleixner wrote: > The current console/printk subsystem is protected by a Big Kernel Lock, > aka. console_lock which has has ill defined semantics and is more or less > stateless. This puts severe limitations on the console subsystem and makes > forced takeover and output in emergency and panic situations a fragile > endavour which is based on try and pray. > > The goal of non-BKL consoles is to break out of the console lock jail and > to provide a new infrastructure which avoids the pitfalls and allows > console drivers to be gradually converted over. > > The proposed infrastructure aims for the following properties: > > - Lockless (SCRU protected) console list walk > - Per console locking instead of global locking > - Per console state which allows to make informed decisions > - Stateful handover and takeover > > As a first step this adds state to struct console. The per console state is > a atomic_long_t with a 32bit bit field and on 64bit a 32bit sequence for > tracking the last printed ringbuffer sequence number. On 32bit the sequence > is seperate from state for obvious reasons which requires to handle a few > extra race conditions. > > Add the initial state with the most basic 'alive' and 'enabled' bits and > wire it up into the console register/unregister functionality and exclude > such consoles from being handled in the console BKL mechanisms. > > The decision to use a bitfield was made as using a plain u32 and mask/shift > operations turned out to result in uncomprehensible code. > > --- a/include/linux/console.h > +++ b/include/linux/console.h > @@ -237,6 +272,9 @@ struct console { > unsigned long dropped; > void *data; > struct hlist_node node; > + > + /* NOBKL console specific members */ > + atomic_long_t __private atomic_state[2];
Just to be sure about the meaning. "real" state means the current state and "handover" means a requested state.
> }; > > #ifdef CONFIG_LOCKDEP > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -2339,7 +2339,9 @@ static bool suppress_message_printing(in > static bool pr_flush(int timeout_ms, bool reset_on_progress) { return true; } > static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress) { return true; } > > -#endif /* CONFIG_PRINTK */ > +#endif /* !CONFIG_PRINTK */ > + > +#include "printk_nobkl.c"
Is there any chance to get rid of this?
If we need to use some of this API in printk.c then please declare it either in "internal.h" or in a new "printk_noblk.h".
Honestly, I do not have any real arguments why it is bad. But there are probably reasons why it is not a common pattern. IMHO, split sources might help to:
+ speed up compilation + separate public and internal API + keep #ifdef/#else/#endif close each other in .h files + keep the sources somehow usable even without cscope + ???
> #ifdef CONFIG_EARLY_PRINTK > struct console *early_console; > @@ -2635,6 +2637,13 @@ static bool abandon_console_lock_in_pani > */ > static inline bool console_is_usable(struct console *con) > { > + /* > + * Exclude the NOBKL consoles. They are handled seperately > + * as they do not require the console BKL > + */ > + if ((con->flags & CON_NO_BKL)) > + return false;
This is confusing. Nobody would expect that a function called "console_is_usable()" would return false just because the console has CON_NO_BLK flag set.
Either we need a better name, for example, console_is_blk_and_usable(). Or please put the test into a separate function, e.g. console_is_blk() and check it separately where needed.
IMHO, the original console_is_usable() would be useful even for CON_NO_BLK consoles.
> + > if (!(con->flags & CON_ENABLED)) > return false; > > --- /dev/null > +++ b/kernel/printk/printk_nobkl.c > @@ -0,0 +1,176 @@ > + > +enum state_selector { > + STATE_REAL, > + STATE_HANDOVER, > +};
It might be problem that I am not a native speaker. But the names are a bit ambiguous to me. I would personally use:
enum state_selector { CON_STATE_CURRENT, CON_STATE_REQUESTED, };
or if it is too long: CON_STATE_CUR and CON_STATE_REQ.
Well, I do not resist on the change. I am not sure how the proposed names would play with the followup patches. The original names might be good after all. They are not that bad. I primary wanted to document my first reaction ;-)
> +/** > + * cons_nobkl_init - Initialize the NOBKL console state > + * @con: Console to initialize > + */ > +static void cons_nobkl_init(struct console *con) > +{ > + struct cons_state state = { > + .alive = 1, > + .enabled = !!(con->flags & CON_ENABLED), > + }; > + > + cons_state_set(con, STATE_REAL, &state); > +}
IMHO. we need to update the function description, e.g.
/** * cons_nobkl_init - Initialize the NOBKL console specific data * @con: Console to initialize */
Background:
The function name does not match the rest:
+ The function name suggests that it initializes NOBLK console.
+ The function description and the implementation suggests that it initializes struct cons_state.
I see that the followup patches update this function. It initializes all the members needed by noblk consoles in struct console. It allocates per-CPU data and creates the kthread. It means that the function name is reasonable after all.
> + > +/** > + * cons_nobkl_cleanup - Cleanup the NOBKL console state > + * @con: Console to cleanup > + */ > +static void cons_nobkl_cleanup(struct console *con) > +{ > + struct cons_state state = { }; > + > + cons_state_set(con, STATE_REAL, &state); > +}
Same as with cons_noblk_init(). The function does a lot more in the later patches. The description should be
/** * cons_nobkl_cleanup - Cleanup the NOBKL console specific data * @con: Console to cleanup */
Best Regards, Petr
| |