[Xen-devel] [PATCH v5 0/4] xen: debugtrace cleanup and per-cpu buffer support

[Xen-devel] [PATCH v5 0/4] xen: debugtrace cleanup and per-cpu buffer support

[Juergen Gross]

Another debugtrace enhancement I needed for core scheduling debugging,
plus some cleanup.

Changes in V5:
- several comments by Jan addressed (code: patches 1 and 4, commit
  message of patch 3)

Changes in V4:
- replaced patch 1 (original one was committed, new one requested by
  Jan Beulich)
- several comments by Jan addressed

Changes in V3:
- rebase to current staging

Changes in V2:
- added new patch 1 (preparing the move of debugtrace coding)
- patch 4 (v1 patch 3): avoid leaking buffer

Juergen Gross (4):
  xen: fix debugtrace clearing
  xen: move debugtrace coding to common/debugtrace.c
  xen: refactor debugtrace data
  xen: add per-cpu buffer option to debugtrace

 docs/misc/xen-command-line.pandoc |   7 +-
 xen/common/Makefile               |   1 +
 xen/common/debugtrace.c           | 276 ++++++++++++++++++++++++++++++++++++++
 xen/drivers/char/console.c        | 178 +-----------------------
 4 files changed, 282 insertions(+), 180 deletions(-)
 create mode 100644 xen/common/debugtrace.c

-- 
2.16.4


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

[Xen-devel] [PATCH v5 1/4] xen: fix debugtrace clearing

[Juergen Gross]

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(-)

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
-- 
2.16.4


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

[Xen-devel] [PATCH v5 2/4] xen: move debugtrace coding to common/debugtrace.c

[Juergen Gross]

Instead of living in drivers/char/console.c move the debugtrace
related coding to a new file common/debugtrace.c

No functional change, code movement only.

Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 xen/common/Makefile        |   1 +
 xen/common/debugtrace.c    | 181 +++++++++++++++++++++++++++++++++++++++++++++
 xen/drivers/char/console.c | 179 +-------------------------------------------
 3 files changed, 183 insertions(+), 178 deletions(-)
 create mode 100644 xen/common/debugtrace.c

diff --git a/xen/common/Makefile b/xen/common/Makefile
index eddda5daa6..62b34e69e9 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -4,6 +4,7 @@ obj-y += bsearch.o
 obj-$(CONFIG_CORE_PARKING) += core_parking.o
 obj-y += cpu.o
 obj-y += cpupool.o
+obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o
 obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
 obj-y += domctl.o
 obj-y += domain.o
