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

[John Ogness]

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.

> 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.

READ_ONCE()/WRITE_ONCE() are perfectly fine for what we are doing. The
writes (for registered consoles) are synchronized by the
console_list_lock. There is no need to use atomic operations.

> 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.

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.

John