[v4,1/4] xen: fix debugtrace clearing

Commit Message

Jürgen Groß Sept. 4, 2019, 1:46 p.m. UTC

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 <[e-mail]>
---
 xen/drivers/char/console.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Comments

Jan Beulich Sept. 4, 2019, 1:53 p.m. UTC | #1

On 04.09.2019 15:46, Juergen Gross wrote:
> @@ -1281,14 +1280,14 @@ void debugtrace_printk(const char *fmt, ...)
>      {
>          if ( strcmp(buf, last_buf) )
>          {
> -            last_prd = debugtrace_prd;
> +            debugtrace_prd_last = debugtrace_prd;
>              last_count = ++count;
>              safe_strcpy(last_buf, buf);
>              snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
>          }
>          else
>          {
> -            debugtrace_prd = last_prd;
> +            debugtrace_prd = debugtrace_prd_last;
>              snprintf(cntbuf, sizeof(cntbuf), "%u-%u ", last_count, ++count);
>          }
>          debugtrace_add_to_buf(cntbuf);

I'm afraid it is rather last_buf[] which needs invalidating, at
which point last_prd could imo remain local to this function.

Jan

Jürgen Groß Sept. 4, 2019, 2:25 p.m. UTC | #2

On 04.09.19 15:53, Jan Beulich wrote:
> On 04.09.2019 15:46, Juergen Gross wrote:
>> @@ -1281,14 +1280,14 @@ void debugtrace_printk(const char *fmt, ...)
>>       {
>>           if ( strcmp(buf, last_buf) )
>>           {
>> -            last_prd = debugtrace_prd;
>> +            debugtrace_prd_last = debugtrace_prd;
>>               last_count = ++count;
>>               safe_strcpy(last_buf, buf);
>>               snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
>>           }
>>           else
>>           {
>> -            debugtrace_prd = last_prd;
>> +            debugtrace_prd = debugtrace_prd_last;
>>               snprintf(cntbuf, sizeof(cntbuf), "%u-%u ", last_count, ++count);
>>           }
>>           debugtrace_add_to_buf(cntbuf);
> 
> I'm afraid it is rather last_buf[] which needs invalidating, at
> which point last_prd could imo remain local to this function.

Hmm, right. Will change.

I'll send a new series as soon as you indicate you won't have further
comments to any patch of the series.


Juergen

Patch

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index f49c6f29a8..62477a9728 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1171,6 +1171,7 @@  static volatile int debugtrace_send_to_console;
 
 static char        *debugtrace_buf; /* Debug-trace buffer */
 static unsigned int debugtrace_prd; /* Producer index     */
+static unsigned int debugtrace_prd_last;
 static unsigned int debugtrace_kilobytes = 128, debugtrace_bytes;
 static unsigned int debugtrace_used;
 static DEFINE_SPINLOCK(debugtrace_lock);
@@ -1184,16 +1185,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_prd_last = 0;
 
     printk("debugtrace_dump() finished\n");
 }
@@ -1241,8 +1243,7 @@  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;
     }
 }
@@ -1250,7 +1251,7 @@  static void debugtrace_add_to_buf(char *buf)
 void debugtrace_printk(const char *fmt, ...)
 {
     static char buf[1024], last_buf[1024];
-    static unsigned int count, last_count, last_prd;
+    static unsigned int count, last_count;
 
     char          cntbuf[24];
     va_list       args;
@@ -1264,8 +1265,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);
@@ -1281,14 +1280,14 @@  void debugtrace_printk(const char *fmt, ...)
     {
         if ( strcmp(buf, last_buf) )
         {
-            last_prd = debugtrace_prd;
+            debugtrace_prd_last = debugtrace_prd;
             last_count = ++count;
             safe_strcpy(last_buf, buf);
             snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
         }
         else
         {
-            debugtrace_prd = last_prd;
+            debugtrace_prd = debugtrace_prd_last;
             snprintf(cntbuf, sizeof(cntbuf), "%u-%u ", last_count, ++count);
         }
         debugtrace_add_to_buf(cntbuf);