diff --git a/xen/common/debugtrace.c b/xen/common/debugtrace.c
new file mode 100644
index 0000000000..c1ee3f45b9
--- /dev/null
+++ b/xen/common/debugtrace.c
@@ -0,0 +1,181 @@
+/******************************************************************************
+ * debugtrace.c
+ *
+ * Debugtrace for Xen
+ */
+
+
+#include <xen/console.h>
+#include <xen/init.h>
+#include <xen/keyhandler.h>
+#include <xen/lib.h>
+#include <xen/mm.h>
+#include <xen/serial.h>
+#include <xen/spinlock.h>
+#include <xen/watchdog.h>
+
+#define DEBUG_TRACE_ENTRY_SIZE   1024
+
+/* Send output direct to console, or buffer it? */
+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_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);
+
+static void debugtrace_dump_worker(void)
+{
+    if ( (debugtrace_bytes == 0) || !debugtrace_used )
+        return;
+
+    printk("debugtrace_dump() starting\n");
+
+    /* Print oldest portion of the ring. */
+    if ( debugtrace_buf[debugtrace_prd] != '\0' )
+        console_serial_puts(&debugtrace_buf[debugtrace_prd],
+                            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");
+}
+
+static void debugtrace_toggle(void)
+{
+    unsigned long flags;
+
+    watchdog_disable();
+    spin_lock_irqsave(&debugtrace_lock, flags);
+
+    /*
+     * Dump the buffer *before* toggling, in case the act of dumping the
+     * buffer itself causes more printk() invocations.
+     */
+    printk("debugtrace_printk now writing to %s.\n",
+           !debugtrace_send_to_console ? "console": "buffer");
+    if ( !debugtrace_send_to_console )
+        debugtrace_dump_worker();
+
+    debugtrace_send_to_console = !debugtrace_send_to_console;
+
+    spin_unlock_irqrestore(&debugtrace_lock, flags);
+    watchdog_enable();
+
+}
+
+void debugtrace_dump(void)
+{
+    unsigned long flags;
+
+    watchdog_disable();
+    spin_lock_irqsave(&debugtrace_lock, flags);
+
+    debugtrace_dump_worker();
+
+    spin_unlock_irqrestore(&debugtrace_lock, flags);
+    watchdog_enable();
+}
+
+static void debugtrace_add_to_buf(char *buf)
+{
+    char *p;
+
+    for ( p = buf; *p != '\0'; p++ )
+    {
+        debugtrace_buf[debugtrace_prd++] = *p;
+        if ( debugtrace_prd == debugtrace_bytes )
+            debugtrace_prd = 0;
+    }
+}
+
+void debugtrace_printk(const char *fmt, ...)
+{
+    static char buf[DEBUG_TRACE_ENTRY_SIZE];
+    static unsigned int count, last_count, last_prd;
+
+    char          cntbuf[24];
+    va_list       args;
+    unsigned long flags;
+    unsigned int nr;
+
+    if ( debugtrace_bytes == 0 )
+        return;
+
+    debugtrace_used = 1;
+
+    spin_lock_irqsave(&debugtrace_lock, flags);
+
+    va_start(args, fmt);
+    nr = vsnprintf(buf, sizeof(buf), fmt, args);
+    va_end(args);
+
+    if ( debugtrace_send_to_console )
+    {
+        unsigned int n = scnprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);
+
+        console_serial_puts(cntbuf, n);
+        console_serial_puts(buf, nr);
+    }
+    else
+    {
+        if ( strcmp(buf, debugtrace_last_entry_buf) )
+        {
+            last_prd = debugtrace_prd;
+            last_count = ++count;
+            safe_strcpy(debugtrace_last_entry_buf, buf);
+            snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
+        }
+        else
+        {
+            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);
+}
+
+static void debugtrace_key(unsigned char key)
+{
+    debugtrace_toggle();
+}
+
+static int __init debugtrace_init(void)
+{
+    int order;
+    unsigned int kbytes, bytes;
+
+    /* 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;
+
+    order = get_order_from_bytes(bytes);
+    debugtrace_buf = alloc_xenheap_pages(order, 0);
+    ASSERT(debugtrace_buf != NULL);
+
+    memset(debugtrace_buf, '\0', bytes);
+
+    debugtrace_bytes = bytes;
+
+    register_keyhandler('T', debugtrace_key,
+                        "toggle debugtrace to console/buffer", 0);
+
+    return 0;
+}
+__initcall(debugtrace_init);
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 8df627c84a..7f29190eaf 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1160,184 +1160,7 @@ int printk_ratelimit(void)
 
 /*
  * **************************************************************
- * *************** Serial console ring buffer *******************
- * **************************************************************
- */
-
-#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;
-
-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);
-
-static void debugtrace_dump_worker(void)
-{
-    if ( (debugtrace_bytes == 0) || !debugtrace_used )
-        return;
-
-    printk("debugtrace_dump() starting\n");
-
-    /* Print oldest portion of the ring. */
-    if ( debugtrace_buf[debugtrace_prd] != '\0' )
-        console_serial_puts(&debugtrace_buf[debugtrace_prd],
-                            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");
-}
-
-static void debugtrace_toggle(void)
-{
-    unsigned long flags;
-
-    watchdog_disable();
-    spin_lock_irqsave(&debugtrace_lock, flags);
-
-    /*
-     * Dump the buffer *before* toggling, in case the act of dumping the
-     * buffer itself causes more printk() invocations.
-     */
-    printk("debugtrace_printk now writing to %s.\n",
-           !debugtrace_send_to_console ? "console": "buffer");
-    if ( !debugtrace_send_to_console )
-        debugtrace_dump_worker();
-
-    debugtrace_send_to_console = !debugtrace_send_to_console;
-
-    spin_unlock_irqrestore(&debugtrace_lock, flags);
-    watchdog_enable();
-
-}
-
-void debugtrace_dump(void)
-{
-    unsigned long flags;
-
-    watchdog_disable();
-    spin_lock_irqsave(&debugtrace_lock, flags);
-
-    debugtrace_dump_worker();
-
-    spin_unlock_irqrestore(&debugtrace_lock, flags);
-    watchdog_enable();
-}
-
-static void debugtrace_add_to_buf(char *buf)
-{
-    char *p;
-
-    for ( p = buf; *p != '\0'; p++ )
-    {
-        debugtrace_buf[debugtrace_prd++] = *p;
-        if ( debugtrace_prd == debugtrace_bytes )
-            debugtrace_prd = 0;
-    }
-}
-
-void debugtrace_printk(const char *fmt, ...)
-{
-    static char buf[DEBUG_TRACE_ENTRY_SIZE];
-    static unsigned int count, last_count, last_prd;
-
-    char          cntbuf[24];
-    va_list       args;
-    unsigned long flags;
-    unsigned int nr;
-
-    if ( debugtrace_bytes == 0 )
-        return;
-
-    debugtrace_used = 1;
-
-    spin_lock_irqsave(&debugtrace_lock, flags);
-
-    va_start(args, fmt);
-    nr = vscnprintf(buf, sizeof(buf), fmt, args);
-    va_end(args);
-
-    if ( debugtrace_send_to_console )
-    {
-        unsigned int n = scnprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);
-
-        console_serial_puts(cntbuf, n);
-        console_serial_puts(buf, nr);
-    }
-    else
-    {
-        if ( strcmp(buf, debugtrace_last_entry_buf) )
-        {
-            last_prd = debugtrace_prd;
-            last_count = ++count;
-            safe_strcpy(debugtrace_last_entry_buf, buf);
-            snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
-        }
-        else
-        {
-            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);
-}
-
-static void debugtrace_key(unsigned char key)
-{
-    debugtrace_toggle();
-}
-
-static int __init debugtrace_init(void)
-{
-    int order;
-    unsigned int kbytes, bytes;
-
-    /* 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;
-
-    order = get_order_from_bytes(bytes);
-    debugtrace_buf = alloc_xenheap_pages(order, 0);
-    ASSERT(debugtrace_buf != NULL);
-
-    memset(debugtrace_buf, '\0', bytes);
-
-    debugtrace_bytes = bytes;
-
-    register_keyhandler('T', debugtrace_key,
-                        "toggle debugtrace to console/buffer", 0);
-
-    return 0;
-}
-__initcall(debugtrace_init);
-
-#endif /* !CONFIG_DEBUG_TRACE */
-
-
-/*
- * **************************************************************
- * *************** Debugging/tracing/error-report ***************
+ * ********************** Error-report **************************
  * **************************************************************
  */
 
-- 
2.16.4


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

[Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Juergen Gross]

As a preparation for per-cpu buffers do a little refactoring of the
debugtrace data: put the needed buffer admin data into the buffer as
it will be needed for each buffer. In order not to limit buffer size
switch the related fields from unsigned int to unsigned long, as on
huge machines with RAM in the TB range it might be interesting to
support buffers >4GB.

While at it switch debugtrace_send_to_console and debugtrace_used to
bool and delete an empty line.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
V4:
- renamed struct debugtrace_data_s (Jan Beulich)
- renamed debtr_data (Jan Beulich)
- remove unneeded condition (Jan Beulich)
- recalc debugtrace_bytes (Jan Beulich)
---
 xen/common/debugtrace.c | 64 ++++++++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 27 deletions(-)

diff --git a/xen/common/debugtrace.c b/xen/common/debugtrace.c
index c1ee3f45b9..0eeb1a77c5 100644
--- a/xen/common/debugtrace.c
+++ b/xen/common/debugtrace.c
@@ -17,34 +17,40 @@
 #define DEBUG_TRACE_ENTRY_SIZE   1024
 
 /* Send output direct to console, or buffer it? */
-static volatile int debugtrace_send_to_console;
+static volatile bool debugtrace_send_to_console;
 
-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;
+struct debugtrace_data {
+    unsigned long bytes; /* Size of buffer. */
+    unsigned long prd;   /* Producer index. */
+    char          buf[];
+};
+
+static struct debugtrace_data *dt_data;
+
+static unsigned int debugtrace_kilobytes = 128;
+static bool debugtrace_used;
 static char debugtrace_last_entry_buf[DEBUG_TRACE_ENTRY_SIZE];
 static DEFINE_SPINLOCK(debugtrace_lock);
 integer_param("debugtrace", debugtrace_kilobytes);
 
 static void debugtrace_dump_worker(void)
 {
-    if ( (debugtrace_bytes == 0) || !debugtrace_used )
+    if ( !debugtrace_used )
         return;
 
     printk("debugtrace_dump() starting\n");
 
     /* Print oldest portion of the ring. */
-    if ( debugtrace_buf[debugtrace_prd] != '\0' )
-        console_serial_puts(&debugtrace_buf[debugtrace_prd],
-                            debugtrace_bytes - debugtrace_prd);
+    if ( dt_data->buf[dt_data->prd] != '\0' )
+        console_serial_puts(&dt_data->buf[dt_data->prd],
+                            dt_data->bytes - dt_data->prd);
 
     /* Print youngest portion of the ring. */
-    debugtrace_buf[debugtrace_prd] = '\0';
-    console_serial_puts(&debugtrace_buf[0], debugtrace_prd);
+    dt_data->buf[dt_data->prd] = '\0';
+    console_serial_puts(&dt_data->buf[0], dt_data->prd);
 
-    memset(debugtrace_buf, '\0', debugtrace_bytes);
-    debugtrace_prd = 0;
+    memset(dt_data->buf, '\0', dt_data->bytes);
+    dt_data->prd = 0;
     debugtrace_last_entry_buf[0] = 0;
 
     printk("debugtrace_dump() finished\n");
@@ -70,7 +76,6 @@ static void debugtrace_toggle(void)
 
     spin_unlock_irqrestore(&debugtrace_lock, flags);
     watchdog_enable();
-
 }
 
 void debugtrace_dump(void)
@@ -92,26 +97,27 @@ static void debugtrace_add_to_buf(char *buf)
 
     for ( p = buf; *p != '\0'; p++ )
     {
-        debugtrace_buf[debugtrace_prd++] = *p;
-        if ( debugtrace_prd == debugtrace_bytes )
-            debugtrace_prd = 0;
+        dt_data->buf[dt_data->prd++] = *p;
+        if ( dt_data->prd == dt_data->bytes )
+            dt_data->prd = 0;
     }
 }
 
 void debugtrace_printk(const char *fmt, ...)
 {
     static char buf[DEBUG_TRACE_ENTRY_SIZE];
-    static unsigned int count, last_count, last_prd;
+    static unsigned int count, last_count;
+    static unsigned long last_prd;
 
     char          cntbuf[24];
     va_list       args;
     unsigned long flags;
     unsigned int nr;
 
-    if ( debugtrace_bytes == 0 )
+    if ( !dt_data )
         return;
 
-    debugtrace_used = 1;
+    debugtrace_used = true;
 
     spin_lock_irqsave(&debugtrace_lock, flags);
 
@@ -130,14 +136,14 @@ void debugtrace_printk(const char *fmt, ...)
     {
         if ( strcmp(buf, debugtrace_last_entry_buf) )
         {
-            last_prd = debugtrace_prd;
+            last_prd = dt_data->prd;
             last_count = ++count;
             safe_strcpy(debugtrace_last_entry_buf, buf);
             snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
         }
         else
         {
-            debugtrace_prd = last_prd;
+            dt_data->prd = last_prd;
             snprintf(cntbuf, sizeof(cntbuf), "%u-%u ", last_count, ++count);
         }
         debugtrace_add_to_buf(cntbuf);
@@ -155,7 +161,8 @@ static void debugtrace_key(unsigned char key)
 static int __init debugtrace_init(void)
 {
     int order;
-    unsigned int kbytes, bytes;
+    unsigned long kbytes, bytes;
+    struct debugtrace_data *data;
 
     /* Round size down to next power of two. */
     while ( (kbytes = (debugtrace_kilobytes & (debugtrace_kilobytes-1))) != 0 )
@@ -166,12 +173,15 @@ static int __init debugtrace_init(void)
         return 0;
 
     order = get_order_from_bytes(bytes);
-    debugtrace_buf = alloc_xenheap_pages(order, 0);
-    ASSERT(debugtrace_buf != NULL);
+    data = alloc_xenheap_pages(order, 0);
+    if ( !data )
+        return -ENOMEM;
 
-    memset(debugtrace_buf, '\0', bytes);
+    bytes = PAGE_SIZE << order;
+    memset(data, '\0', bytes);
 
-    debugtrace_bytes = bytes;
+    data->bytes = bytes - sizeof(*data);
+    dt_data = data;
 
     register_keyhandler('T', debugtrace_key,
                         "toggle debugtrace to console/buffer", 0);
-- 
2.16.4


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

[Xen-devel] [PATCH v5 4/4] xen: add per-cpu buffer option to debugtrace

[Juergen Gross]

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)
V5:
- don't silently ignore trailing characters when parsing buffer size
  (Jan Beulich)
- limit scope of some variables (Jan Beulich)
- adjust error message format (Jan Beulich)
---
 docs/misc/xen-command-line.pandoc |   7 +-
 xen/common/debugtrace.c           | 159 +++++++++++++++++++++++++++++---------
 2 files changed, 126 insertions(+), 40 deletions(-)

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 0eeb1a77c5..3467fc7363 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>
@@ -26,34 +27,74 @@ 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 char debugtrace_last_entry_buf[DEBUG_TRACE_ENTRY_SIZE];
 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 )
+    unsigned long bytes;
+
+    if ( !strncmp(s, "cpu:", 4) )
+    {
+        debugtrace_per_cpu = true;
+        s += 4;
+    }
+    bytes = parse_size_and_unit(s, &s);
+
+    if ( *s )
+        return -EINVAL;
+
+    debugtrace_bytes = bytes;
+
+    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;
-    debugtrace_last_entry_buf[0] = 0;
+    memset(data->buf, '\0', data->bytes);
+    data->prd = 0;
+
+    printk("debugtrace_dump() %s buffer finished\n", which);
+}
+
+static void debugtrace_dump_worker(void)
+{
+    unsigned int cpu;
+
+    if ( !debugtrace_used )
+        return;
+
+    debugtrace_dump_buffer(dt_data, "global");
 
-    printk("debugtrace_dump() finished\n");
+    for ( cpu = 0; cpu < nr_cpu_ids; cpu++ )
+    {
+        char buf[16];
+
+        snprintf(buf, sizeof(buf), "cpu %u", cpu);
+        debugtrace_dump_buffer(per_cpu(dt_cpu_data, cpu), buf);
+    }
+
+    debugtrace_last_entry_buf[0] = 0;
 }
 
 static void debugtrace_toggle(void)
@@ -93,28 +134,33 @@ 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[DEBUG_TRACE_ENTRY_SIZE];
-    static unsigned int count, last_count;
+    static unsigned int count, last_count, last_cpu;
     static unsigned long last_prd;
 
     char          cntbuf[24];
     va_list       args;
     unsigned long flags;
     unsigned int nr;
+    struct debugtrace_data *data;
 
-    if ( !dt_data )
+    data = debugtrace_per_cpu ? this_cpu(dt_cpu_data) : dt_data;
+    if ( !data )
         return;
 
     debugtrace_used = true;
@@ -134,16 +180,19 @@ void debugtrace_printk(const char *fmt, ...)
     }
     else
     {
-        if ( strcmp(buf, debugtrace_last_entry_buf) )
+        unsigned int cpu = debugtrace_per_cpu ? smp_processor_id() : 0;
+
+        if ( strcmp(buf, debugtrace_last_entry_buf) || cpu != last_cpu )
         {
-            last_prd = dt_data->prd;
+            last_prd = data->prd;
             last_count = ++count;
+            last_cpu = cpu;
             safe_strcpy(debugtrace_last_entry_buf, buf);
             snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
         }
         else
         {
-            dt_data->prd = last_prd;
+            data->prd = last_prd;
             snprintf(cntbuf, sizeof(cntbuf), "%u-%u ", last_count, ++count);
         }
         debugtrace_add_to_buf(cntbuf);
@@ -158,34 +207,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("CPU%u: failed to allocate debugtrace buffer\n", cpu);
+        else
+            printk("failed to allocate debugtrace buffer\n");
+        return;
+    }
+
+    debugtrace_bytes = PAGE_SIZE << order;
+    memset(data, '\0', debugtrace_bytes);
+    data->bytes = debugtrace_bytes - sizeof(*data);
 
-    bytes = PAGE_SIZE << order;
-    memset(data, '\0', bytes);
+    *ptr = data;
+}
+
+static int debugtrace_cpu_callback(struct notifier_block *nfb,
+                                   unsigned long action, void *hcpu)
+{
+    unsigned int cpu = (unsigned long)hcpu;
 
-    data->bytes = bytes - sizeof(*data);
-    dt_data = data;
+    /* 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);
-- 
2.16.4


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

Re: [Xen-devel] [PATCH v5 4/4] xen: add per-cpu buffer option to debugtrace

[Jan Beulich]

On 05.09.2019 13:39, Juergen Gross wrote:
> 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>

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

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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Jan Beulich]

On 05.09.2019 13:39, Juergen Gross wrote:
> As a preparation for per-cpu buffers do a little refactoring of the
> debugtrace data: put the needed buffer admin data into the buffer as
> it will be needed for each buffer. In order not to limit buffer size
> switch the related fields from unsigned int to unsigned long, as on
> huge machines with RAM in the TB range it might be interesting to
> support buffers >4GB.

Just as a further remark in this regard:

>  void debugtrace_printk(const char *fmt, ...)
>  {
>      static char buf[DEBUG_TRACE_ENTRY_SIZE];
> -    static unsigned int count, last_count, last_prd;
> +    static unsigned int count, last_count;

How long do we think will it take until their wrapping will become
an issue with such huge buffers?

Jan

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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Juergen Gross]

On 05.09.19 14:01, Jan Beulich wrote:
> On 05.09.2019 13:39, Juergen Gross wrote:
>> As a preparation for per-cpu buffers do a little refactoring of the
>> debugtrace data: put the needed buffer admin data into the buffer as
>> it will be needed for each buffer. In order not to limit buffer size
>> switch the related fields from unsigned int to unsigned long, as on
>> huge machines with RAM in the TB range it might be interesting to
>> support buffers >4GB.
> 
> Just as a further remark in this regard:
> 
>>   void debugtrace_printk(const char *fmt, ...)
>>   {
>>       static char buf[DEBUG_TRACE_ENTRY_SIZE];
>> -    static unsigned int count, last_count, last_prd;
>> +    static unsigned int count, last_count;
> 
> How long do we think will it take until their wrapping will become
> an issue with such huge buffers?

Count wrapping will not result in any misbehavior of tracing. With
per-cpu buffers it might result in ambiguity regarding sorting the
entries, but I guess chances are rather low this being a real issue.

BTW: wrapping of count is not related to buffer size, but to the
amount of trace data written.


Juergen

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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Jan Beulich]

On 05.09.2019 13:39, Juergen Gross wrote:
> --- a/xen/common/debugtrace.c
> +++ b/xen/common/debugtrace.c
> @@ -17,34 +17,40 @@
>  #define DEBUG_TRACE_ENTRY_SIZE   1024
>  
>  /* Send output direct to console, or buffer it? */
> -static volatile int debugtrace_send_to_console;
> +static volatile bool debugtrace_send_to_console;
>  
> -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;
> +struct debugtrace_data {
> +    unsigned long bytes; /* Size of buffer. */

Hmm, I'm sorry for recognizing this only now, but why does this
field need replicating? It's the same in all instances of the
structure afaict.

Jan

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

Re: [Xen-devel] [PATCH v5 1/4] xen: fix debugtrace clearing

[Jan Beulich]

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

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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Juergen Gross]

On 05.09.19 14:13, Jan Beulich wrote:
> On 05.09.2019 13:39, Juergen Gross wrote:
>> --- a/xen/common/debugtrace.c
>> +++ b/xen/common/debugtrace.c
>> @@ -17,34 +17,40 @@
>>   #define DEBUG_TRACE_ENTRY_SIZE   1024
>>   
>>   /* Send output direct to console, or buffer it? */
>> -static volatile int debugtrace_send_to_console;
>> +static volatile bool debugtrace_send_to_console;
>>   
>> -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;
>> +struct debugtrace_data {
>> +    unsigned long bytes; /* Size of buffer. */
> 
> Hmm, I'm sorry for recognizing this only now, but why does this
> field need replicating? It's the same in all instances of the
> structure afaict.

Oh, right. In the beginning I had plans to support modifying the buffer
size at runtime.

Okay, I'll change it.


Juergen

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

Re: [Xen-devel] [PATCH v5 2/4] xen: move debugtrace coding to common/debugtrace.c

[Jan Beulich]

On 05.09.2019 13:39, Juergen Gross wrote:
> --- /dev/null
> +++ b/xen/common/debugtrace.c
> @@ -0,0 +1,181 @@
> +/******************************************************************************
> + * debugtrace.c
> + *
> + * Debugtrace for Xen
> + */
> +
> +
> +#include <xen/console.h>
> +#include <xen/init.h>
> +#include <xen/keyhandler.h>
> +#include <xen/lib.h>
> +#include <xen/mm.h>
> +#include <xen/serial.h>
> +#include <xen/spinlock.h>
> +#include <xen/watchdog.h>
> +
> +#define DEBUG_TRACE_ENTRY_SIZE   1024
> +
> +/* Send output direct to console, or buffer it? */
> +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_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);
> +
> +static void debugtrace_dump_worker(void)

And another remark here too, despite my prior ack: By moving this into
its own file, the debugtrace_ prefixes of static symbols now all
become redundant, at least as far as their occurrence in e.g. call
stacks goes (where they'd be prefixes by the disambiguating source
file name). But I know we've got ample other examples where this is
also the case, and I also know there are contrary opinions on the
matter, so this is not strictly a request for further change.

Jan

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

Re: [Xen-devel] [PATCH v5 1/4] xen: fix debugtrace clearing

[Juergen Gross]

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

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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Jan Beulich]

