Re: [Xen-devel] [PATCH v3 4/4] xen: add per-cpu buffer option to debugtrace

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.08.2019 10:00, Juergen Gross wrote:
> @@ -24,32 +25,62 @@ struct debugtrace_data_s {
>  };
>  
>  static struct debugtrace_data_s *debtr_data;
> +static DEFINE_PER_CPU(struct debugtrace_data_s *, debtr_cpu_data);
>  
> -static unsigned int debugtrace_kilobytes = 128;
> +static unsigned int debugtrace_bytes = 128 << 10;

And after patch 3 this is now left as "unsigned int"?

> +static bool debugtrace_per_cpu;
>  static bool debugtrace_used;
>  static DEFINE_SPINLOCK(debugtrace_lock);
> -integer_param("debugtrace", debugtrace_kilobytes);
>  
> -static void debugtrace_dump_worker(void)
> +static int __init debugtrace_parse_param(const char *s)
> +{
> +    if ( !strncmp(s, "cpu:", 4) )
> +    {
> +        debugtrace_per_cpu = true;
> +        s += 4;
> +    }
> +    debugtrace_bytes =  parse_size_and_unit(s, NULL);

Stray double blank.

> +    return 0;
> +}
> +custom_param("debugtrace", debugtrace_parse_param);
> +
> +static void debugtrace_dump_buffer(struct debugtrace_data_s *data,
> +                                   const char *which)
>  {
> -    if ( !debtr_data || !debugtrace_used )
> +    if ( !data )
>          return;
>  
> -    printk("debugtrace_dump() starting\n");
> +    printk("debugtrace_dump() %s buffer starting\n", which);
>  
>      /* Print oldest portion of the ring. */
> -    ASSERT(debtr_data->buf[debtr_data->bytes - 1] == 0);
> -    if ( debtr_data->buf[debtr_data->prd] != '\0' )
> -        console_serial_puts(&debtr_data->buf[debtr_data->prd],
> -                            debtr_data->bytes - debtr_data->prd - 1);
> +    ASSERT(data->buf[data->bytes - 1] == 0);
> +    if ( data->buf[data->prd] != '\0' )
> +        console_serial_puts(&data->buf[data->prd], data->bytes - data->prd - 
> 1);

Seeing this getting changed yet another time I now really wonder if
this nul termination is really still needed now that a size is being
passed into the actual output function. If you got rid of this in an
early prereq patch, the subsequent (to that one) ones would shrink.

Furthermore I can't help thinking that said change to pass the size
into the actual output functions actually broke the logic here: By
memset()-ing the buffer to zero, output on a subsequent invocation
would have been suitably truncated (in fact, until prd had wrapped,
I think it would have got truncated more than intended). Julien,
can you please look into this apparent regression?

> @@ -156,33 +195,70 @@ static void debugtrace_key(unsigned char key)
>      debugtrace_toggle();
>  }
>  
> -static int __init debugtrace_init(void)
> +static void debugtrace_alloc_buffer(struct debugtrace_data_s **ptr)
>  {
>      int order;
> -    unsigned long kbytes, bytes;
>      struct debugtrace_data_s *data;
>  
> -    /* Round size down to next power of two. */
> -    while ( (kbytes = (debugtrace_kilobytes & (debugtrace_kilobytes-1))) != 
> 0 )
> -        debugtrace_kilobytes = kbytes;
> -
> -    bytes = debugtrace_kilobytes << 10;
> -    if ( bytes == 0 )
> -        return 0;
> +    if ( debugtrace_bytes < PAGE_SIZE || *ptr )

Why the check against PAGE_SIZE?

> +        return;
>  
> -    order = get_order_from_bytes(bytes);
> +    order = get_order_from_bytes(debugtrace_bytes);
>      data = alloc_xenheap_pages(order, 0);
>      if ( !data )
> -        return -ENOMEM;
> +    {
> +        printk("failed to allocate debugtrace buffer\n");

Perhaps better to also indicate which/whose buffer?

> +        return;
> +    }
> +
> +    memset(data, '\0', debugtrace_bytes);
> +    data->bytes = debugtrace_bytes - sizeof(*data);
> +
> +    *ptr = data;
> +}
> +
> +static int debugtrace_cpu_callback(struct notifier_block *nfb,
> +                                   unsigned long action, void *hcpu)
> +{
> +    unsigned int cpu = (unsigned long)hcpu;
> +
> +    /* Buffers are only ever allocated, never freed. */
> +    switch ( action )
> +    {
> +    case CPU_UP_PREPARE:
> +        debugtrace_alloc_buffer(&per_cpu(debtr_cpu_data, cpu));
> +        break;
> +    default:
> +        break;

There no apparent need for "default:" here; quite possibly this
could be if() instead of switch() anyway.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel