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

  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

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)
  

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@linutronix.de>
> ---
>  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
  

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
  

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