On 05.09.2019 14:12, Juergen Gross wrote:
> On 05.09.19 14:01, Jan Beulich wrote:
>> On 05.09.2019 13:39, Juergen Gross wrote:
>>> As a preparation for per-cpu buffers do a little refactoring of the
>>> debugtrace data: put the needed buffer admin data into the buffer as
>>> it will be needed for each buffer. In order not to limit buffer size
>>> switch the related fields from unsigned int to unsigned long, as on
>>> huge machines with RAM in the TB range it might be interesting to
>>> support buffers >4GB.
>>
>> Just as a further remark in this regard:
>>
>>>   void debugtrace_printk(const char *fmt, ...)
>>>   {
>>>       static char buf[DEBUG_TRACE_ENTRY_SIZE];
>>> -    static unsigned int count, last_count, last_prd;
>>> +    static unsigned int count, last_count;
>>
>> How long do we think will it take until their wrapping will become
>> an issue with such huge buffers?
> 
> Count wrapping will not result in any misbehavior of tracing. With
> per-cpu buffers it might result in ambiguity regarding sorting the
> entries, but I guess chances are rather low this being a real issue.
> 
> BTW: wrapping of count is not related to buffer size, but to the
> amount of trace data written.

Sure, but the chance of ambiguity increases with larger buffer sizes.

Jan

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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Jan Beulich]

