| Message ID | 20190828080028.18205-4-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: debugtrace cleanup and per-cpu buffer support | expand |
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@suse.com> > --- > 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? > + 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? > + char buf[]; > +}; > + > +static struct debugtrace_data_s *debtr_data; How about s/debtr/dt/ or s/debtr/debugtrace/ ? > +static unsigned int debugtrace_kilobytes = 128; Since you touch this anyway, add __initdata? Maybe also move it next to its integer_param()? > +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? > @@ -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"? Jan
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@suse.com> >> --- >> 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
On 03.09.2019 12:31, Juergen Gross wrote: > On 03.09.19 12:16, Jan Beulich wrote: >> On 28.08.2019 10:00, Juergen Gross wrote: >>> +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. Right, I've meanwhile noticed this. Hence it's fine to keep as is. >>> @@ -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. Actually I wasn't, which I did notice seeing the relevant piece of code getting touched in patch 4: while ( (kbytes = (debugtrace_kilobytes & (debugtrace_kilobytes-1))) != 0 ) debugtrace_kilobytes = kbytes; Jan
On 03.09.19 13:50, Jan Beulich wrote: > On 03.09.2019 12:31, Juergen Gross wrote: >> On 03.09.19 12:16, Jan Beulich wrote: >>> On 28.08.2019 10:00, Juergen Gross wrote: >>>> +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. > > Right, I've meanwhile noticed this. Hence it's fine to keep as is. > >>>> @@ -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. > > Actually I wasn't, which I did notice seeing the relevant piece > of code getting touched in patch 4: > > while ( (kbytes = (debugtrace_kilobytes & (debugtrace_kilobytes-1))) != 0 ) > debugtrace_kilobytes = kbytes; For kbytes < 4 you still were right. Juergen
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 { + unsigned long bytes; /* Size of buffer. */ + unsigned long prd; /* Producer index. */ + char buf[]; +}; + +static struct debugtrace_data_s *debtr_data; + +static unsigned int debugtrace_kilobytes = 128; +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; printk("debugtrace_dump() starting\n"); /* Print oldest portion of the ring. */ - ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); - if ( debugtrace_buf[debugtrace_prd] != '\0' ) - console_serial_puts(&debugtrace_buf[debugtrace_prd], - debugtrace_bytes - debugtrace_prd - 1); + 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); /* Print youngest portion of the ring. */ - debugtrace_buf[debugtrace_prd] = '\0'; - console_serial_puts(&debugtrace_buf[0], debugtrace_prd); + debtr_data->buf[debtr_data->prd] = '\0'; + console_serial_puts(&debtr_data->buf[0], debtr_data->prd); - memset(debugtrace_buf, '\0', debugtrace_bytes); + memset(debtr_data->buf, '\0', debtr_data->bytes); printk("debugtrace_dump() finished\n"); } @@ -66,7 +72,6 @@ static void debugtrace_toggle(void) spin_unlock_irqrestore(&debugtrace_lock, flags); watchdog_enable(); - } void debugtrace_dump(void) @@ -88,10 +93,10 @@ static void debugtrace_add_to_buf(char *buf) for ( p = buf; *p != '\0'; p++ ) { - debugtrace_buf[debugtrace_prd++] = *p; + debtr_data->buf[debtr_data->prd++] = *p; /* Always leave a nul byte at the end of the buffer. */ - if ( debugtrace_prd == (debugtrace_bytes - 1) ) - debugtrace_prd = 0; + if ( debtr_data->prd == (debtr_data->bytes - 1) ) + debtr_data->prd = 0; } } @@ -105,14 +110,14 @@ void debugtrace_printk(const char *fmt, ...) unsigned long flags; unsigned int nr; - if ( debugtrace_bytes == 0 ) + if ( !debtr_data ) return; - debugtrace_used = 1; + debugtrace_used = true; spin_lock_irqsave(&debugtrace_lock, flags); - ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); + ASSERT(debtr_data->buf[debtr_data->bytes - 1] == 0); va_start(args, fmt); nr = vsnprintf(buf, sizeof(buf), fmt, args); @@ -129,14 +134,14 @@ void debugtrace_printk(const char *fmt, ...) { if ( strcmp(buf, last_buf) ) { - last_prd = debugtrace_prd; + last_prd = debtr_data->prd; last_count = ++count; safe_strcpy(last_buf, buf); snprintf(cntbuf, sizeof(cntbuf), "%u ", count); } else { - debugtrace_prd = last_prd; + debtr_data->prd = last_prd; snprintf(cntbuf, sizeof(cntbuf), "%u-%u ", last_count, ++count); } debugtrace_add_to_buf(cntbuf); @@ -154,7 +159,8 @@ static void debugtrace_key(unsigned char key) static int __init debugtrace_init(void) { int order; - unsigned int kbytes, bytes; + 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 ) @@ -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; register_keyhandler('T', debugtrace_key, "toggle debugtrace to console/buffer", 0);
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@suse.com> --- xen/common/debugtrace.c | 62 ++++++++++++++++++++++++++++--------------------- 1 file changed, 35 insertions(+), 27 deletions(-)