| Message ID | 20190904134608.18425-5-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: debugtrace cleanup and per-cpu buffer support | expand |
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. > +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. > 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? > @@ -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? Jan
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
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 7c72e31032..832797e2e2 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -644,12 +644,13 @@ over the PCI busses sequentially) or by PCI device (must be on segment 0). Limits the number lines printed in Xen stack traces. ### debugtrace -> `= <integer>` +> `= [cpu:]<size>` > Default: `128` -Specify the size of the console debug trace buffer in KiB. The debug -trace feature is only enabled in debugging builds of Xen. +Specify the size of the console debug trace buffer. By specifying `cpu:` +additionally a trace buffer of the specified size is allocated per cpu. +The debug trace feature is only enabled in debugging builds of Xen. ### dma_bits > `= <integer>` diff --git a/xen/common/debugtrace.c b/xen/common/debugtrace.c index 8fa0e7fb0e..449406b9b6 100644 --- a/xen/common/debugtrace.c +++ b/xen/common/debugtrace.c @@ -6,6 +6,7 @@ #include <xen/console.h> +#include <xen/cpu.h> #include <xen/init.h> #include <xen/keyhandler.h> #include <xen/lib.h> @@ -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); + return 0; +} +custom_param("debugtrace", debugtrace_parse_param); + +static void debugtrace_dump_buffer(struct debugtrace_data *data, + const char *which) +{ + if ( !data ) return; - printk("debugtrace_dump() starting\n"); + printk("debugtrace_dump() %s buffer starting\n", which); /* Print oldest portion of the ring. */ - if ( dt_data->buf[dt_data->prd] != '\0' ) - console_serial_puts(&dt_data->buf[dt_data->prd], - dt_data->bytes - dt_data->prd); + if ( data->buf[data->prd] != '\0' ) + console_serial_puts(&data->buf[data->prd], data->bytes - data->prd); /* Print youngest portion of the ring. */ - dt_data->buf[dt_data->prd] = '\0'; - console_serial_puts(&dt_data->buf[0], dt_data->prd); + data->buf[data->prd] = '\0'; + console_serial_puts(&data->buf[0], data->prd); - memset(dt_data->buf, '\0', dt_data->bytes); - dt_data->prd = 0; - dt_data->prd_last = 0; + memset(data->buf, '\0', data->bytes); + data->prd = 0; + data->prd_last = 0; - printk("debugtrace_dump() finished\n"); + printk("debugtrace_dump() %s buffer finished\n", which); +} + +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); + } } static void debugtrace_toggle(void) @@ -91,27 +122,34 @@ void debugtrace_dump(void) static void debugtrace_add_to_buf(char *buf) { + struct debugtrace_data *data; char *p; + data = debugtrace_per_cpu ? this_cpu(dt_cpu_data) : dt_data; + for ( p = buf; *p != '\0'; p++ ) { - dt_data->buf[dt_data->prd++] = *p; - if ( dt_data->prd == dt_data->bytes ) - dt_data->prd = 0; + data->buf[data->prd++] = *p; + if ( data->prd == data->bytes ) + data->prd = 0; } } 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; + if ( !data ) return; debugtrace_used = true; @@ -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); @@ -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); + else + printk("failed to allocate debugtrace buffer\n"); + return; + } - bytes = PAGE_SIZE << order; - memset(data, '\0', bytes); + debugtrace_bytes = PAGE_SIZE << order; + memset(data, '\0', debugtrace_bytes); + data->bytes = debugtrace_bytes - sizeof(*data); - data->bytes = bytes - sizeof(*data); - dt_data = 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. */ + if ( action == CPU_UP_PREPARE ) + debugtrace_alloc_buffer(&per_cpu(dt_cpu_data, cpu), cpu); + + return 0; +} + +static struct notifier_block debugtrace_nfb = { + .notifier_call = debugtrace_cpu_callback +}; + +static int __init debugtrace_init(void) +{ + unsigned long bytes; + unsigned int cpu; + + /* Round size down to next power of two. */ + while ( (bytes = (debugtrace_bytes & (debugtrace_bytes - 1))) != 0 ) + debugtrace_bytes = bytes; register_keyhandler('T', debugtrace_key, "toggle debugtrace to console/buffer", 0); + if ( debugtrace_per_cpu ) + { + for_each_online_cpu ( cpu ) + debugtrace_alloc_buffer(&per_cpu(dt_cpu_data, cpu), cpu); + register_cpu_notifier(&debugtrace_nfb); + } + else + debugtrace_alloc_buffer(&dt_data, 0); + return 0; } __initcall(debugtrace_init);
debugtrace is normally writing trace entries into a single trace buffer. There are cases where this is not optimal, e.g. when hunting a bug which requires writing lots of trace entries and one cpu is stuck. This will result in other cpus filling the trace buffer and finally overwriting the interesting trace entries of the hanging cpu. In order to be able to debug such situations add the capability to use per-cpu trace buffers. This can be selected by specifying the debugtrace boot parameter with the modifier "cpu:", like: debugtrace=cpu:16 At the same time switch the parsing function to accept size modifiers (e.g. 4M or 1G). Printing out the trace entries is done for each buffer in order to minimize the effort needed during printing. As each entry is prefixed with its sequence number sorting the entries can easily be done when analyzing them. Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - only allocate buffer if not already done so V4: - unsigned int -> unsigned long (Jan Beulich) - replace check for bytes < PAGE_SIZE by !bytes (Jan Beulich) - print info which buffer allocation failed (Jan Beulich) - replace switch by if in cpu notifier handler (Jan Beulich) --- docs/misc/xen-command-line.pandoc | 7 +- xen/common/debugtrace.c | 149 ++++++++++++++++++++++++++++---------- 2 files changed, 116 insertions(+), 40 deletions(-)