On 05.09.2019 14:19, Juergen Gross wrote:
> On 05.09.19 14:13, Jan Beulich wrote:
>> On 05.09.2019 13:39, Juergen Gross wrote:
>>> --- a/xen/common/debugtrace.c
>>> +++ b/xen/common/debugtrace.c
>>> @@ -17,34 +17,40 @@
>>>   #define DEBUG_TRACE_ENTRY_SIZE   1024
>>>   
>>>   /* Send output direct to console, or buffer it? */
>>> -static volatile int debugtrace_send_to_console;
>>> +static volatile bool debugtrace_send_to_console;
>>>   
>>> -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;
>>> +struct debugtrace_data {
>>> +    unsigned long bytes; /* Size of buffer. */
>>
>> Hmm, I'm sorry for recognizing this only now, but why does this
>> field need replicating? It's the same in all instances of the
>> structure afaict.
> 
> Oh, right. In the beginning I had plans to support modifying the buffer
> size at runtime.
> 
> Okay, I'll change it.

Thanks. FAOD this is not going to invalidate any of my acks.

Jan

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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Juergen Gross]

On 05.09.19 14:22, Jan Beulich wrote:
> On 05.09.2019 14:12, Juergen Gross wrote:
>> On 05.09.19 14:01, Jan Beulich wrote:
>>> On 05.09.2019 13:39, Juergen Gross wrote:
>>>> As a preparation for per-cpu buffers do a little refactoring of the
>>>> debugtrace data: put the needed buffer admin data into the buffer as
>>>> it will be needed for each buffer. In order not to limit buffer size
>>>> switch the related fields from unsigned int to unsigned long, as on
>>>> huge machines with RAM in the TB range it might be interesting to
>>>> support buffers >4GB.
>>>
>>> Just as a further remark in this regard:
>>>
>>>>    void debugtrace_printk(const char *fmt, ...)
>>>>    {
>>>>        static char buf[DEBUG_TRACE_ENTRY_SIZE];
>>>> -    static unsigned int count, last_count, last_prd;
>>>> +    static unsigned int count, last_count;
>>>
>>> How long do we think will it take until their wrapping will become
>>> an issue with such huge buffers?
>>
>> Count wrapping will not result in any misbehavior of tracing. With
>> per-cpu buffers it might result in ambiguity regarding sorting the
>> entries, but I guess chances are rather low this being a real issue.
>>
>> BTW: wrapping of count is not related to buffer size, but to the
>> amount of trace data written.
> 
> Sure, but the chance of ambiguity increases with larger buffer sizes.

Well, better safe than sorry. Switching to unsigned long will rarely
hurt, so I'm going to do just that. The only downside will be some waste
of buffer space for very long running traces with huge amounts of
entries.


Juergen

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

Re: [Xen-devel] [PATCH v5 2/4] xen: move debugtrace coding to common/debugtrace.c

[Juergen Gross]

On 05.09.19 14:20, Jan Beulich wrote:
> On 05.09.2019 13:39, Juergen Gross wrote:
>> --- /dev/null
>> +++ b/xen/common/debugtrace.c
>> @@ -0,0 +1,181 @@
>> +/******************************************************************************
>> + * debugtrace.c
>> + *
>> + * Debugtrace for Xen
>> + */
>> +
>> +
>> +#include <xen/console.h>
>> +#include <xen/init.h>
>> +#include <xen/keyhandler.h>
>> +#include <xen/lib.h>
>> +#include <xen/mm.h>
>> +#include <xen/serial.h>
>> +#include <xen/spinlock.h>
>> +#include <xen/watchdog.h>
>> +
>> +#define DEBUG_TRACE_ENTRY_SIZE   1024
>> +
>> +/* Send output direct to console, or buffer it? */
>> +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_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);
>> +
>> +static void debugtrace_dump_worker(void)
> 
> And another remark here too, despite my prior ack: By moving this into
> its own file, the debugtrace_ prefixes of static symbols now all
> become redundant, at least as far as their occurrence in e.g. call
> stacks goes (where they'd be prefixes by the disambiguating source
> file name). But I know we've got ample other examples where this is
> also the case, and I also know there are contrary opinions on the
> matter, so this is not strictly a request for further change.

