Re: [PATCH printk v2 17/18] printk: Avoid false positive lockdep report for legacy printing

[Petr Mladek]

On Tue 2024-06-04 01:30:52, John Ogness wrote:
> Legacy console printing from printk() caller context may invoke
> the console driver from atomic context. This leads to a lockdep
> splat because the console driver will acquire a sleeping lock
> and the caller may already hold a spinning lock. This is noticed
> by lockdep on !PREEMPT_RT configurations because it will lead to
> a problem on PREEMPT_RT.
> 
> However, on PREEMPT_RT the printing path from atomic context is
> always avoided and the console driver is always invoked from a
> dedicated thread. Thus the lockdep splat on !PREEMPT_RT is a
> false positive.
> 
> For !PREEMPT_RT override the lock-context before invoking the
> console driver to avoid the false positive.
> 
> Do not override the lock-context for PREEMPT_RT in order to
> allow lockdep to catch any real locking context issues related
> to the write callback usage.

Could you please provide more information?
Is this anything new?
How did it work so far?

See below.

> Signed-off-by: John Ogness <john.ogness@xxxxxxxxxxxxx>
> ---
>  kernel/printk/printk.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index ea2d66152256..5e9778f69206 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2980,6 +2980,33 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
>  	return true;
>  }
>  
> +/*
> + * Legacy console printing from printk() caller context does not respect
> + * raw_spinlock/spinlock nesting. For !PREEMPT_RT the lockdep warning is a
> + * false positive. For PREEMPT_RT the false positive condition does not
> + * occur.
> + *
> + * This map is used to establish LD_WAIT_SLEEP context for the console write
> + * callbacks when legacy printing to avoid false positive lockdep complaints,
> + * thus allowing lockdep to continue to function for real issues.
> + */
> +#ifdef CONFIG_PREEMPT_RT
> +static inline void printk_legacy_lock_map_acquire_try(void) { }
> +static inline void printk_legacy_lock_map_release(void) { }
> +#else
> +static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_SLEEP);
> +
> +static inline void printk_legacy_lock_map_acquire_try(void)
> +{
> +	lock_map_acquire_try(&printk_legacy_map);
> +}
> +
> +static inline void printk_legacy_lock_map_release(void)
> +{
> +	lock_map_release(&printk_legacy_map);
> +}
> +#endif /* CONFIG_PREEMPT_RT */
> +
>  /*
>   * Used as the printk buffers for non-panic, serialized console printing.
>   * This is for legacy (!CON_NBCON) as well as all boot (CON_BOOT) consoles.
> @@ -3035,7 +3062,7 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
>  		/*
>  		 * With forced threading this function is either in a thread
>  		 * or panic context. So there is no need for concern about
> -		 * printk reentrance or handovers.
> +		 * printk reentrance, handovers, or lockdep complaints.
>  		 */
>  
>  		con->write(con, outbuf, pmsg.outbuf_len);
> @@ -3057,7 +3084,9 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
>  		/* Do not trace print latency. */
>  		stop_critical_timings();
>  
> +		printk_legacy_lock_map_acquire_try();

I am not familiar with lockdep in these depths. The comment in
include/kernel/lockdep_types.h says that LD_WAIT_SLEEP is
a type used for sleeping locks. And this code says that
it is acquire_try(). I would assume that similar lockdep
map is set also by "console_trylock()".

Do we need this for a new callpath without console_trylock()?
Or do I miss anything?

>  		con->write(con, outbuf, pmsg.outbuf_len);
> +		printk_legacy_lock_map_release();
>  
>  		start_critical_timings();

Best Regards,
Petr