Re: [PATCH v1 1/3] xen/console: cleanup conring management

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 3 Apr 2025, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> Move console_locks_busted handling inside conring_puts() to remove
> tasklet code duplication.
> 
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>

This patch is a good cleanup but makes one functional change: previously
guest_console_write would always call tasklet_schedule. Now, it only
calls tasklet_schedule if !console_locks_busted.

On ARM, we don't call console_force_unlock and never set
console_locks_busted. It makes no difference.

On x86, there are a few callers of console_force_unlock, so it would
make a difference. However, looking at the callers, it seems to me that
the change is for the better and better aligns the code with the
intention behind console_force_unlock.

On my side:
Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

but it would be good for an x86 maintainer to confirm


> ---
>  xen/drivers/char/console.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index c3150fbdb7..aaa97088aa 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -325,6 +325,17 @@ static void cf_check do_dec_thresh(unsigned char key, 
> bool unused)
>   * ********************************************************
>   */
>  
> +static void cf_check notify_dom0_con_ring(void *unused)
> +{
> +    send_global_virq(VIRQ_CON_RING);
> +}
> +
> +static DECLARE_SOFTIRQ_TASKLET(notify_dom0_con_ring_tasklet,
> +                               notify_dom0_con_ring,
> +                               NULL);
> +
> +static bool console_locks_busted;
> +
>  static void conring_puts(const char *str, size_t len)
>  {
>      ASSERT(rspin_is_locked(&console_lock));
> @@ -334,6 +345,9 @@ static void conring_puts(const char *str, size_t len)
>  
>      if ( conringp - conringc > conring_size )
>          conringc = conringp - conring_size;
> +
> +    if ( !console_locks_busted )
> +        tasklet_schedule(&notify_dom0_con_ring_tasklet);
>  }
>  
>  long read_console_ring(struct xen_sysctl_readconsole *op)
> @@ -594,13 +608,6 @@ static void cf_check serial_rx(char c)
>      __serial_rx(c);
>  }
>  
> -static void cf_check notify_dom0_con_ring(void *unused)
> -{
> -    send_global_virq(VIRQ_CON_RING);
> -}
> -static DECLARE_SOFTIRQ_TASKLET(notify_dom0_con_ring_tasklet,
> -                               notify_dom0_con_ring, NULL);
> -
>  #ifdef CONFIG_X86
>  static inline void xen_console_write_debug_port(const char *buf, size_t len)
>  {
> @@ -648,10 +655,7 @@ static long 
> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
>  #endif
>  
>              if ( opt_console_to_ring )
> -            {
>                  conring_puts(kbuf, kcount);
> -                tasklet_schedule(&notify_dom0_con_ring_tasklet);
> -            }
>  
>              nrspin_unlock_irq(&console_lock);
>          }
> @@ -753,8 +757,6 @@ long do_console_io(
>   * *****************************************************
>   */
>  
> -static bool console_locks_busted;
> -
>  static void __putstr(const char *str)
>  {
>      size_t len = strlen(str);
> @@ -775,9 +777,6 @@ static void __putstr(const char *str)
>  #endif
>  
>      conring_puts(str, len);
> -
> -    if ( !console_locks_busted )
> -        tasklet_schedule(&notify_dom0_con_ring_tasklet);
>  }
>  
>  static int printk_prefix_check(char *p, char **pp)
> -- 
> 2.34.1
> 
>