I'm one of the "contrary opinion" guys. :-)

So I'd rather keep the prefix.


Juergen


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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Jan Beulich]

On 05.09.2019 14:27, Juergen Gross wrote:
> On 05.09.19 14:22, Jan Beulich wrote:
>> On 05.09.2019 14:12, Juergen Gross wrote:
>>> On 05.09.19 14:01, Jan Beulich wrote:
>>>> On 05.09.2019 13:39, Juergen Gross wrote:
>>>>> As a preparation for per-cpu buffers do a little refactoring of the
>>>>> debugtrace data: put the needed buffer admin data into the buffer as
>>>>> it will be needed for each buffer. In order not to limit buffer size
>>>>> switch the related fields from unsigned int to unsigned long, as on
>>>>> huge machines with RAM in the TB range it might be interesting to
>>>>> support buffers >4GB.
>>>>
>>>> Just as a further remark in this regard:
>>>>
>>>>>    void debugtrace_printk(const char *fmt, ...)
>>>>>    {
>>>>>        static char buf[DEBUG_TRACE_ENTRY_SIZE];
>>>>> -    static unsigned int count, last_count, last_prd;
>>>>> +    static unsigned int count, last_count;
>>>>
>>>> How long do we think will it take until their wrapping will become
>>>> an issue with such huge buffers?
>>>
>>> Count wrapping will not result in any misbehavior of tracing. With
>>> per-cpu buffers it might result in ambiguity regarding sorting the
>>> entries, but I guess chances are rather low this being a real issue.
>>>
>>> BTW: wrapping of count is not related to buffer size, but to the
>>> amount of trace data written.
>>
>> Sure, but the chance of ambiguity increases with larger buffer sizes.
> 
> Well, better safe than sorry. Switching to unsigned long will rarely
> hurt, so I'm going to do just that. The only downside will be some waste
> of buffer space for very long running traces with huge amounts of
> entries.

Hmm, true. Maybe we could get someone else's opinion on this - anyone?

Jan

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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Juergen Gross]

