Re: [PATCH printk v3 07/40] console: introduce console_is_enabled() wrapper

[Petr Mladek]

On Thu 2022-11-10 16:11:43, John Ogness wrote:
> On 2022-11-08, Petr Mladek <pmladek@xxxxxxxx> wrote:
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -3021,7 +3021,7 @@ void console_stop(struct console *console)
> >>  {
> >>  	__pr_flush(console, 1000, true);
> >>  	console_lock();
> >> -	console->flags &= ~CON_ENABLED;
> >> +	WRITE_ONCE(console->flags, console->flags & ~CON_ENABLED);
> >
> > My first reaction is that using the atomic operation only for the
> > store side is suspicious. It is correct because the read is serialized
> > by console_lock(). But it is far from obvious why we need and can do
> > it this way.
> 
> The READ_ONCE()/WRITE_ONCE() usage is really about documenting data-race
> reads and the writes that they are racing with.
> 
> For WRITE_ONCE() the rule is:
> 
> - If console->flags is modified for a registered console, it is done
>   under the console_list_lock and using WRITE_ONCE().
> 
> If we use a wrapper for this rule, then we can also add the lockdep
> assertion that console_list_lock is held.
>
> For READ_ONCE() the rule is:
> 
> - If console->flags is read for a registered console, then either
>   console_list_lock must be held _or_ it must be read via READ_ONCE().
>
> If we use wrappers here, then we can use lockdep assertion on
> console_list_lock for the non-READ_ONCE wrapper, and scru_read_lock
> assertion for the READ_ONCE wrapper.

Exactly, all the assertions are one big advantage for hiding this into
an API.

> > It would deserve a comment. But there are several other writes.
> > Also it is not obvious why many other con->flags modifications
> > do not need this.
> >
> > I think about hiding this into an API. We could also add some
> > checks that it is used the right way. Also it might make sense
> > to avoid using the READ_ONCE()/WRITE_ONCE by using
> > set_bit()/test_bit().
> 
> I do not see any advantage of set_bit()/test_bit(). They have the
> disadvantage that they only work with 1 bit at a time. And there are
> multiple sites where more than 1 bit is set/tested. It is important that
> the multi-bit tests are simultaneous.

Fair enough.

> > I would prefer to use the proposed API because it should make all
> > accesses more clear and safe. And most importantly, it would help use
> > to catch bugs.
> >
> > But I do not resist on it. The patch looks correct and we could do
> > this later. I could live with it if we add some comments above the
> > WRITE_ONCE() calls.
> 
> I do not want to do a full API replacement for all console->flags access
> in this series or at this time. I am concerned that it is taking us too
> far away from our current goal. Also, with the upcoming atomic/threaded
> model, all consoles need to be modified that want to use it anyway. So
> that would be a more appropriate time to require the use of new API's.

Fair enough.

> For console_is_enabled() I will add the srcu_read_lock check. I suppose
> I should also name the function console_srcu_is_enabled().
> 
> For the WRITE_ONCE() calls, I will add a static inline wrapper in
> printk.c that includes the lockdep console_list_lock assertion. Perhaps
> called console_srcu_write_flags(struct console *con, short flags).
> 
> In console_srcu_write_flags() and console_srcu_is_enabled() I can
> document their relationship and when they are to be used. Both these
> functions are used rarely and should be considered the exception, not
> the rule.
> 
> For code that is reading registered console->flags under the
> console_list_lock, I will leave the "normal access" as is. Just as I am
> leaving the "normal access" for non-registered console-flags as is. We
> can convert those to a new generic API later if we think it is really
> necessary.

Sounds reasonable.

The main motivation for introducing the wrappers is that there are
currently 40+ locations where console->flags are touched. It is easy
to miss something especially when we are reworking the locking around
this code. Some of the callers are outside kernel/printk/* which
even increases the risk of a misuse. The lockdep checks
would help us to catch potential mistakes.

Anyway, I am fine with adding the wrappers for the synchronized reads
later. This patchset is already big enough.

Best Regards,
Petr