[PATCH 0/2] xen: some debug trace enhancements

[PATCH 0/2] xen: some debug trace enhancements

[Juergen Gross]

While doing some scheduler work I used debug trace for diagnosis of
problems during dom0 boot. This small series is the result of adapting
debug trace to my needs.

Juergen Gross (2):
  xen/debug: make debugtrace configurable via Kconfig
  xen/debug: make debugtrace more clever regarding repeating entries

 xen/Kconfig.debug          |  7 +++++++
 xen/drivers/char/console.c | 46 ++++++++++++++++++++++++++++++++++------------
 xen/include/xen/lib.h      |  3 +--
 3 files changed, 42 insertions(+), 14 deletions(-)

-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/2] xen/debug: make debugtrace configurable via Kconfig

[Juergen Gross]

Instead of having to edit include/xen/lib.h for making debugtrace
available make it configurable via Kconfig.

Default is off, it is available only in expert mode or in debug builds.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/Kconfig.debug          | 7 +++++++
 xen/drivers/char/console.c | 2 +-
 xen/include/xen/lib.h      | 3 +--
 3 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug
index 4d5d7f87cb..daacf85141 100644
--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -98,6 +98,13 @@ config UBSAN
 
 	  If unsure, say N here.
 
+config DEBUG_TRACE
+	bool "Debug trace support"
+	---help---
+	  Debug trace enables to record debug trace messages which are printed
+	  either directly to the console or are printed to console in case of
+	  a system crash.
+
 endif # DEBUG || EXPERT
 
 endmenu
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 4315588f05..41ec13ce52 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1157,7 +1157,7 @@ int printk_ratelimit(void)
  * **************************************************************
  */
 
-#ifdef DEBUG_TRACE_DUMP
+#ifdef CONFIG_DEBUG_TRACE
 
 /* Send output direct to console, or buffer it? */
 static volatile int debugtrace_send_to_console;
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 89939f43c8..e0b7bcb6b7 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -86,8 +86,7 @@ int parse_boolean(const char *name, const char *s, const char *e);
  */
 int cmdline_strcmp(const char *frag, const char *name);
 
-/*#define DEBUG_TRACE_DUMP*/
-#ifdef DEBUG_TRACE_DUMP
+#ifdef CONFIG_DEBUG_TRACE
 extern void debugtrace_dump(void);
 extern void debugtrace_printk(const char *fmt, ...)
     __attribute__ ((format (printf, 1, 2)));
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/2] xen/debug: make debugtrace configurable via Kconfig

[Jan Beulich]

>>> On 14.03.19 at 10:37, <jgross@suse.com> wrote:
> Instead of having to edit include/xen/lib.h for making debugtrace
> available make it configurable via Kconfig.
> 
> Default is off, it is available only in expert mode or in debug builds.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/2] xen/debug: make debugtrace more clever regarding repeating entries

[Juergen Gross]

In case debugtrace is writing to memory and the last entry is repeated
don't fill up the trace buffer, but modify the count prefix to "x-y "
style instead.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/drivers/char/console.c | 44 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 41ec13ce52..38dcfc732e 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1225,13 +1225,28 @@ void debugtrace_dump(void)
     watchdog_enable();
 }
 
+static void debugtrace_add_to_buf(char *buf)
+{
+    char *p;
+
+    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) )
+            debugtrace_prd = 0;
+    }
+}
+
 void debugtrace_printk(const char *fmt, ...)
 {
-    static char    buf[1024];
-    static u32 count;
+    static char buf[1024];
+    static char last_buf[1024];
+    static u32 count, last_count;
+    static unsigned int last_prd;
 
+    char          cntbuf[24];
     va_list       args;
-    char         *p;
     unsigned long flags;
 
     if ( debugtrace_bytes == 0 )
@@ -1243,25 +1258,32 @@ void debugtrace_printk(const char *fmt, ...)
 
     ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
 
-    snprintf(buf, sizeof(buf), "%u ", ++count);
-
     va_start(args, fmt);
-    (void)vsnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), fmt, args);
+    (void)vsnprintf(buf, sizeof(buf), fmt, args);
     va_end(args);
 
     if ( debugtrace_send_to_console )
     {
+        snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);
+        serial_puts(sercon_handle, cntbuf);
         serial_puts(sercon_handle, buf);
     }
     else
     {
-        for ( p = buf; *p != '\0'; p++ )
+        if ( strcmp(buf, last_buf) )
+        {
+            last_prd = debugtrace_prd;
+            last_count = ++count;
+            safe_strcpy(last_buf, buf);
+            snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
+        }
+        else
         {
-            debugtrace_buf[debugtrace_prd++] = *p;            
-            /* Always leave a nul byte at the end of the buffer. */
-            if ( debugtrace_prd == (debugtrace_bytes - 1) )
-                debugtrace_prd = 0;
+            debugtrace_prd = last_prd;
+            snprintf(cntbuf, sizeof(cntbuf), "%u-%u ", last_count, ++count);
         }
+        debugtrace_add_to_buf(cntbuf);
+        debugtrace_add_to_buf(buf);
     }
 
     spin_unlock_irqrestore(&debugtrace_lock, flags);
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] xen/debug: make debugtrace more clever regarding repeating entries

[Jan Beulich]

>>> On 14.03.19 at 10:37, <jgross@suse.com> wrote:
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -1225,13 +1225,28 @@ void debugtrace_dump(void)
>      watchdog_enable();
>  }
>  
> +static void debugtrace_add_to_buf(char *buf)
> +{
> +    char *p;
> +
> +    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) )
> +            debugtrace_prd = 0;
> +    }
> +}
> +
>  void debugtrace_printk(const char *fmt, ...)
>  {
> -    static char    buf[1024];
> -    static u32 count;
> +    static char buf[1024];
> +    static char last_buf[1024];
> +    static u32 count, last_count;

Please change to uint32_t or even better simply to unsigned int.

> @@ -1243,25 +1258,32 @@ void debugtrace_printk(const char *fmt, ...)
>  
>      ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
>  
> -    snprintf(buf, sizeof(buf), "%u ", ++count);
> -
>      va_start(args, fmt);
> -    (void)vsnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), fmt, args);
> +    (void)vsnprintf(buf, sizeof(buf), fmt, args);

Please take the opportunity and drop the stray cast.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] xen/debug: make debugtrace more clever regarding repeating entries

[Juergen Gross]

On 14/03/2019 14:33, Jan Beulich wrote:
>>>> On 14.03.19 at 10:37, <jgross@suse.com> wrote:
>> --- a/xen/drivers/char/console.c
>> +++ b/xen/drivers/char/console.c
>> @@ -1225,13 +1225,28 @@ void debugtrace_dump(void)
>>      watchdog_enable();
>>  }
>>  
>> +static void debugtrace_add_to_buf(char *buf)
>> +{
>> +    char *p;
>> +
>> +    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) )
>> +            debugtrace_prd = 0;
>> +    }
>> +}
>> +
>>  void debugtrace_printk(const char *fmt, ...)
>>  {
>> -    static char    buf[1024];
>> -    static u32 count;
>> +    static char buf[1024];
>> +    static char last_buf[1024];
>> +    static u32 count, last_count;
> 
> Please change to uint32_t or even better simply to unsigned int.

Okay.

> 
>> @@ -1243,25 +1258,32 @@ void debugtrace_printk(const char *fmt, ...)
>>  
>>      ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
>>  
>> -    snprintf(buf, sizeof(buf), "%u ", ++count);
>> -
>>      va_start(args, fmt);
>> -    (void)vsnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), fmt, args);
>> +    (void)vsnprintf(buf, sizeof(buf), fmt, args);
> 
> Please take the opportunity and drop the stray cast.

Will do.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] xen/debug: make debugtrace more clever regarding repeating entries

[Andrew Cooper]

On 14/03/2019 13:38, Juergen Gross wrote:
> On 14/03/2019 14:33, Jan Beulich wrote:
>>>>> On 14.03.19 at 10:37, <jgross@suse.com> wrote:
>>> --- a/xen/drivers/char/console.c
>>> +++ b/xen/drivers/char/console.c
>>> @@ -1225,13 +1225,28 @@ void debugtrace_dump(void)
>>>      watchdog_enable();
>>>  }
>>>  
>>> +static void debugtrace_add_to_buf(char *buf)
>>> +{
>>> +    char *p;
>>> +
>>> +    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) )
>>> +            debugtrace_prd = 0;
>>> +    }
>>> +}
>>> +
>>>  void debugtrace_printk(const char *fmt, ...)
>>>  {
>>> -    static char    buf[1024];
>>> -    static u32 count;
>>> +    static char buf[1024];
>>> +    static char last_buf[1024];
>>> +    static u32 count, last_count;
>> Please change to uint32_t or even better simply to unsigned int.
> Okay.
>
>>> @@ -1243,25 +1258,32 @@ void debugtrace_printk(const char *fmt, ...)
>>>  
>>>      ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
>>>  
>>> -    snprintf(buf, sizeof(buf), "%u ", ++count);
>>> -
>>>      va_start(args, fmt);
>>> -    (void)vsnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), fmt, args);
>>> +    (void)vsnprintf(buf, sizeof(buf), fmt, args);
>> Please take the opportunity and drop the stray cast.
> Will do.