On 05.09.19 14:37, Jan Beulich wrote:
> On 05.09.2019 14:27, Juergen Gross wrote:
>> On 05.09.19 14:22, Jan Beulich wrote:
>>> On 05.09.2019 14:12, Juergen Gross wrote:
>>>> On 05.09.19 14:01, Jan Beulich wrote:
>>>>> On 05.09.2019 13:39, Juergen Gross wrote:
>>>>>> As a preparation for per-cpu buffers do a little refactoring of the
>>>>>> debugtrace data: put the needed buffer admin data into the buffer as
>>>>>> it will be needed for each buffer. In order not to limit buffer size
>>>>>> switch the related fields from unsigned int to unsigned long, as on
>>>>>> huge machines with RAM in the TB range it might be interesting to
>>>>>> support buffers >4GB.
>>>>>
>>>>> Just as a further remark in this regard:
>>>>>
>>>>>>     void debugtrace_printk(const char *fmt, ...)
>>>>>>     {
>>>>>>         static char buf[DEBUG_TRACE_ENTRY_SIZE];
>>>>>> -    static unsigned int count, last_count, last_prd;
>>>>>> +    static unsigned int count, last_count;
>>>>>
>>>>> How long do we think will it take until their wrapping will become
>>>>> an issue with such huge buffers?
>>>>
>>>> Count wrapping will not result in any misbehavior of tracing. With
>>>> per-cpu buffers it might result in ambiguity regarding sorting the
>>>> entries, but I guess chances are rather low this being a real issue.
>>>>
>>>> BTW: wrapping of count is not related to buffer size, but to the
>>>> amount of trace data written.
>>>
>>> Sure, but the chance of ambiguity increases with larger buffer sizes.
>>
>> Well, better safe than sorry. Switching to unsigned long will rarely
>> hurt, so I'm going to do just that. The only downside will be some waste
>> of buffer space for very long running traces with huge amounts of
>> entries.
> 
> Hmm, true. Maybe we could get someone else's opinion on this - anyone?

TBH, I wouldn't be concerned about the buffer space. In case there are
really billions of entries, the difference between a 10-digit count
value and maybe a 15 digit one (and that is already a massive amount)
isn't that large. The average printed size of count with about
4 billion entries is 9.75 digits (and not just 5 :-) ).


Juergen


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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Juergen Gross]

On 05.09.19 14:46, Juergen Gross wrote:
> On 05.09.19 14:37, Jan Beulich wrote:
>> On 05.09.2019 14:27, Juergen Gross wrote:
>>> On 05.09.19 14:22, Jan Beulich wrote:
>>>> On 05.09.2019 14:12, Juergen Gross wrote:
>>>>> On 05.09.19 14:01, Jan Beulich wrote:
>>>>>> On 05.09.2019 13:39, Juergen Gross wrote:
>>>>>>> As a preparation for per-cpu buffers do a little refactoring of the
>>>>>>> debugtrace data: put the needed buffer admin data into the buffer as
>>>>>>> it will be needed for each buffer. In order not to limit buffer size
>>>>>>> switch the related fields from unsigned int to unsigned long, as on
>>>>>>> huge machines with RAM in the TB range it might be interesting to
>>>>>>> support buffers >4GB.
>>>>>>
>>>>>> Just as a further remark in this regard:
>>>>>>
>>>>>>>     void debugtrace_printk(const char *fmt, ...)
>>>>>>>     {
>>>>>>>         static char buf[DEBUG_TRACE_ENTRY_SIZE];
>>>>>>> -    static unsigned int count, last_count, last_prd;
>>>>>>> +    static unsigned int count, last_count;
>>>>>>
>>>>>> How long do we think will it take until their wrapping will become
>>>>>> an issue with such huge buffers?
>>>>>
>>>>> Count wrapping will not result in any misbehavior of tracing. With
>>>>> per-cpu buffers it might result in ambiguity regarding sorting the
>>>>> entries, but I guess chances are rather low this being a real issue.
>>>>>
>>>>> BTW: wrapping of count is not related to buffer size, but to the
>>>>> amount of trace data written.
>>>>
>>>> Sure, but the chance of ambiguity increases with larger buffer sizes.
>>>
>>> Well, better safe than sorry. Switching to unsigned long will rarely
>>> hurt, so I'm going to do just that. The only downside will be some waste
>>> of buffer space for very long running traces with huge amounts of
>>> entries.
>>
>> Hmm, true. Maybe we could get someone else's opinion on this - anyone?
> 
> TBH, I wouldn't be concerned about the buffer space. In case there are
> really billions of entries, the difference between a 10-digit count
> value and maybe a 15 digit one (and that is already a massive amount)
> isn't that large. The average printed size of count with about
> 4 billion entries is 9.75 digits (and not just 5 :-) ).

Another option would be to let count wrap at e.g. 4 billion (or even 1
million) and add a wrap count. Each buffer struct would contain the
wrap count of the last entry, and when hitting a higher wrap count a
new entry like ("wrap %u->%u", old_wrap, new_wrap) would be printed.
This saves buffer space without loosing information.


Juergen

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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Jan Beulich]

