[v6,1/5] xen: fix debugtrace clearing

Commit Message

Jürgen Groß Sept. 9, 2019, 7:35 a.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]>
Reviewed-by: Jan Beulich <[e-mail]>
---
V5:
- invalidate last_buf instead of last_prd after printing the buffer
  (Jan Beulich)
V6:
- instead of moving last_buf to file scope use a boolean for buffer
  invalidation (Jan Beulich)
---
 xen/drivers/char/console.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Jan Beulich Sept. 9, 2019, 7:53 a.m. UTC | #1

On 09.09.2019 09:35, Juergen Gross wrote:
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -1173,6 +1173,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 bool debugtrace_buf_empty;

Wouldn't it be more logical for this to start out as "true"?
And I'm debating with myself whether it might want to be
__read_mostly. Both could be adjusted (if agreement can be
reached) while committing.

Jan

Jan Beulich Sept. 9, 2019, 8:11 a.m. UTC | #2

On 09.09.2019 09:53, Jan Beulich wrote:
> On 09.09.2019 09:35, Juergen Gross wrote:
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -1173,6 +1173,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 bool debugtrace_buf_empty;
> 
> Wouldn't it be more logical for this to start out as "true"?
> And I'm debating with myself whether it might want to be
> __read_mostly.

Hmm, no, the latter rather not.

Jan

Jürgen Groß Sept. 9, 2019, 8:19 a.m. UTC | #3

On 09.09.19 09:53, Jan Beulich wrote:
> On 09.09.2019 09:35, Juergen Gross wrote:
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -1173,6 +1173,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 bool debugtrace_buf_empty;
> 
> Wouldn't it be more logical for this to start out as "true"?

In the end it doesn't matter, as last_buf[] will be empty initially.
But I can change it to let its semantics match the reality.


Juergen

Patch

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index f49c6f29a8..11e1db5684 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1173,6 +1173,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 bool debugtrace_buf_empty;
 static DEFINE_SPINLOCK(debugtrace_lock);
 integer_param("debugtrace", debugtrace_kilobytes);
 
@@ -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_buf_empty = true;
 
     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;
     }
 }
@@ -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);
@@ -1279,8 +1278,9 @@  void debugtrace_printk(const char *fmt, ...)
     }
     else
     {
-        if ( strcmp(buf, last_buf) )
+        if ( debugtrace_buf_empty || strcmp(buf, last_buf) )
         {
+            debugtrace_buf_empty = false;
             last_prd = debugtrace_prd;
             last_count = ++count;
             safe_strcpy(last_buf, buf);