Both can be done on commit, surely?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] xen/debug: make debugtrace more clever regarding repeating entries

[Jan Beulich]

>>> On 14.03.19 at 14:40, <andrew.cooper3@citrix.com> wrote:
> On 14/03/2019 13:38, Juergen Gross wrote:
>> On 14/03/2019 14:33, Jan Beulich wrote:
>>>>>> On 14.03.19 at 10:37, <jgross@suse.com> wrote:
>>>> --- a/xen/drivers/char/console.c
>>>> +++ b/xen/drivers/char/console.c
>>>> @@ -1225,13 +1225,28 @@ void debugtrace_dump(void)
>>>>      watchdog_enable();
>>>>  }
>>>>  
>>>> +static void debugtrace_add_to_buf(char *buf)
>>>> +{
>>>> +    char *p;
>>>> +
>>>> +    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) )
>>>> +            debugtrace_prd = 0;
>>>> +    }
>>>> +}
>>>> +
>>>>  void debugtrace_printk(const char *fmt, ...)
>>>>  {
>>>> -    static char    buf[1024];
>>>> -    static u32 count;
>>>> +    static char buf[1024];
>>>> +    static char last_buf[1024];
>>>> +    static u32 count, last_count;
>>> Please change to uint32_t or even better simply to unsigned int.
>> Okay.
>>
>>>> @@ -1243,25 +1258,32 @@ void debugtrace_printk(const char *fmt, ...)
>>>>  
>>>>      ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
>>>>  
>>>> -    snprintf(buf, sizeof(buf), "%u ", ++count);
>>>> -
>>>>      va_start(args, fmt);
>>>> -    (void)vsnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), fmt, args);
>>>> +    (void)vsnprintf(buf, sizeof(buf), fmt, args);
>>> Please take the opportunity and drop the stray cast.
>> Will do.
> 
> Both can be done on commit, surely?

Perhaps, albeit iirc the first would amount to more than just s/u32/.../
on the line in question.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] xen/debug: make debugtrace more clever regarding repeating entries

[Juergen Gross]

On 14/03/2019 15:14, Jan Beulich wrote:
>>>> On 14.03.19 at 14:40, <andrew.cooper3@citrix.com> wrote:
>> On 14/03/2019 13:38, Juergen Gross wrote:
>>> On 14/03/2019 14:33, Jan Beulich wrote:
>>>>>>> On 14.03.19 at 10:37, <jgross@suse.com> wrote:
>>>>> --- a/xen/drivers/char/console.c
>>>>> +++ b/xen/drivers/char/console.c
>>>>> @@ -1225,13 +1225,28 @@ void debugtrace_dump(void)
>>>>>      watchdog_enable();
>>>>>  }
>>>>>  
>>>>> +static void debugtrace_add_to_buf(char *buf)
>>>>> +{
>>>>> +    char *p;
>>>>> +
>>>>> +    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) )
>>>>> +            debugtrace_prd = 0;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>  void debugtrace_printk(const char *fmt, ...)
>>>>>  {
>>>>> -    static char    buf[1024];
>>>>> -    static u32 count;
>>>>> +    static char buf[1024];
>>>>> +    static char last_buf[1024];
>>>>> +    static u32 count, last_count;
>>>> Please change to uint32_t or even better simply to unsigned int.
>>> Okay.
>>>
>>>>> @@ -1243,25 +1258,32 @@ void debugtrace_printk(const char *fmt, ...)
>>>>>  
>>>>>      ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
>>>>>  
>>>>> -    snprintf(buf, sizeof(buf), "%u ", ++count);
>>>>> -
>>>>>      va_start(args, fmt);
>>>>> -    (void)vsnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), fmt, args);
>>>>> +    (void)vsnprintf(buf, sizeof(buf), fmt, args);
>>>> Please take the opportunity and drop the stray cast.
>>> Will do.
>>
>> Both can be done on commit, surely?
> 
> Perhaps, albeit iirc the first would amount to more than just s/u32/.../
> on the line in question.

What else would you want to do?


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] xen/debug: make debugtrace more clever regarding repeating entries

[Jan Beulich]