On 05.09.2019 16:36, Juergen Gross wrote:
> On 05.09.19 14:46, Juergen Gross wrote:
>> On 05.09.19 14:37, Jan Beulich wrote:
>>> On 05.09.2019 14:27, Juergen Gross wrote:
>>>> On 05.09.19 14:22, Jan Beulich wrote:
>>>>> On 05.09.2019 14:12, Juergen Gross wrote:
>>>>>> On 05.09.19 14:01, Jan Beulich wrote:
>>>>>>> On 05.09.2019 13:39, Juergen Gross wrote:
>>>>>>>> As a preparation for per-cpu buffers do a little refactoring of the
>>>>>>>> debugtrace data: put the needed buffer admin data into the buffer as
>>>>>>>> it will be needed for each buffer. In order not to limit buffer size
>>>>>>>> switch the related fields from unsigned int to unsigned long, as on
>>>>>>>> huge machines with RAM in the TB range it might be interesting to
>>>>>>>> support buffers >4GB.
>>>>>>>
>>>>>>> Just as a further remark in this regard:
>>>>>>>
>>>>>>>>     void debugtrace_printk(const char *fmt, ...)
>>>>>>>>     {
>>>>>>>>         static char buf[DEBUG_TRACE_ENTRY_SIZE];
>>>>>>>> -    static unsigned int count, last_count, last_prd;
>>>>>>>> +    static unsigned int count, last_count;
>>>>>>>
>>>>>>> How long do we think will it take until their wrapping will become
>>>>>>> an issue with such huge buffers?
>>>>>>
>>>>>> Count wrapping will not result in any misbehavior of tracing. With
>>>>>> per-cpu buffers it might result in ambiguity regarding sorting the
>>>>>> entries, but I guess chances are rather low this being a real issue.
>>>>>>
>>>>>> BTW: wrapping of count is not related to buffer size, but to the
>>>>>> amount of trace data written.
>>>>>
>>>>> Sure, but the chance of ambiguity increases with larger buffer sizes.
>>>>
>>>> Well, better safe than sorry. Switching to unsigned long will rarely
>>>> hurt, so I'm going to do just that. The only downside will be some waste
>>>> of buffer space for very long running traces with huge amounts of
>>>> entries.
>>>
>>> Hmm, true. Maybe we could get someone else's opinion on this - anyone?
>>
>> TBH, I wouldn't be concerned about the buffer space. In case there are
>> really billions of entries, the difference between a 10-digit count
>> value and maybe a 15 digit one (and that is already a massive amount)
>> isn't that large. The average printed size of count with about
>> 4 billion entries is 9.75 digits (and not just 5 :-) ).
> 
> Another option would be to let count wrap at e.g. 4 billion (or even 1
> million) and add a wrap count. Each buffer struct would contain the
> wrap count of the last entry, and when hitting a higher wrap count a
> new entry like ("wrap %u->%u", old_wrap, new_wrap) would be printed.
> This saves buffer space without loosing information.

This sounds quite neat.

Jan

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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Juergen Gross]

On 05.09.19 16:43, Jan Beulich wrote:
> On 05.09.2019 16:36, Juergen Gross wrote:
>> On 05.09.19 14:46, Juergen Gross wrote:
>>> On 05.09.19 14:37, Jan Beulich wrote:
>>>> On 05.09.2019 14:27, Juergen Gross wrote:
>>>>> On 05.09.19 14:22, Jan Beulich wrote:
>>>>>> On 05.09.2019 14:12, Juergen Gross wrote:
>>>>>>> On 05.09.19 14:01, Jan Beulich wrote:
>>>>>>>> On 05.09.2019 13:39, Juergen Gross wrote:
>>>>>>>>> As a preparation for per-cpu buffers do a little refactoring of the
>>>>>>>>> debugtrace data: put the needed buffer admin data into the buffer as
>>>>>>>>> it will be needed for each buffer. In order not to limit buffer size
>>>>>>>>> switch the related fields from unsigned int to unsigned long, as on
>>>>>>>>> huge machines with RAM in the TB range it might be interesting to
>>>>>>>>> support buffers >4GB.
>>>>>>>>
>>>>>>>> Just as a further remark in this regard:
>>>>>>>>
>>>>>>>>>      void debugtrace_printk(const char *fmt, ...)
>>>>>>>>>      {
>>>>>>>>>          static char buf[DEBUG_TRACE_ENTRY_SIZE];
>>>>>>>>> -    static unsigned int count, last_count, last_prd;
>>>>>>>>> +    static unsigned int count, last_count;
>>>>>>>>
>>>>>>>> How long do we think will it take until their wrapping will become
>>>>>>>> an issue with such huge buffers?
>>>>>>>
>>>>>>> Count wrapping will not result in any misbehavior of tracing. With
>>>>>>> per-cpu buffers it might result in ambiguity regarding sorting the
>>>>>>> entries, but I guess chances are rather low this being a real issue.
>>>>>>>
>>>>>>> BTW: wrapping of count is not related to buffer size, but to the
>>>>>>> amount of trace data written.
>>>>>>
>>>>>> Sure, but the chance of ambiguity increases with larger buffer sizes.
>>>>>
>>>>> Well, better safe than sorry. Switching to unsigned long will rarely
>>>>> hurt, so I'm going to do just that. The only downside will be some waste
>>>>> of buffer space for very long running traces with huge amounts of
>>>>> entries.
>>>>
>>>> Hmm, true. Maybe we could get someone else's opinion on this - anyone?
>>>
>>> TBH, I wouldn't be concerned about the buffer space. In case there are
>>> really billions of entries, the difference between a 10-digit count
>>> value and maybe a 15 digit one (and that is already a massive amount)
>>> isn't that large. The average printed size of count with about
>>> 4 billion entries is 9.75 digits (and not just 5 :-) ).
>>
>> Another option would be to let count wrap at e.g. 4 billion (or even 1
>> million) and add a wrap count. Each buffer struct would contain the
>> wrap count of the last entry, and when hitting a higher wrap count a
>> new entry like ("wrap %u->%u", old_wrap, new_wrap) would be printed.
>> This saves buffer space without loosing information.
> 
> This sounds quite neat.

I'm adding a new patch for that purpose, as it is adding a new feature.

Any preferences for the max value of count? I'm in favor of limiting it
to 6-digit numbers as those are much easier to compare by just looking
at them.


Juergen


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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Jan Beulich]

