Messages in this thread | | | Date | Tue, 8 Nov 2022 18:40:30 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH printk v3 08/40] printk: use console_is_enabled() |
| |
On Mon 2022-11-07 15:22:06, John Ogness wrote: > Replace (console->flags & CON_ENABLED) usage with console_is_enabled() > if it involves a data race. Otherwise add comments mentioning why the > wrapper is not used.
The wrapper will be used/needed only on few locations. There are many more locations where the console flags are modified without any locking.
Note that it is not only about CON_ENABLE flag. If we started playing the game with WRITE_ONCE()/READ_ONCE() then we would need to consider all locations where the flags are modified.
In the end, it might be easier to use the proposed console_set_flag(), console_clear_flag(), console_check_flag(), and console_check_flag_unsafe(), instead of documenting all the locations.
Also it is more important to document why it is acceptable to use the racy variant. The wrappers would make sure that all the other accesses are safe.
> Note that this is a preparatory change for when console_lock no longer > provides synchronization for console->flags. > > Signed-off-by: John Ogness <john.ogness@linutronix.de> > --- > kernel/printk/printk.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c > index 79811984da34..f243bb56a3ba 100644 > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -2660,7 +2660,7 @@ static bool abandon_console_lock_in_panic(void) > */ > static inline bool console_is_usable(struct console *con) > { > - if (!(con->flags & CON_ENABLED)) > + if (!console_is_enabled(con))
I agree that it makes sense to do this now. console_is_usable() is used in two cycles. They will get switched to the srcu walk one by one.
Just please document that this allows to use console_is_usable() under console_srcu_read_lock. And that in this case, the value of the flag might get cleared at any time but the console still might be used as long as the console_srcu_read_lock is held.
We should actually add a check into console_is_enabled() that either console_lock or console_srcu_read_lock is held. The console_lock should later be changed to console_list_lock.
> return false; > > if (!con->write) > @@ -2946,7 +2946,7 @@ void console_unblank(void) > console_locked = 1; > console_may_schedule = 0; > for_each_console(c) > - if ((c->flags & CON_ENABLED) && c->unblank) > + if (console_is_enabled(c) && c->unblank)
I would prefer to do this change together with switching to for_each_console_srcu(). It would be more clear why this change is needed.
> c->unblank(); > console_unlock(); > > @@ -3104,8 +3104,11 @@ static int try_enable_preferred_console(struct console *newcon, > * Some consoles, such as pstore and netconsole, can be enabled even > * without matching. Accept the pre-enabled consoles only when match() > * and setup() had a chance to be called. > + * > + * Note that reading @flags is race-free because the console is not > + * yet added to the console list.
Nit: This is not completely true. We just know that it will not get modified by the printk/console framework because the console is not registered yet.
Well, I could live with it. The comment is good enough. I am still more concerned about how to distinguish when READ_ONCE()/WRITE_ONCE() is needed. And it would affect all accesses to the flags.
> */ > - if (newcon->flags & CON_ENABLED && c->user_specified == user_specified) > + if ((newcon->flags & CON_ENABLED) && (c->user_specified == user_specified)) > return 0; > > return -ENOENT;
Best Regards, Petr
| |