Re: [Xen-devel] [PATCH v3 3/4] xen: refactor debugtrace data

[Juergen Gross <jgross@xxxxxxxx>]

On 03.09.19 12:16, Jan Beulich wrote:
> On 28.08.2019 10:00, Juergen Gross wrote:
> > As a preparation for per-cpu buffers do a little refactoring of the
> > debugtrace data: put the needed buffer admin data into the buffer as
> > it will be needed for each buffer.
> >
> > While at it switch debugtrace_send_to_console and debugtrace_used to
> > bool and delete an empty line.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> >   xen/common/debugtrace.c | 62 ++++++++++++++++++++++++++++---------------------
> >   1 file changed, 35 insertions(+), 27 deletions(-)
> >
> > diff --git a/xen/common/debugtrace.c b/xen/common/debugtrace.c
> > index a2fab0d72c..7a9e4fb99f 100644
> > --- a/xen/common/debugtrace.c
> > +++ b/xen/common/debugtrace.c
> > @@ -15,33 +15,39 @@
> >   #include <xen/watchdog.h>
> >   
> >   /* Send output direct to console, or buffer it? */
> > -static volatile int debugtrace_send_to_console;
> > +static volatile bool debugtrace_send_to_console;
> >   
> > -static char        *debugtrace_buf; /* Debug-trace buffer */
> > -static unsigned int debugtrace_prd; /* Producer index     */
> > -static unsigned int debugtrace_kilobytes = 128, debugtrace_bytes;
> > -static unsigned int debugtrace_used;
> > +struct debugtrace_data_s {
> Is the _s suffix really needed for some reason?
No, not really. I can drop it.

> > +    unsigned long bytes; /* Size of buffer. */
> > +    unsigned long prd;   /* Producer index. */
> Seeing that you switch from int to long here - are you really
> fancying these buffers to go beyond 4GiB in size?
I didn't want to rule it out on a multi-TB machine. :-)

> > +    char          buf[];
> > +};
> > +
> > +static struct debugtrace_data_s *debtr_data;
> How about s/debtr/dt/ or s/debtr/debugtrace/ ?
Yes, dt seems to be fine.

> > +static unsigned int debugtrace_kilobytes = 128;
> Since you touch this anyway, add __initdata? Maybe also move it
> next to its integer_param()?
This is modified in patch 4 again, and there I need it in the running
system for support of cpu hotplug with per-cpu buffers.

> > +static bool debugtrace_used;
> >   static DEFINE_SPINLOCK(debugtrace_lock);
> >   integer_param("debugtrace", debugtrace_kilobytes);
> >   
> >   static void debugtrace_dump_worker(void)
> >   {
> > -    if ( (debugtrace_bytes == 0) || !debugtrace_used )
> > +    if ( !debtr_data || !debugtrace_used )
> >           return;
> Why don't you get rid of the left side of the || altogether?
In patch 4 I do. :-)

I can drop it here already.

> > @@ -165,12 +171,14 @@ static int __init debugtrace_init(void)
> >           return 0;
> >   
> >       order = get_order_from_bytes(bytes);
> > -    debugtrace_buf = alloc_xenheap_pages(order, 0);
> > -    ASSERT(debugtrace_buf != NULL);
> > +    data = alloc_xenheap_pages(order, 0);
> > +    if ( !data )
> > +        return -ENOMEM;
> >   
> > -    memset(debugtrace_buf, '\0', bytes);
> > +    memset(data, '\0', bytes);
> >   
> > -    debugtrace_bytes = bytes;
> > +    data->bytes = bytes - sizeof(*data);
> > +    debtr_data = data;
> The allocation may end up being almost twice as big as what gets
> actually used this way. Wouldn't it make sense to re-calculate
> "bytes" from "order"?
Yes, you are right.


Juergen

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