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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.09.2019 15:46, Juergen Gross wrote:
> @@ -25,33 +26,63 @@ struct debugtrace_data {
>  };
>  
>  static struct debugtrace_data *dt_data;
> +static DEFINE_PER_CPU(struct debugtrace_data *, dt_cpu_data);
>  
> -static unsigned int debugtrace_kilobytes = 128;
> +static unsigned long debugtrace_bytes = 128 << 10;
> +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 ( !debugtrace_used )
> +    if ( !strncmp(s, "cpu:", 4) )
> +    {
> +        debugtrace_per_cpu = true;
> +        s += 4;
> +    }
> +    debugtrace_bytes = parse_size_and_unit(s, NULL);

I'm afraid you can't pass NULL here, or a trailing % will be at best
ambiguous. In fact, the latest with the addition of % support to the
function I can't see (anymore) how calling the function with a NULL
2nd arg can be a good idea at all.

> +static void debugtrace_dump_worker(void)
> +{
> +    unsigned int cpu;
> +    char buf[16];
> +
> +    if ( !debugtrace_used )
> +        return;
> +
> +    debugtrace_dump_buffer(dt_data, "global");
> +
> +    for ( cpu = 0; cpu < nr_cpu_ids; cpu++ )
> +    {
> +        snprintf(buf, sizeof(buf), "cpu %u", cpu);
> +        debugtrace_dump_buffer(per_cpu(dt_cpu_data, cpu), buf);
> +    }

I think it would be nice if buf[]'s scope was just the for() body.

>  void debugtrace_printk(const char *fmt, ...)
>  {
>      static char buf[1024], last_buf[1024];
> -    static unsigned int count, last_count;
> +    static unsigned int count, last_count, last_cpu;
>  
>      char          cntbuf[24];
>      va_list       args;
>      unsigned long flags;
>      unsigned int nr;
> +    struct debugtrace_data *data;
> +    unsigned int cpu;
>  
> -    if ( !dt_data )
> +    data = debugtrace_per_cpu ? this_cpu(dt_cpu_data) : dt_data;
> +    cpu = debugtrace_per_cpu ? smp_processor_id() : 0;

You use "cpu" only ...

> @@ -131,16 +169,17 @@ void debugtrace_printk(const char *fmt, ...)
>      }
>      else
>      {
> -        if ( strcmp(buf, last_buf) )
> +        if ( strcmp(buf, last_buf) || cpu != last_cpu )
>          {
> -            dt_data->prd_last = dt_data->prd;
> +            data->prd_last = data->prd;
>              last_count = ++count;
> +            last_cpu = cpu;
>              safe_strcpy(last_buf, buf);
>              snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
>          }
>          else
>          {
> -            dt_data->prd = dt_data->prd_last;
> +            data->prd = data->prd_last;
>              snprintf(cntbuf, sizeof(cntbuf), "%u-%u ", last_count, ++count);
>          }
>          debugtrace_add_to_buf(cntbuf);

.. in this scope, so perhaps worth latching (and declaring) it only
inside the first "else" above?

> @@ -155,34 +194,70 @@ static void debugtrace_key(unsigned char key)
>      debugtrace_toggle();
>  }
>  
> -static int __init debugtrace_init(void)
> +static void debugtrace_alloc_buffer(struct debugtrace_data **ptr,
> +                                    unsigned int cpu)
>  {
>      int order;
> -    unsigned long kbytes, bytes;
>      struct debugtrace_data *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 || *ptr )
> +        return;
>  
> -    order = get_order_from_bytes(bytes);
> +    order = get_order_from_bytes(debugtrace_bytes);
>      data = alloc_xenheap_pages(order, 0);
>      if ( !data )
> -        return -ENOMEM;
> +    {
> +        if ( debugtrace_per_cpu )
> +            printk("failed to allocate debugtrace buffer for cpu %u\n", cpu);

Could I talk you into using the more common "CPU%u: ..." layout here?

Jan

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