Messages in this thread | | | From | John Ogness <> | Subject | Re: [PATCH printk v2 27/38] printk: console_flush_all: use srcu console list iterator | Date | Mon, 07 Nov 2022 01:06:02 +0106 |
| |
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
| |