Re: [PATCH v6 3/4] xen/console: use memcpy() in console_init_ring()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, May 08, 2026 at 05:57:13PM -0700, dmukhin@xxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx> 
> 
> Make console_init_ring() more efficient by using memcpy()'s, rather than
> copying the ring a byte at a time.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

A couple of notes below.

> ---
> Changes since v5:
> - fixed memcpy() logic
> ---
>  xen/drivers/char/console.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 5ab3b0de12d8..5cac87d052b9 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -463,7 +463,8 @@ static void cf_check conring_dump_keyhandler(unsigned 
> char key)
>  void __init console_init_ring(void)
>  {
>      char *ring;
> -    unsigned int i, order, memflags;
> +    XENCONS_RING_IDX i, size;
> +    unsigned int order, memflags;
>      unsigned long flags;
>  
>      if ( !opt_conring_size )
> @@ -479,8 +480,22 @@ void __init console_init_ring(void)
>      opt_conring_size = PAGE_SIZE << order;
>  
>      nrspin_lock_irqsave(&console_lock, flags);
> -    for ( i = conringc ; i != conringp; i++ )
> -        ring[i & (opt_conring_size - 1)] = conring[i & (conring_size - 1)];
> +
> +    i = 0;
> +    size = conringp - conringc;
> +    while ( i < size )
> +    {
> +        XENCONS_RING_IDX src = (conringc + i) & (conring_size - 1);
> +        XENCONS_RING_IDX dst = (conringc + i) & (opt_conring_size - 1);
> +        XENCONS_RING_IDX n;
> +
> +        n = min(opt_conring_size - dst, conring_size - src);
> +        n = min(size - i, n);
> +
> +        memcpy(&ring[dst], &conring[src], n);
> +        i += n;
> +    }

It's a bit more complex logic for not much gain IMO: this will only be
used once to move from the init buffer to the dynamically allocated
one.  A couple of notes, feel free to ignore if you don't think it's
an improvement: I would rename `i` to `done`, as `i` is usually
associated with an index, and here it's just an accumulator of the
amount of characters processed.  I would also consider making this a
for loop, by pulling n to the outer context, ie:

XENCONS_RING_IDX done, size, n;
[...]

for ( done = 0; done < size; done += n )
{
    XENCONS_RING_IDX src = (conringc + done) & (conring_size - 1);
    XENCONS_RING_IDX dst = (conringc + done) & (opt_conring_size - 1);

    n = min(opt_conring_size - dst, conring_size - src);
    n = min(size - done, n);

    memcpy(&ring[dst], &conring[src], n);
}

Thanks, Roger.