Re: [PATCH printk v2 27/38] printk: console_flush_all: use srcu console list iterator

John Ogness
2022-11-07T00:00:11Z

On 2022-10-25, Petr Mladek <pmladek@suse.com> wrote:
>> console_lock()
>> | mutex_acquire(&console_lock_dep_map) <-- console lock
>> |
>> console_unlock()
>> | console_flush_all()
>> | | srcu_read_lock(&console_srcu) <-- srcu lock
>> | | console_emit_next_record()
>> | | | console_lock_spinning_disable_and_check()
>> | | | | srcu_read_unlock(&console_srcu) <-- srcu unlock
>> | | | | mutex_release(&console_lock_dep_map) <-- console unlock
>>
>> @@ -2819,12 +2827,17 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
>> /* Extended consoles do not print "dropped messages". */
>> progress = console_emit_next_record(con, &text[0],
>> &ext_text[0], NULL,
>> - handover);
>> + handover, cookie);
>> } else {
>> progress = console_emit_next_record(con, &text[0],
>> NULL, &dropped_text[0],
>> - handover);
>> + handover, cookie);
>> }
>> +
>> + /*
>> + * If a handover has occurred, the SRCU read lock
>> + * is already released.
>> + */
>> if (*handover)
>> return false;
>
> Please, release the SRCU read lock here:
>
> if (*handover) {
> console_srcu_read_unlock(cookie);
> return false;
> }
>
> The lock should be released in the same function where it was taken.
> It does not require passing the cookie and looks more straightforward.

It looks more straight forward, but it is incorrect from a locking
perspective.

The locking order was:

console_lock()
console_srcu_read_lock()

But for a handover at this point in code, console_emit_next_record() has
already released the console_lock (to the spinning context). The
console_srcu_read_lock should have been released first.

> We actually do the same when abandon_console_lock_in_panic()
> returns true. We could share the code:
>
> handover_abandon:
> console_srcu_read_unlock(cookie);
> return false;

This case is different. Here the console_lock was not released yet so it
is fine to perform the console_srcu_read_unlock() here.

John Ogness