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

[Juergen Gross <jgross@xxxxxxxx>]

On 03.09.19 14:01, Jan Beulich wrote:
> On 03.09.2019 13:10, Juergen Gross wrote:
> > On 03.09.19 12:51, Jan Beulich wrote:
> > > On 28.08.2019 10:00, Juergen Gross wrote:
> > > > +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.
> I'm afraid it's not this simple: Doing so will confuse
> debugtrace_printk() - consider the function then storing the
> previously latched last_prd into debugtrace_prd.
Sure, this has to be handled (and it is still easy to do so).

> > > > -    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.
> Well, if the allocation fails for a CPU, it's not impossible for
> the CPU bringup to then also fail. Subsequent to this the system
> would then still provide an almost complete set of debugtrace
> entries (ones issued by subsequent bringup actions would be
> missing), _despite_ this log message.
You seem to want the information, so I can add it.


Juergen

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