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

[Juergen Gross <jgross@xxxxxxxx>]

On 03.09.19 12:51, Jan Beulich wrote:
> 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"?
Good catch. :-)

> > +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.
Yes. Will remove one.

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

> 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?
I can do that. Resetting prd to 0 when clearing the buffer is
required here.

> > @@ -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?
With recaclulating debugtrace_bytes this can be dropped.

> > +        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?
Hmm, I'm not sure this is really required. I can add it if you want, but
as a user of debugtrace I'd be only interested in the information
whether I can expect all trace entries to be seen or not.

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


Juergen


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