Re: [PATCH printk v4 20/27] printk: Avoid console_lock dance if no legacy or boot consoles

[Petr Mladek]

On Wed 2024-04-03 12:07:32, John Ogness wrote:
> On 2024-04-03, John Ogness <john.ogness@xxxxxxxxxxxxx> wrote:
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index df84c6bfbb2d..4ff3800e8e8e 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -3810,6 +3833,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> >  	u64 last_diff = 0;
> >  	u64 printk_seq;
> >  	short flags;
> > +	bool locked;
> >  	int cookie;
> >  	u64 diff;
> >  	u64 seq;
> > @@ -3819,22 +3843,35 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> >  	seq = prb_next_reserve_seq(prb);
> >  
> >  	/* Flush the consoles so that records up to @seq are printed. */
> > -	console_lock();
> > -	console_unlock();
> > +	if (printing_via_unlock) {
> > +		console_lock();
> > +		console_unlock();
> > +	}
> >  
> >  	for (;;) {
> >  		unsigned long begin_jiffies;
> >  		unsigned long slept_jiffies;
> >  
> > +		locked = false;
> >  		diff = 0;
> >  
> > +		if (printing_via_unlock) {
> > +			/*
> > +			 * Hold the console_lock to guarantee safe access to
> > +			 * console->seq. Releasing console_lock flushes more
> > +			 * records in case @seq is still not printed on all
> > +			 * usable consoles.
> > +			 */
> > +			console_lock();
> > +			locked = true;
> > +		}
> > +
> >  		/*
> > -		 * Hold the console_lock to guarantee safe access to
> > -		 * console->seq. Releasing console_lock flushes more
> > -		 * records in case @seq is still not printed on all
> > -		 * usable consoles.
> > +		 * Ensure the compiler does not optimize @locked to be
> > +		 * @printing_via_unlock since the latter can change at any
> > +		 * time.
> >  		 */
> > -		console_lock();
> > +		barrier();
> 
> When I originally implemented this, my objective was to force the
> compiler to use a local variable. But to be fully paranoid (which we
> should) we must also forbid the compiler from being able to do this
> nonsense:
> 
> if (printing_via_unlock) {
> 	console_lock();
> 	locked = printing_via_unlock;
> }
> 
> I suggest replacing the __pr_flush() hunks to be as follows instead
> (i.e. ensure all conditional console lock usage within the loop is using
> the local variable):
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index df84c6bfbb2d..1dbd2a837b67 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3819,22 +3842,34 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
>  	seq = prb_next_reserve_seq(prb);
>  
>  	/* Flush the consoles so that records up to @seq are printed. */
> -	console_lock();
> -	console_unlock();
> +	if (printing_via_unlock) {
> +		console_lock();
> +		console_unlock();
> +	}
>  
>  	for (;;) {
>  		unsigned long begin_jiffies;
>  		unsigned long slept_jiffies;
> -
> -		diff = 0;
> +		bool use_console_lock = printing_via_unlock;
>  
>  		/*
> -		 * Hold the console_lock to guarantee safe access to
> -		 * console->seq. Releasing console_lock flushes more
> -		 * records in case @seq is still not printed on all
> -		 * usable consoles.
> +		 * Ensure the compiler does not optimize @use_console_lock to
> +		 * be @printing_via_unlock since the latter can change at any
> +		 * time.
>  		 */
> -		console_lock();
> +		barrier();

Indeed, this looks better. I have missed this as well.

It seems that using the barrier() is more tricky than I have expected.
I wonder if it would be better to use WRITE_ONCE()/READ_ONCE().
It would be more clear what we want to achieve. Also it would let
the compiler to optimize other things. Not that the the performance is
importnat here but...

Otherwise, the patch looks fine. With this change, feel free to use:

Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>

Best Regargds,
Petr