Re: [PATCH printk v3 10/19] printk: nbcon: Use thread callback if in task context for legacy

[Petr Mladek]

On Mon 2024-07-22 19:25:30, John Ogness wrote:
> When printing via console_lock, the write_atomic() callback is
> used for nbcon consoles. However, if it is known that the
> current context is a task context, the write_thread() callback
> can be used instead.
> 
> Using write_thread() instead of write_atomic() helps to reduce
> large disabled preemption regions when the device_lock does not
> disable preemption.
> 
> This is mainly a preparatory change to allow avoiding
> write_atomic() completely during normal operation if boot
> consoles are registered.
> 
> Signed-off-by: John Ogness <john.ogness@xxxxxxxxxxxxx>

Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>

Well, I would update a comment, see below.

> ---
>  kernel/printk/internal.h |  4 +--
>  kernel/printk/nbcon.c    | 54 +++++++++++++++++++++++++++++-----------
>  kernel/printk/printk.c   |  5 ++--
>  3 files changed, 45 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index 12605e0aff11..bb02788acc7c 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -91,7 +91,7 @@ void nbcon_free(struct console *con);
>  enum nbcon_prio nbcon_get_default_prio(void);
>  void nbcon_atomic_flush_pending(void);
>  bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
> -				   int cookie);
> +				   int cookie, bool use_atomic);
>  bool nbcon_kthread_create(struct console *con);
>  void nbcon_kthread_stop(struct console *con);
>  void nbcon_wake_kthreads(void);
> @@ -174,7 +174,7 @@ static inline void nbcon_free(struct console *con) { }
>  static inline enum nbcon_prio nbcon_get_default_prio(void) { return NBCON_PRIO_NONE; }
>  static inline void nbcon_atomic_flush_pending(void) { }
>  static inline bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
> -						 int cookie) { return false; }
> +						 int cookie, bool use_atomic) { return false; }
>  static inline void nbcon_kthread_wake(struct console *con) { }
>  
>  static inline bool console_is_usable(struct console *con, short flags,
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index 69cecf97bf24..233ab8f90fef 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1294,9 +1294,10 @@ enum nbcon_prio nbcon_get_default_prio(void)
>  }
>  
>  /*
> - * nbcon_atomic_emit_one - Print one record for an nbcon console using the
> - *				write_atomic() callback
> + * nbcon_emit_one - Print one record for an nbcon console using the
> + *			specified callback
>   * @wctxt:	An initialized write context struct to use for this context
> + * @use_atomic:	True if the write_atomic() callback is to be used
>   *
>   * Return:	True, when a record has been printed and there are still
>   *		pending records. The caller might want to continue flushing.
> @@ -1309,7 +1310,7 @@ enum nbcon_prio nbcon_get_default_prio(void)
>   * This is an internal helper to handle the locking of the console before
>   * calling nbcon_emit_next_record().

This is not completely true when @use_atomic == false. The function
takes only the nbcon context. But also con->device_lock()
is needed for the non-atomic case.

We should either update the commit message. Or it might make sense
to move "con->device_lock()/unlock() stuff to nbcon_emit_one
so that it actually does all the necessary locking.

BTW: We could use this function in nbcon_kthread_func() then as well.

    */
> -static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
> +static bool nbcon_emit_one(struct nbcon_write_context *wctxt, bool use_atomic)
>  {
>  	struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
>  

Otherwise, it looks good.

Best Regards,
Petr