Messages in this thread | | | Date | Mon, 7 Nov 2022 14:03:31 +0100 | From | Petr Mladek <> | Subject | Re: [PATCH printk v2 27/38] printk: console_flush_all: use srcu console list iterator |
| |
On Mon 2022-11-07 01:06:02, John Ogness wrote: > 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.
Ah, I see. I should be read all the comments more carefully. I do not know about any better solution. Feel free to use:
Reviewed-by: Petr Mladek <pmladek@suse.com>
Note:
The complexity is caused by calling mutex_release() in console_lock_spinning_disable_and_check() and mutex_acquire() in console_trylock_spinning().
I wondered if we really need to do so. These functions actually do not release or acquire the console_lock. But it seems that it is necessary because lockdep is not able to track the lock when it was moved into another process.
I even tried to replace mutex_acquire()/mutex_release() with rwsem_acquire()/rwsem_release(). But it did not help. lockdep still complained when I removed these calls from the _spinning_() API. Sigh.
Best Regards, Petr
| |