| Message ID | 20190905113955.24870-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: debugtrace cleanup and per-cpu buffer support | expand |
On 05.09.2019 13:39, Juergen Gross wrote: > After dumping the debugtrace buffer it is cleared. This results in some > entries not being printed in case the buffer is dumped again before > having wrapped. > > While at it remove the trailing zero byte in the buffer as it is no > longer needed. Commit b5e6e1ee8da59f introduced passing the number of > chars to be printed in the related interfaces, so the trailing 0 byte > is no longer required. > > Signed-off-by: Juergen Gross <jgross@suse.com> Technically this is fine, so it can have my Reviewed-by: Jan Beulich <jbeulich@suse.com> However, ... > @@ -1173,6 +1175,7 @@ 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; > +static char debugtrace_last_entry_buf[DEBUG_TRACE_ENTRY_SIZE]; ... this is what I was afraid would happen, but I admit I didn't reply in a way previously indicating that I dislike such a solution. This is also why, when noticing the issue, I didn't put together a patch myself right away. In particular I'm of the opinion that the three last_* values would better all stay together, and then would better stay inside the only function using them. > @@ -1279,11 +1280,11 @@ void debugtrace_printk(const char *fmt, ...) > } > else > { > - if ( strcmp(buf, last_buf) ) > + if ( strcmp(buf, debugtrace_last_entry_buf) ) Wouldn't moving count to file scope and latching its value into a new dump_count when dumping work: if ( count == dump_count || strcmp(buf, last_buf) ) work? Jan
On 05.09.19 14:17, Jan Beulich wrote: > On 05.09.2019 13:39, Juergen Gross wrote: >> After dumping the debugtrace buffer it is cleared. This results in some >> entries not being printed in case the buffer is dumped again before >> having wrapped. >> >> While at it remove the trailing zero byte in the buffer as it is no >> longer needed. Commit b5e6e1ee8da59f introduced passing the number of >> chars to be printed in the related interfaces, so the trailing 0 byte >> is no longer required. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > Technically this is fine, so it can have my > Reviewed-by: Jan Beulich <jbeulich@suse.com> > However, ... > >> @@ -1173,6 +1175,7 @@ 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; >> +static char debugtrace_last_entry_buf[DEBUG_TRACE_ENTRY_SIZE]; > > ... this is what I was afraid would happen, but I admit I didn't > reply in a way previously indicating that I dislike such a > solution. This is also why, when noticing the issue, I didn't put > together a patch myself right away. In particular I'm of the > opinion that the three last_* values would better all stay > together, and then would better stay inside the only function > using them. > >> @@ -1279,11 +1280,11 @@ void debugtrace_printk(const char *fmt, ...) >> } >> else >> { >> - if ( strcmp(buf, last_buf) ) >> + if ( strcmp(buf, debugtrace_last_entry_buf) ) > > Wouldn't moving count to file scope and latching its value into > a new dump_count when dumping work: > > if ( count == dump_count || strcmp(buf, last_buf) ) > > work? I'd rather have a bool which will be reset in above condition. This will avoid problems when count is wrapping. Juergen
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index f49c6f29a8..8df627c84a 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -1166,6 +1166,8 @@ int printk_ratelimit(void) #ifdef CONFIG_DEBUG_TRACE +#define DEBUG_TRACE_ENTRY_SIZE 1024 + /* Send output direct to console, or buffer it? */ static volatile int debugtrace_send_to_console; @@ -1173,6 +1175,7 @@ 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; +static char debugtrace_last_entry_buf[DEBUG_TRACE_ENTRY_SIZE]; static DEFINE_SPINLOCK(debugtrace_lock); integer_param("debugtrace", debugtrace_kilobytes); @@ -1184,16 +1187,17 @@ static void debugtrace_dump_worker(void) 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); + debugtrace_bytes - debugtrace_prd); /* Print youngest portion of the ring. */ debugtrace_buf[debugtrace_prd] = '\0'; console_serial_puts(&debugtrace_buf[0], debugtrace_prd); memset(debugtrace_buf, '\0', debugtrace_bytes); + debugtrace_prd = 0; + debugtrace_last_entry_buf[0] = 0; printk("debugtrace_dump() finished\n"); } @@ -1241,15 +1245,14 @@ static void debugtrace_add_to_buf(char *buf) for ( p = buf; *p != '\0'; p++ ) { debugtrace_buf[debugtrace_prd++] = *p; - /* Always leave a nul byte at the end of the buffer. */ - if ( debugtrace_prd == (debugtrace_bytes - 1) ) + if ( debugtrace_prd == debugtrace_bytes ) debugtrace_prd = 0; } } void debugtrace_printk(const char *fmt, ...) { - static char buf[1024], last_buf[1024]; + static char buf[DEBUG_TRACE_ENTRY_SIZE]; static unsigned int count, last_count, last_prd; char cntbuf[24]; @@ -1264,8 +1267,6 @@ void debugtrace_printk(const char *fmt, ...) spin_lock_irqsave(&debugtrace_lock, flags); - ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); - va_start(args, fmt); nr = vscnprintf(buf, sizeof(buf), fmt, args); va_end(args); @@ -1279,11 +1280,11 @@ void debugtrace_printk(const char *fmt, ...) } else { - if ( strcmp(buf, last_buf) ) + if ( strcmp(buf, debugtrace_last_entry_buf) ) { last_prd = debugtrace_prd; last_count = ++count; - safe_strcpy(last_buf, buf); + safe_strcpy(debugtrace_last_entry_buf, buf); snprintf(cntbuf, sizeof(cntbuf), "%u ", count); } else
After dumping the debugtrace buffer it is cleared. This results in some entries not being printed in case the buffer is dumped again before having wrapped. While at it remove the trailing zero byte in the buffer as it is no longer needed. Commit b5e6e1ee8da59f introduced passing the number of chars to be printed in the related interfaces, so the trailing 0 byte is no longer required. Signed-off-by: Juergen Gross <jgross@suse.com> --- V5: - invalidate last_buf instead of last_prd after printing the buffer (Jan Beulich) --- xen/drivers/char/console.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)