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

[Juergen Gross <jgross@xxxxxxxx>]

On 05.09.19 12:28, Jan Beulich wrote:
> 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.
NP to change that.

> > +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.
Okay.

> >   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?
Fine with me.

> > @@ -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?
Sure.


Juergen

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