On 06.09.2019 10:49, Juergen Gross wrote:
> On 05.09.19 16:43, Jan Beulich wrote:
>> On 05.09.2019 16:36, Juergen Gross wrote:
>>> On 05.09.19 14:46, Juergen Gross wrote:
>>>> On 05.09.19 14:37, Jan Beulich wrote:
>>>>> On 05.09.2019 14:27, Juergen Gross wrote:
>>>>>> On 05.09.19 14:22, Jan Beulich wrote:
>>>>>>> On 05.09.2019 14:12, Juergen Gross wrote:
>>>>>>>> On 05.09.19 14:01, Jan Beulich wrote:
>>>>>>>>> On 05.09.2019 13:39, Juergen Gross wrote:
>>>>>>>>>> As a preparation for per-cpu buffers do a little refactoring of the
>>>>>>>>>> debugtrace data: put the needed buffer admin data into the buffer as
>>>>>>>>>> it will be needed for each buffer. In order not to limit buffer size
>>>>>>>>>> switch the related fields from unsigned int to unsigned long, as on
>>>>>>>>>> huge machines with RAM in the TB range it might be interesting to
>>>>>>>>>> support buffers >4GB.
>>>>>>>>>
>>>>>>>>> Just as a further remark in this regard:
>>>>>>>>>
>>>>>>>>>>      void debugtrace_printk(const char *fmt, ...)
>>>>>>>>>>      {
>>>>>>>>>>          static char buf[DEBUG_TRACE_ENTRY_SIZE];
>>>>>>>>>> -    static unsigned int count, last_count, last_prd;
>>>>>>>>>> +    static unsigned int count, last_count;
>>>>>>>>>
>>>>>>>>> How long do we think will it take until their wrapping will become
>>>>>>>>> an issue with such huge buffers?
>>>>>>>>
>>>>>>>> Count wrapping will not result in any misbehavior of tracing. With
>>>>>>>> per-cpu buffers it might result in ambiguity regarding sorting the
>>>>>>>> entries, but I guess chances are rather low this being a real issue.
>>>>>>>>
>>>>>>>> BTW: wrapping of count is not related to buffer size, but to the
>>>>>>>> amount of trace data written.
>>>>>>>
>>>>>>> Sure, but the chance of ambiguity increases with larger buffer sizes.
>>>>>>
>>>>>> Well, better safe than sorry. Switching to unsigned long will rarely
>>>>>> hurt, so I'm going to do just that. The only downside will be some waste
>>>>>> of buffer space for very long running traces with huge amounts of
>>>>>> entries.
>>>>>
>>>>> Hmm, true. Maybe we could get someone else's opinion on this - anyone?
>>>>
>>>> TBH, I wouldn't be concerned about the buffer space. In case there are
>>>> really billions of entries, the difference between a 10-digit count
>>>> value and maybe a 15 digit one (and that is already a massive amount)
>>>> isn't that large. The average printed size of count with about
>>>> 4 billion entries is 9.75 digits (and not just 5 :-) ).
>>>
>>> Another option would be to let count wrap at e.g. 4 billion (or even 1
>>> million) and add a wrap count. Each buffer struct would contain the
>>> wrap count of the last entry, and when hitting a higher wrap count a
>>> new entry like ("wrap %u->%u", old_wrap, new_wrap) would be printed.
>>> This saves buffer space without loosing information.
>>
>> This sounds quite neat.
> 
> I'm adding a new patch for that purpose, as it is adding a new feature.
> 
> Any preferences for the max value of count? I'm in favor of limiting it
> to 6-digit numbers as those are much easier to compare by just looking
> at them.

I'm not overly fussed; personally I'd probably wrap at 30 bits, making it
generally up to 8 digits, just very slightly going into the 9-digit range.

Jan

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

Re: [Xen-devel] [PATCH v5 3/4] xen: refactor debugtrace data

[Juergen Gross]

On 06.09.19 11:10, Jan Beulich wrote:
> On 06.09.2019 10:49, Juergen Gross wrote:
>> On 05.09.19 16:43, Jan Beulich wrote:
>>> On 05.09.2019 16:36, Juergen Gross wrote:
>>>> On 05.09.19 14:46, Juergen Gross wrote:
>>>>> On 05.09.19 14:37, Jan Beulich wrote:
>>>>>> On 05.09.2019 14:27, Juergen Gross wrote:
>>>>>>> On 05.09.19 14:22, Jan Beulich wrote:
>>>>>>>> On 05.09.2019 14:12, Juergen Gross wrote:
>>>>>>>>> On 05.09.19 14:01, Jan Beulich wrote:
>>>>>>>>>> On 05.09.2019 13:39, Juergen Gross wrote:
>>>>>>>>>>> As a preparation for per-cpu buffers do a little refactoring of the
>>>>>>>>>>> debugtrace data: put the needed buffer admin data into the buffer as
>>>>>>>>>>> it will be needed for each buffer. In order not to limit buffer size
>>>>>>>>>>> switch the related fields from unsigned int to unsigned long, as on
>>>>>>>>>>> huge machines with RAM in the TB range it might be interesting to
>>>>>>>>>>> support buffers >4GB.
>>>>>>>>>>
>>>>>>>>>> Just as a further remark in this regard:
>>>>>>>>>>
>>>>>>>>>>>       void debugtrace_printk(const char *fmt, ...)
>>>>>>>>>>>       {
>>>>>>>>>>>           static char buf[DEBUG_TRACE_ENTRY_SIZE];
>>>>>>>>>>> -    static unsigned int count, last_count, last_prd;
>>>>>>>>>>> +    static unsigned int count, last_count;
>>>>>>>>>>
>>>>>>>>>> How long do we think will it take until their wrapping will become
>>>>>>>>>> an issue with such huge buffers?
>>>>>>>>>
>>>>>>>>> Count wrapping will not result in any misbehavior of tracing. With
>>>>>>>>> per-cpu buffers it might result in ambiguity regarding sorting the
>>>>>>>>> entries, but I guess chances are rather low this being a real issue.
>>>>>>>>>
>>>>>>>>> BTW: wrapping of count is not related to buffer size, but to the
>>>>>>>>> amount of trace data written.
>>>>>>>>
>>>>>>>> Sure, but the chance of ambiguity increases with larger buffer sizes.
>>>>>>>
>>>>>>> Well, better safe than sorry. Switching to unsigned long will rarely
>>>>>>> hurt, so I'm going to do just that. The only downside will be some waste
>>>>>>> of buffer space for very long running traces with huge amounts of
>>>>>>> entries.
>>>>>>
>>>>>> Hmm, true. Maybe we could get someone else's opinion on this - anyone?
>>>>>
>>>>> TBH, I wouldn't be concerned about the buffer space. In case there are
>>>>> really billions of entries, the difference between a 10-digit count
>>>>> value and maybe a 15 digit one (and that is already a massive amount)
>>>>> isn't that large. The average printed size of count with about
>>>>> 4 billion entries is 9.75 digits (and not just 5 :-) ).
>>>>
>>>> Another option would be to let count wrap at e.g. 4 billion (or even 1
>>>> million) and add a wrap count. Each buffer struct would contain the
>>>> wrap count of the last entry, and when hitting a higher wrap count a
>>>> new entry like ("wrap %u->%u", old_wrap, new_wrap) would be printed.
>>>> This saves buffer space without loosing information.
>>>
>>> This sounds quite neat.
>>
>> I'm adding a new patch for that purpose, as it is adding a new feature.
>>
>> Any preferences for the max value of count? I'm in favor of limiting it
>> to 6-digit numbers as those are much easier to compare by just looking
>> at them.
> 
> I'm not overly fussed; personally I'd probably wrap at 30 bits, making it
> generally up to 8 digits, just very slightly going into the 9-digit range.

2^30 is a 10-digit number. :-)

But wrapping at 100.000.000 is fine, too, as it is just

   if ( count == 100000000 )

and that is not required to be a nice binary number.


Juergen

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