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

[Petr Mladek]

On Wed 2022-10-19 17:01:49, John Ogness wrote:
> Guarantee safe iteration of the console list by using SRCU.
> 
> Note that in the case of a handover, the SRCU read lock is also
> released. This is documented in the function description and as
> comments in the code. It is a bit tricky, but this preserves the
> lockdep lock ordering for the context handing over the
> console_lock:
> 
>   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

I believe that we could avoid this complexity, see below.

> Signed-off-by: John Ogness <john.ogness@xxxxxxxxxxxxx>
> ---
>  kernel/printk/printk.c | 38 ++++++++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 7ff2fc75fc3b..c4d5d58b5977 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2808,7 +2815,8 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
>  	do {
>  		any_progress = false;
>  
> -		for_each_console(con) {
> +		cookie = console_srcu_read_lock();
> +		for_each_console_srcu(con) {
>  			bool progress;
>  
>  			if (!console_is_usable(con))
> @@ -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.

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;


Or do I miss anything, please?

> @@ -2838,14 +2851,19 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
>  
>  			/* Allow panic_cpu to take over the consoles safely. */
>  			if (abandon_console_lock_in_panic())
> -				return false;
> +				goto abandon;
>  
>  			if (do_cond_resched)
>  				cond_resched();
>  		}
> +		console_srcu_read_unlock(cookie);
>  	} while (any_progress);
>  
>  	return any_usable;
> +
> +abandon:
> +	console_srcu_read_unlock(cookie);
> +	return false;
>  }

Best Regards,
Petr