Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 25 Oct 2022 17:17:55 +0200
From:   Petr Mladek <pmladek@suse.com>
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH printk v2 27/38] printk: console_flush_all: use srcu
 console list iterator
Message-ID: <Y1f+Izra36qQ4ae6@alley>
References: <20221019145600.1282823-1-john.ogness@linutronix.de>
 <20221019145600.1282823-28-john.ogness@linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221019145600.1282823-28-john.ogness@linutronix.de>
Precedence: bulk

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