>>> On 14.03.19 at 15:28, <jgross@suse.com> wrote:
> On 14/03/2019 15:14, Jan Beulich wrote:
>>>>> On 14.03.19 at 14:40, <andrew.cooper3@citrix.com> wrote:
>>> On 14/03/2019 13:38, Juergen Gross wrote:
>>>> On 14/03/2019 14:33, Jan Beulich wrote:
>>>>>>>> On 14.03.19 at 10:37, <jgross@suse.com> wrote:
>>>>>> --- a/xen/drivers/char/console.c
>>>>>> +++ b/xen/drivers/char/console.c
>>>>>> @@ -1225,13 +1225,28 @@ void debugtrace_dump(void)
>>>>>>      watchdog_enable();
>>>>>>  }
>>>>>>  
>>>>>> +static void debugtrace_add_to_buf(char *buf)
>>>>>> +{
>>>>>> +    char *p;
>>>>>> +
>>>>>> +    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) )
>>>>>> +            debugtrace_prd = 0;
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>>  void debugtrace_printk(const char *fmt, ...)
>>>>>>  {
>>>>>> -    static char    buf[1024];
>>>>>> -    static u32 count;
>>>>>> +    static char buf[1024];
>>>>>> +    static char last_buf[1024];
>>>>>> +    static u32 count, last_count;
>>>>> Please change to uint32_t or even better simply to unsigned int.
>>>> Okay.
>>>>
>>>>>> @@ -1243,25 +1258,32 @@ void debugtrace_printk(const char *fmt, ...)
>>>>>>  
>>>>>>      ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
>>>>>>  
>>>>>> -    snprintf(buf, sizeof(buf), "%u ", ++count);
>>>>>> -
>>>>>>      va_start(args, fmt);
>>>>>> -    (void)vsnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), fmt, args);
>>>>>> +    (void)vsnprintf(buf, sizeof(buf), fmt, args);
>>>>> Please take the opportunity and drop the stray cast.
>>>> Will do.
>>>
>>> Both can be done on commit, surely?
>> 
>> Perhaps, albeit iirc the first would amount to more than just s/u32/.../
>> on the line in question.
> 
> What else would you want to do?

Fold it with the adjacent unsigned int declaration (iirc there was one).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/2] xen/debug: make debugtrace more clever regarding repeating entries

[Juergen Gross]

On 14/03/2019 15:31, Jan Beulich wrote:
>>>> On 14.03.19 at 15:28, <jgross@suse.com> wrote:
>> On 14/03/2019 15:14, Jan Beulich wrote:
>>>>>> On 14.03.19 at 14:40, <andrew.cooper3@citrix.com> wrote:
>>>> On 14/03/2019 13:38, Juergen Gross wrote:
>>>>> On 14/03/2019 14:33, Jan Beulich wrote:
>>>>>>>>> On 14.03.19 at 10:37, <jgross@suse.com> wrote:
>>>>>>> --- a/xen/drivers/char/console.c
>>>>>>> +++ b/xen/drivers/char/console.c
>>>>>>> @@ -1225,13 +1225,28 @@ void debugtrace_dump(void)
>>>>>>>      watchdog_enable();
>>>>>>>  }
>>>>>>>  
>>>>>>> +static void debugtrace_add_to_buf(char *buf)
>>>>>>> +{
>>>>>>> +    char *p;
>>>>>>> +
>>>>>>> +    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) )
>>>>>>> +            debugtrace_prd = 0;
>>>>>>> +    }
>>>>>>> +}
>>>>>>> +
>>>>>>>  void debugtrace_printk(const char *fmt, ...)
>>>>>>>  {
>>>>>>> -    static char    buf[1024];
>>>>>>> -    static u32 count;
>>>>>>> +    static char buf[1024];
>>>>>>> +    static char last_buf[1024];
>>>>>>> +    static u32 count, last_count;
>>>>>> Please change to uint32_t or even better simply to unsigned int.
>>>>> Okay.
>>>>>
>>>>>>> @@ -1243,25 +1258,32 @@ void debugtrace_printk(const char *fmt, ...)
>>>>>>>  
>>>>>>>      ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
>>>>>>>  
>>>>>>> -    snprintf(buf, sizeof(buf), "%u ", ++count);
>>>>>>> -
>>>>>>>      va_start(args, fmt);
>>>>>>> -    (void)vsnprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), fmt, args);
>>>>>>> +    (void)vsnprintf(buf, sizeof(buf), fmt, args);
>>>>>> Please take the opportunity and drop the stray cast.
>>>>> Will do.
>>>>
>>>> Both can be done on commit, surely?
>>>
>>> Perhaps, albeit iirc the first would amount to more than just s/u32/.../
>>> on the line in question.
>>
>> What else would you want to do?
> 
> Fold it with the adjacent unsigned int declaration (iirc there was one).

I can post a V3 if you want including the folding.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel