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

[Xen-devel] [PATCH v3 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 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: use common output function in debugtrace
  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           | 265 ++++++++++++++++++++++++++++++++++++++
 xen/drivers/char/console.c        | 186 +-------------------------
 xen/include/xen/console.h         |   3 +
 5 files changed, 278 insertions(+), 184 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 v3 1/4] xen: use common output function in debugtrace

[Juergen Gross]

Today dumping the debugtrace buffers is done via sercon_puts(), while
direct printing of trace entries (after toggling output to the console)
is using serial_puts().

Use sercon_puts() in both cases, as the difference between both is not
really making sense.

In order to prepare moving debugtrace functionality to an own source
file rename sercon_puts() to console_serial_puts() and make it globally
visible.

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

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index d67e1993f2..f49c6f29a8 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -346,7 +346,7 @@ void console_giveback(int id)
         serial_steal_fn = NULL;
 }
 
-static void sercon_puts(const char *s, size_t nr)
+void console_serial_puts(const char *s, size_t nr)
 {
     if ( serial_steal_fn != NULL )
         serial_steal_fn(s, nr);
@@ -388,7 +388,7 @@ static void dump_console_ring_key(unsigned char key)
         c += len;
     }
 
-    sercon_puts(buf, sofar);
+    console_serial_puts(buf, sofar);
     video_puts(buf, sofar);
 
     free_xenheap_pages(buf, order);
@@ -547,7 +547,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
             /* Use direct console output as it could be interactive */
             spin_lock_irq(&console_lock);
 
-            sercon_puts(kbuf, kcount);
+            console_serial_puts(kbuf, kcount);
             video_puts(kbuf, kcount);
 
 #ifdef CONFIG_X86
@@ -674,7 +674,7 @@ static void __putstr(const char *str)
 
     ASSERT(spin_is_locked(&console_lock));
 
-    sercon_puts(str, len);
+    console_serial_puts(str, len);
     video_puts(str, len);
 
 #ifdef CONFIG_X86
@@ -1186,12 +1186,12 @@ static void debugtrace_dump_worker(void)
     /* Print oldest portion of the ring. */
     ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
     if ( debugtrace_buf[debugtrace_prd] != '\0' )
-        sercon_puts(&debugtrace_buf[debugtrace_prd],
-                    debugtrace_bytes - debugtrace_prd - 1);
+        console_serial_puts(&debugtrace_buf[debugtrace_prd],
+                            debugtrace_bytes - debugtrace_prd - 1);
 
     /* Print youngest portion of the ring. */
     debugtrace_buf[debugtrace_prd] = '\0';
-    sercon_puts(&debugtrace_buf[0], debugtrace_prd);
+    console_serial_puts(&debugtrace_buf[0], debugtrace_prd);
 
     memset(debugtrace_buf, '\0', debugtrace_bytes);
 
@@ -1274,8 +1274,8 @@ void debugtrace_printk(const char *fmt, ...)
     {
         unsigned int n = scnprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);
 
-        serial_puts(sercon_handle, cntbuf, n);
-        serial_puts(sercon_handle, buf, nr);
+        console_serial_puts(cntbuf, n);
+        console_serial_puts(buf, nr);
     }
     else
     {
diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
index ba914f9e5b..53c56191ba 100644
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -46,6 +46,9 @@ void console_giveback(int id);
 int console_suspend(void);
 int console_resume(void);
 
+/* Emit a string via the serial console. */
+void console_serial_puts(const char *s, size_t nr);
+
 extern int8_t opt_console_xen;
 
 #endif /* __CONSOLE_H__ */
-- 
2.16.4


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

[Xen-devel] [PATCH v3 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>
---
 xen/common/Makefile        |   1 +
 xen/common/debugtrace.c    | 181 +++++++++++++++++++++++++++++++++++++++++++++
 xen/drivers/char/console.c | 178 +-------------------------------------------
 3 files changed, 183 insertions(+), 177 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..a2fab0d72c
--- /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>
+
+/* 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 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. */
+    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);
+
+    /* 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);
+
+    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;
+        /* 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], last_buf[1024];
+    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);
+
+    ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
+
+    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, last_buf) )
+        {
+            last_prd = debugtrace_prd;
+            last_count = ++count;
+            safe_strcpy(last_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 f49c6f29a8..7f29190eaf 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1160,183 +1160,7 @@ int printk_ratelimit(void)
 
 /*
  * **************************************************************
- * *************** Serial console ring buffer *******************
- * **************************************************************
- */
-
-#ifdef CONFIG_DEBUG_TRACE
-
-/* 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 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. */
-    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);
-
-    /* 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);
-
-    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;
-        /* 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], last_buf[1024];
-    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);
-
-    ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
-
-    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, last_buf) )
-        {
-            last_prd = debugtrace_prd;
-            last_count = ++count;
-            safe_strcpy(last_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 v3 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.

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>
---
 xen/common/debugtrace.c | 62 ++++++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 27 deletions(-)

diff --git a/xen/common/debugtrace.c b/xen/common/debugtrace.c
index a2fab0d72c..7a9e4fb99f 100644
--- a/xen/common/debugtrace.c
+++ b/xen/common/debugtrace.c
@@ -15,33 +15,39 @@
 #include <xen/watchdog.h>
 
 /* 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_s {
+    unsigned long bytes; /* Size of buffer. */
+    unsigned long prd;   /* Producer index. */
+    char          buf[];
+};
+
+static struct debugtrace_data_s *debtr_data;
+
+static unsigned int debugtrace_kilobytes = 128;
+static bool debugtrace_used;
 static DEFINE_SPINLOCK(debugtrace_lock);
 integer_param("debugtrace", debugtrace_kilobytes);
 
 static void debugtrace_dump_worker(void)
 {
-    if ( (debugtrace_bytes == 0) || !debugtrace_used )
+    if ( !debtr_data || !debugtrace_used )
         return;
 
     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);
+    ASSERT(debtr_data->buf[debtr_data->bytes - 1] == 0);
+    if ( debtr_data->buf[debtr_data->prd] != '\0' )
+        console_serial_puts(&debtr_data->buf[debtr_data->prd],
+                            debtr_data->bytes - debtr_data->prd - 1);
 
     /* Print youngest portion of the ring. */
-    debugtrace_buf[debugtrace_prd] = '\0';
-    console_serial_puts(&debugtrace_buf[0], debugtrace_prd);
+    debtr_data->buf[debtr_data->prd] = '\0';
+    console_serial_puts(&debtr_data->buf[0], debtr_data->prd);
 
-    memset(debugtrace_buf, '\0', debugtrace_bytes);
+    memset(debtr_data->buf, '\0', debtr_data->bytes);
 
     printk("debugtrace_dump() finished\n");
 }
@@ -66,7 +72,6 @@ static void debugtrace_toggle(void)
 
     spin_unlock_irqrestore(&debugtrace_lock, flags);
     watchdog_enable();
-
 }
 
 void debugtrace_dump(void)
@@ -88,10 +93,10 @@ static void debugtrace_add_to_buf(char *buf)
 
     for ( p = buf; *p != '\0'; p++ )
     {
-        debugtrace_buf[debugtrace_prd++] = *p;
+        debtr_data->buf[debtr_data->prd++] = *p;
         /* Always leave a nul byte at the end of the buffer. */
-        if ( debugtrace_prd == (debugtrace_bytes - 1) )
-            debugtrace_prd = 0;
+        if ( debtr_data->prd == (debtr_data->bytes - 1) )
+            debtr_data->prd = 0;
     }
 }
 
@@ -105,14 +110,14 @@ void debugtrace_printk(const char *fmt, ...)
     unsigned long flags;
     unsigned int nr;
 
-    if ( debugtrace_bytes == 0 )
+    if ( !debtr_data )
         return;
 
-    debugtrace_used = 1;
+    debugtrace_used = true;
 
     spin_lock_irqsave(&debugtrace_lock, flags);
 
-    ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
+    ASSERT(debtr_data->buf[debtr_data->bytes - 1] == 0);
 
     va_start(args, fmt);
     nr = vsnprintf(buf, sizeof(buf), fmt, args);
@@ -129,14 +134,14 @@ void debugtrace_printk(const char *fmt, ...)
     {
         if ( strcmp(buf, last_buf) )
         {
-            last_prd = debugtrace_prd;
+            last_prd = debtr_data->prd;
             last_count = ++count;
             safe_strcpy(last_buf, buf);
             snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
         }
         else
         {
-            debugtrace_prd = last_prd;
+            debtr_data->prd = last_prd;
             snprintf(cntbuf, sizeof(cntbuf), "%u-%u ", last_count, ++count);
         }
         debugtrace_add_to_buf(cntbuf);
@@ -154,7 +159,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_s *data;
 
     /* Round size down to next power of two. */
     while ( (kbytes = (debugtrace_kilobytes & (debugtrace_kilobytes-1))) != 0 )
@@ -165,12 +171,14 @@ 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);
+    memset(data, '\0', bytes);
 
-    debugtrace_bytes = bytes;
+    data->bytes = bytes - sizeof(*data);
+    debtr_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 v3 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
---
 docs/misc/xen-command-line.pandoc |   7 +-
 xen/common/debugtrace.c           | 148 ++++++++++++++++++++++++++++----------
 2 files changed, 116 insertions(+), 39 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 7a9e4fb99f..a54888cea9 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>
@@ -24,32 +25,62 @@ struct debugtrace_data_s {
 };
 
 static struct debugtrace_data_s *debtr_data;
+static DEFINE_PER_CPU(struct debugtrace_data_s *, debtr_cpu_data);
 
-static unsigned int debugtrace_kilobytes = 128;
+static unsigned int debugtrace_bytes = 128 << 10;
+static bool debugtrace_per_cpu;
 static bool debugtrace_used;
 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 ( !strncmp(s, "cpu:", 4) )
+    {
+        debugtrace_per_cpu = true;
+        s += 4;
+    }
+    debugtrace_bytes =  parse_size_and_unit(s, NULL);
+    return 0;
+}
+custom_param("debugtrace", debugtrace_parse_param);
+
+static void debugtrace_dump_buffer(struct debugtrace_data_s *data,
+                                   const char *which)
 {
-    if ( !debtr_data || !debugtrace_used )
+    if ( !data )
         return;
 
-    printk("debugtrace_dump() starting\n");
+    printk("debugtrace_dump() %s buffer starting\n", which);
 
     /* Print oldest portion of the ring. */
-    ASSERT(debtr_data->buf[debtr_data->bytes - 1] == 0);
-    if ( debtr_data->buf[debtr_data->prd] != '\0' )
-        console_serial_puts(&debtr_data->buf[debtr_data->prd],
-                            debtr_data->bytes - debtr_data->prd - 1);
+    ASSERT(data->buf[data->bytes - 1] == 0);
+    if ( data->buf[data->prd] != '\0' )
+        console_serial_puts(&data->buf[data->prd], data->bytes - data->prd - 1);
 
     /* Print youngest portion of the ring. */
-    debtr_data->buf[debtr_data->prd] = '\0';
-    console_serial_puts(&debtr_data->buf[0], debtr_data->prd);
+    data->buf[data->prd] = '\0';
+    console_serial_puts(&data->buf[0], data->prd);
+
+    memset(data->buf, '\0', data->bytes);
+
+    printk("debugtrace_dump() %s buffer finished\n", which);
+}
+
+static void debugtrace_dump_worker(void)
+{
+    unsigned int cpu;
+    char buf[16];
+
+    if ( !debugtrace_used )
+        return;
 
-    memset(debtr_data->buf, '\0', debtr_data->bytes);
+    debugtrace_dump_buffer(debtr_data, "global");
 
-    printk("debugtrace_dump() finished\n");
+    for ( cpu = 0; cpu < nr_cpu_ids; cpu++ )
+    {
+        snprintf(buf, sizeof(buf), "cpu %u", cpu);
+        debugtrace_dump_buffer(per_cpu(debtr_cpu_data, cpu), buf);
+    }
 }
 
 static void debugtrace_toggle(void)
@@ -89,35 +120,42 @@ void debugtrace_dump(void)
 
 static void debugtrace_add_to_buf(char *buf)
 {
+    struct debugtrace_data_s *data;
     char *p;
 
+    data = debugtrace_per_cpu ? this_cpu(debtr_cpu_data) : debtr_data;
+
     for ( p = buf; *p != '\0'; p++ )
     {
-        debtr_data->buf[debtr_data->prd++] = *p;
+        data->buf[data->prd++] = *p;
         /* Always leave a nul byte at the end of the buffer. */
-        if ( debtr_data->prd == (debtr_data->bytes - 1) )
-            debtr_data->prd = 0;
+        if ( data->prd == (data->bytes - 1) )
+            data->prd = 0;
     }
 }
 
 void debugtrace_printk(const char *fmt, ...)
 {
     static char buf[1024], last_buf[1024];
-    static unsigned int count, last_count, last_prd;
+    static unsigned int count, last_count, last_prd, last_cpu;
 
     char          cntbuf[24];
     va_list       args;
     unsigned long flags;
     unsigned int nr;
+    struct debugtrace_data_s *data;
+    unsigned int cpu;
 
-    if ( !debtr_data )
+    data = debugtrace_per_cpu ? this_cpu(debtr_cpu_data) : debtr_data;
+    cpu = debugtrace_per_cpu ? smp_processor_id() : 0;
+    if ( !data )
         return;
 
     debugtrace_used = true;
 
     spin_lock_irqsave(&debugtrace_lock, flags);
 
-    ASSERT(debtr_data->buf[debtr_data->bytes - 1] == 0);
+    ASSERT(data->buf[data->bytes - 1] == 0);
 
     va_start(args, fmt);
     nr = vsnprintf(buf, sizeof(buf), fmt, args);
@@ -132,16 +170,17 @@ void debugtrace_printk(const char *fmt, ...)
     }
     else
     {
-        if ( strcmp(buf, last_buf) )
+        if ( strcmp(buf, last_buf) || cpu != last_cpu )
         {
-            last_prd = debtr_data->prd;
+            last_prd = data->prd;
             last_count = ++count;
+            last_cpu = cpu;
             safe_strcpy(last_buf, buf);
             snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
         }
         else
         {
-            debtr_data->prd = last_prd;
+            data->prd = last_prd;
             snprintf(cntbuf, sizeof(cntbuf), "%u-%u ", last_count, ++count);
         }
         debugtrace_add_to_buf(cntbuf);
@@ -156,33 +195,70 @@ static void debugtrace_key(unsigned char key)
     debugtrace_toggle();
 }
 
-static int __init debugtrace_init(void)
+static void debugtrace_alloc_buffer(struct debugtrace_data_s **ptr)
 {
     int order;
-    unsigned long kbytes, bytes;
     struct debugtrace_data_s *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 < PAGE_SIZE || *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;
+    {
+        printk("failed to allocate debugtrace buffer\n");
+        return;
+    }
+
+    memset(data, '\0', debugtrace_bytes);
+    data->bytes = debugtrace_bytes - sizeof(*data);
+
+    *ptr = data;
+}
+
+static int debugtrace_cpu_callback(struct notifier_block *nfb,
+                                   unsigned long action, void *hcpu)
+{
+    unsigned int cpu = (unsigned long)hcpu;
+
+    /* Buffers are only ever allocated, never freed. */
+    switch ( action )
+    {
+    case CPU_UP_PREPARE:
+        debugtrace_alloc_buffer(&per_cpu(debtr_cpu_data, cpu));
+        break;
+    default:
+        break;
+    }
+    return 0;
+}
 
-    memset(data, '\0', bytes);
+static struct notifier_block debugtrace_nfb = {
+    .notifier_call = debugtrace_cpu_callback
+};
+
+static int __init debugtrace_init(void)
+{
+    unsigned long bytes;
+    unsigned int cpu;
 
-    data->bytes = bytes - sizeof(*data);
-    debtr_data = data;
+    /* 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(debtr_cpu_data, cpu));
+        register_cpu_notifier(&debugtrace_nfb);
+    }
+    else
+        debugtrace_alloc_buffer(&debtr_data);
+
     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 v3 1/4] xen: use common output function in debugtrace

[Jan Beulich]

On 28.08.2019 10:00, Juergen Gross wrote:
> Today dumping the debugtrace buffers is done via sercon_puts(), while
> direct printing of trace entries (after toggling output to the console)
> is using serial_puts().
> 
> Use sercon_puts() in both cases, as the difference between both is not
> really making sense.

No matter that I like this change, I'm not convinced it's correct:
There are two differences between the functions, neither of which
I could qualify as "not really making sense": Why is it obvious
that it makes no sense for the debugtrace output to bypass one or
both of serial_steal_fn() and pv_console_puts()? If you're
convinced of this, please provide the "why"-s behind the sentence
above.

Jan

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

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

[Jan Beulich]

On 28.08.2019 10:00, Juergen Gross wrote:
> 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-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

On 28.08.2019 10:00, 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.
> 
> 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>
> ---
>  xen/common/debugtrace.c | 62 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 35 insertions(+), 27 deletions(-)
> 
> diff --git a/xen/common/debugtrace.c b/xen/common/debugtrace.c
> index a2fab0d72c..7a9e4fb99f 100644
> --- a/xen/common/debugtrace.c
> +++ b/xen/common/debugtrace.c
> @@ -15,33 +15,39 @@
>  #include <xen/watchdog.h>
>  
>  /* 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_s {

Is the _s suffix really needed for some reason?

> +    unsigned long bytes; /* Size of buffer. */
> +    unsigned long prd;   /* Producer index. */

Seeing that you switch from int to long here - are you really
fancying these buffers to go beyond 4GiB in size?

> +    char          buf[];
> +};
> +
> +static struct debugtrace_data_s *debtr_data;

How about s/debtr/dt/ or s/debtr/debugtrace/ ?

> +static unsigned int debugtrace_kilobytes = 128;

Since you touch this anyway, add __initdata? Maybe also move it
next to its integer_param()?

> +static bool debugtrace_used;
>  static DEFINE_SPINLOCK(debugtrace_lock);
>  integer_param("debugtrace", debugtrace_kilobytes);
>  
>  static void debugtrace_dump_worker(void)
>  {
> -    if ( (debugtrace_bytes == 0) || !debugtrace_used )
> +    if ( !debtr_data || !debugtrace_used )
>          return;

Why don't you get rid of the left side of the || altogether?

> @@ -165,12 +171,14 @@ 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);
> +    memset(data, '\0', bytes);
>  
> -    debugtrace_bytes = bytes;
> +    data->bytes = bytes - sizeof(*data);
> +    debtr_data = data;

The allocation may end up being almost twice as big as what gets
actually used this way. Wouldn't it make sense to re-calculate
"bytes" from "order"?

Jan

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

Re: [Xen-devel] [PATCH v3 1/4] xen: use common output function in debugtrace

[Juergen Gross]

On 03.09.19 12:00, Jan Beulich wrote:
> On 28.08.2019 10:00, Juergen Gross wrote:
>> Today dumping the debugtrace buffers is done via sercon_puts(), while
>> direct printing of trace entries (after toggling output to the console)
>> is using serial_puts().
>>
>> Use sercon_puts() in both cases, as the difference between both is not
>> really making sense.
> 
> No matter that I like this change, I'm not convinced it's correct:
> There are two differences between the functions, neither of which
> I could qualify as "not really making sense": Why is it obvious
> that it makes no sense for the debugtrace output to bypass one or
> both of serial_steal_fn() and pv_console_puts()? If you're
> convinced of this, please provide the "why"-s behind the sentence
> above.

Okay, I'll use:

Use sercon_puts() in both cases, to be consistent between dumping the
buffer when switching debugtrace to console output and when printing
a debugtrace entry directly to console.


Juergen

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

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

[Juergen Gross]

On 03.09.19 12:16, Jan Beulich wrote:
> On 28.08.2019 10:00, 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.
>>
>> 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>
>> ---
>>   xen/common/debugtrace.c | 62 ++++++++++++++++++++++++++++---------------------
>>   1 file changed, 35 insertions(+), 27 deletions(-)
>>
>> diff --git a/xen/common/debugtrace.c b/xen/common/debugtrace.c
>> index a2fab0d72c..7a9e4fb99f 100644
>> --- a/xen/common/debugtrace.c
>> +++ b/xen/common/debugtrace.c
>> @@ -15,33 +15,39 @@
>>   #include <xen/watchdog.h>
>>   
>>   /* 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_s {
> 
> Is the _s suffix really needed for some reason?

No, not really. I can drop it.

> 
>> +    unsigned long bytes; /* Size of buffer. */
>> +    unsigned long prd;   /* Producer index. */
> 
> Seeing that you switch from int to long here - are you really
> fancying these buffers to go beyond 4GiB in size?

I didn't want to rule it out on a multi-TB machine. :-)

> 
>> +    char          buf[];
>> +};
>> +
>> +static struct debugtrace_data_s *debtr_data;
> 
> How about s/debtr/dt/ or s/debtr/debugtrace/ ?

Yes, dt seems to be fine.

> 
>> +static unsigned int debugtrace_kilobytes = 128;
> 
> Since you touch this anyway, add __initdata? Maybe also move it
> next to its integer_param()?

This is modified in patch 4 again, and there I need it in the running
system for support of cpu hotplug with per-cpu buffers.

> 
>> +static bool debugtrace_used;
>>   static DEFINE_SPINLOCK(debugtrace_lock);
>>   integer_param("debugtrace", debugtrace_kilobytes);
>>   
>>   static void debugtrace_dump_worker(void)
>>   {
>> -    if ( (debugtrace_bytes == 0) || !debugtrace_used )
>> +    if ( !debtr_data || !debugtrace_used )
>>           return;
> 
> Why don't you get rid of the left side of the || altogether?

In patch 4 I do. :-)

I can drop it here already.

> 
>> @@ -165,12 +171,14 @@ 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);
>> +    memset(data, '\0', bytes);
>>   
>> -    debugtrace_bytes = bytes;
>> +    data->bytes = bytes - sizeof(*data);
>> +    debtr_data = data;
> 
> The allocation may end up being almost twice as big as what gets
> actually used this way. Wouldn't it make sense to re-calculate
> "bytes" from "order"?

Yes, you are right.


Juergen

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

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

[Jan Beulich]

On 28.08.2019 10:00, Juergen Gross wrote:
> @@ -24,32 +25,62 @@ struct debugtrace_data_s {
>  };
>  
>  static struct debugtrace_data_s *debtr_data;
> +static DEFINE_PER_CPU(struct debugtrace_data_s *, debtr_cpu_data);
>  
> -static unsigned int debugtrace_kilobytes = 128;
> +static unsigned int debugtrace_bytes = 128 << 10;

And after patch 3 this is now left as "unsigned int"?

> +static bool debugtrace_per_cpu;
>  static bool debugtrace_used;
>  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 ( !strncmp(s, "cpu:", 4) )
> +    {
> +        debugtrace_per_cpu = true;
> +        s += 4;
> +    }
> +    debugtrace_bytes =  parse_size_and_unit(s, NULL);

Stray double blank.

> +    return 0;
> +}
> +custom_param("debugtrace", debugtrace_parse_param);
> +
> +static void debugtrace_dump_buffer(struct debugtrace_data_s *data,
> +                                   const char *which)
>  {
> -    if ( !debtr_data || !debugtrace_used )
> +    if ( !data )
>          return;
>  
> -    printk("debugtrace_dump() starting\n");
> +    printk("debugtrace_dump() %s buffer starting\n", which);
>  
>      /* Print oldest portion of the ring. */
> -    ASSERT(debtr_data->buf[debtr_data->bytes - 1] == 0);
> -    if ( debtr_data->buf[debtr_data->prd] != '\0' )
> -        console_serial_puts(&debtr_data->buf[debtr_data->prd],
> -                            debtr_data->bytes - debtr_data->prd - 1);
> +    ASSERT(data->buf[data->bytes - 1] == 0);
> +    if ( data->buf[data->prd] != '\0' )
> +        console_serial_puts(&data->buf[data->prd], data->bytes - data->prd - 1);

Seeing this getting changed yet another time I now really wonder if
this nul termination is really still needed now that a size is being
passed into the actual output function. If you got rid of this in an
early prereq patch, the subsequent (to that one) ones would shrink.

Furthermore I can't help thinking that said change to pass the size
into the actual output functions actually broke the logic here: By
memset()-ing the buffer to zero, output on a subsequent invocation
would have been suitably truncated (in fact, until prd had wrapped,
I think it would have got truncated more than intended). Julien,
can you please look into this apparent regression?

> @@ -156,33 +195,70 @@ static void debugtrace_key(unsigned char key)
>      debugtrace_toggle();
>  }
>  
> -static int __init debugtrace_init(void)
> +static void debugtrace_alloc_buffer(struct debugtrace_data_s **ptr)
>  {
>      int order;
> -    unsigned long kbytes, bytes;
>      struct debugtrace_data_s *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 < PAGE_SIZE || *ptr )

Why the check against PAGE_SIZE?

> +        return;
>  
> -    order = get_order_from_bytes(bytes);
> +    order = get_order_from_bytes(debugtrace_bytes);
>      data = alloc_xenheap_pages(order, 0);
>      if ( !data )
> -        return -ENOMEM;
> +    {
> +        printk("failed to allocate debugtrace buffer\n");

Perhaps better to also indicate which/whose buffer?

> +        return;
> +    }
> +
> +    memset(data, '\0', debugtrace_bytes);
> +    data->bytes = debugtrace_bytes - sizeof(*data);
> +
> +    *ptr = data;
> +}
> +
> +static int debugtrace_cpu_callback(struct notifier_block *nfb,
> +                                   unsigned long action, void *hcpu)
> +{
> +    unsigned int cpu = (unsigned long)hcpu;
> +
> +    /* Buffers are only ever allocated, never freed. */
> +    switch ( action )
> +    {
> +    case CPU_UP_PREPARE:
> +        debugtrace_alloc_buffer(&per_cpu(debtr_cpu_data, cpu));
> +        break;
> +    default:
> +        break;

There no apparent need for "default:" here; quite possibly this
could be if() instead of switch() anyway.

Jan

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

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

[Juergen Gross]

On 03.09.19 12:51, Jan Beulich wrote:
> On 28.08.2019 10:00, Juergen Gross wrote:
>> @@ -24,32 +25,62 @@ struct debugtrace_data_s {
>>   };
>>   
>>   static struct debugtrace_data_s *debtr_data;
>> +static DEFINE_PER_CPU(struct debugtrace_data_s *, debtr_cpu_data);
>>   
>> -static unsigned int debugtrace_kilobytes = 128;
>> +static unsigned int debugtrace_bytes = 128 << 10;
> 
> And after patch 3 this is now left as "unsigned int"?

Good catch. :-)

> 
>> +static bool debugtrace_per_cpu;
>>   static bool debugtrace_used;
>>   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 ( !strncmp(s, "cpu:", 4) )
>> +    {
>> +        debugtrace_per_cpu = true;
>> +        s += 4;
>> +    }
>> +    debugtrace_bytes =  parse_size_and_unit(s, NULL);
> 
> Stray double blank.

Yes. Will remove one.

> 
>> +    return 0;
>> +}
>> +custom_param("debugtrace", debugtrace_parse_param);
>> +
>> +static void debugtrace_dump_buffer(struct debugtrace_data_s *data,
>> +                                   const char *which)
>>   {
>> -    if ( !debtr_data || !debugtrace_used )
>> +    if ( !data )
>>           return;
>>   
>> -    printk("debugtrace_dump() starting\n");
>> +    printk("debugtrace_dump() %s buffer starting\n", which);
>>   
>>       /* Print oldest portion of the ring. */
>> -    ASSERT(debtr_data->buf[debtr_data->bytes - 1] == 0);
>> -    if ( debtr_data->buf[debtr_data->prd] != '\0' )
>> -        console_serial_puts(&debtr_data->buf[debtr_data->prd],
>> -                            debtr_data->bytes - debtr_data->prd - 1);
>> +    ASSERT(data->buf[data->bytes - 1] == 0);
>> +    if ( data->buf[data->prd] != '\0' )
>> +        console_serial_puts(&data->buf[data->prd], data->bytes - data->prd - 1);
> 
> Seeing this getting changed yet another time I now really wonder if
> this nul termination is really still needed now that a size is being
> passed into the actual output function. If you got rid of this in an
> early prereq patch, the subsequent (to that one) ones would shrink.

Yes.

> 
> Furthermore I can't help thinking that said change to pass the size
> into the actual output functions actually broke the logic here: By
> memset()-ing the buffer to zero, output on a subsequent invocation
> would have been suitably truncated (in fact, until prd had wrapped,
> I think it would have got truncated more than intended). Julien,
> can you please look into this apparent regression?

I can do that. Resetting prd to 0 when clearing the buffer is
required here.

> 
>> @@ -156,33 +195,70 @@ static void debugtrace_key(unsigned char key)
>>       debugtrace_toggle();
>>   }
>>   
>> -static int __init debugtrace_init(void)
>> +static void debugtrace_alloc_buffer(struct debugtrace_data_s **ptr)
>>   {
>>       int order;
>> -    unsigned long kbytes, bytes;
>>       struct debugtrace_data_s *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 < PAGE_SIZE || *ptr )
> 
> Why the check against PAGE_SIZE?

With recaclulating debugtrace_bytes this can be dropped.

> 
>> +        return;
>>   
>> -    order = get_order_from_bytes(bytes);
>> +    order = get_order_from_bytes(debugtrace_bytes);
>>       data = alloc_xenheap_pages(order, 0);
>>       if ( !data )
>> -        return -ENOMEM;
>> +    {
>> +        printk("failed to allocate debugtrace buffer\n");
> 
> Perhaps better to also indicate which/whose buffer?

Hmm, I'm not sure this is really required. I can add it if you want, but
as a user of debugtrace I'd be only interested in the information
whether I can expect all trace entries to be seen or not.

> 
>> +        return;
>> +    }
>> +
>> +    memset(data, '\0', debugtrace_bytes);
>> +    data->bytes = debugtrace_bytes - sizeof(*data);
>> +
>> +    *ptr = data;
>> +}
>> +
>> +static int debugtrace_cpu_callback(struct notifier_block *nfb,
>> +                                   unsigned long action, void *hcpu)
>> +{
>> +    unsigned int cpu = (unsigned long)hcpu;
>> +
>> +    /* Buffers are only ever allocated, never freed. */
>> +    switch ( action )
>> +    {
>> +    case CPU_UP_PREPARE:
>> +        debugtrace_alloc_buffer(&per_cpu(debtr_cpu_data, cpu));
>> +        break;
>> +    default:
>> +        break;
> 
> There no apparent need for "default:" here; quite possibly this
> could be if() instead of switch() anyway.

Fine with me.


Juergen


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

Re: [Xen-devel] [PATCH v3 1/4] xen: use common output function in debugtrace

[Jan Beulich]

On 03.09.2019 12:22, Juergen Gross wrote:
> On 03.09.19 12:00, Jan Beulich wrote:
>> On 28.08.2019 10:00, Juergen Gross wrote:
>>> Today dumping the debugtrace buffers is done via sercon_puts(), while
>>> direct printing of trace entries (after toggling output to the console)
>>> is using serial_puts().
>>>
>>> Use sercon_puts() in both cases, as the difference between both is not
>>> really making sense.
>>
>> No matter that I like this change, I'm not convinced it's correct:
>> There are two differences between the functions, neither of which
>> I could qualify as "not really making sense": Why is it obvious
>> that it makes no sense for the debugtrace output to bypass one or
>> both of serial_steal_fn() and pv_console_puts()? If you're
>> convinced of this, please provide the "why"-s behind the sentence
>> above.
> 
> Okay, I'll use:
> 
> Use sercon_puts() in both cases, to be consistent between dumping the
> buffer when switching debugtrace to console output and when printing
> a debugtrace entry directly to console.

Well, this is better as an explanation indeed, but it still doesn't
make clear whether it wasn't in fact wanted for there to be a
difference in where output gets sent. I may believe that bypassing
pv_console_puts() wasn't intended, but the steal function bypass
had been there in 3.2 already, so may well have been on purpose.

Jan

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

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

[Jan Beulich]

On 03.09.2019 12:31, Juergen Gross wrote:
> On 03.09.19 12:16, Jan Beulich wrote:
>> On 28.08.2019 10:00, Juergen Gross wrote:
>>> +static unsigned int debugtrace_kilobytes = 128;
>>
>> Since you touch this anyway, add __initdata? Maybe also move it
>> next to its integer_param()?
> 
> This is modified in patch 4 again, and there I need it in the running
> system for support of cpu hotplug with per-cpu buffers.

Right, I've meanwhile noticed this. Hence it's fine to keep as is.

>>> @@ -165,12 +171,14 @@ 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);
>>> +    memset(data, '\0', bytes);
>>>   
>>> -    debugtrace_bytes = bytes;
>>> +    data->bytes = bytes - sizeof(*data);
>>> +    debtr_data = data;
>>
>> The allocation may end up being almost twice as big as what gets
>> actually used this way. Wouldn't it make sense to re-calculate
>> "bytes" from "order"?
> 
> Yes, you are right.

Actually I wasn't, which I did notice seeing the relevant piece
of code getting touched in patch 4:

    while ( (kbytes = (debugtrace_kilobytes & (debugtrace_kilobytes-1))) != 0 )
        debugtrace_kilobytes = kbytes;

Jan

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

Re: [Xen-devel] [PATCH v3 1/4] xen: use common output function in debugtrace

[Juergen Gross]

On 03.09.19 13:47, Jan Beulich wrote:
> On 03.09.2019 12:22, Juergen Gross wrote:
>> On 03.09.19 12:00, Jan Beulich wrote:
>>> On 28.08.2019 10:00, Juergen Gross wrote:
>>>> Today dumping the debugtrace buffers is done via sercon_puts(), while
>>>> direct printing of trace entries (after toggling output to the console)
>>>> is using serial_puts().
>>>>
>>>> Use sercon_puts() in both cases, as the difference between both is not
>>>> really making sense.
>>>
>>> No matter that I like this change, I'm not convinced it's correct:
>>> There are two differences between the functions, neither of which
>>> I could qualify as "not really making sense": Why is it obvious
>>> that it makes no sense for the debugtrace output to bypass one or
>>> both of serial_steal_fn() and pv_console_puts()? If you're
>>> convinced of this, please provide the "why"-s behind the sentence
>>> above.
>>
>> Okay, I'll use:
>>
>> Use sercon_puts() in both cases, to be consistent between dumping the
>> buffer when switching debugtrace to console output and when printing
>> a debugtrace entry directly to console.
> 
> Well, this is better as an explanation indeed, but it still doesn't
> make clear whether it wasn't in fact wanted for there to be a
> difference in where output gets sent. I may believe that bypassing
> pv_console_puts() wasn't intended, but the steal function bypass
> had been there in 3.2 already, so may well have been on purpose.

There are two users of console_steal():

suspend handling - I believe it is a good idea to not use the serial
interface while it is potentially uninitialized.

gdb support - Why should that be special? Not treating the output data
appropriate to the attached debugger will be worse than encapsulating
it in a way the debugger can handle.


Juergen

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

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

[Jan Beulich]

On 03.09.2019 13:10, Juergen Gross wrote:
> On 03.09.19 12:51, Jan Beulich wrote:
>> On 28.08.2019 10:00, Juergen Gross wrote:
>>> +static void debugtrace_dump_buffer(struct debugtrace_data_s *data,
>>> +                                   const char *which)
>>>   {
>>> -    if ( !debtr_data || !debugtrace_used )
>>> +    if ( !data )
>>>           return;
>>>   
>>> -    printk("debugtrace_dump() starting\n");
>>> +    printk("debugtrace_dump() %s buffer starting\n", which);
>>>   
>>>       /* Print oldest portion of the ring. */
>>> -    ASSERT(debtr_data->buf[debtr_data->bytes - 1] == 0);
>>> -    if ( debtr_data->buf[debtr_data->prd] != '\0' )
>>> -        console_serial_puts(&debtr_data->buf[debtr_data->prd],
>>> -                            debtr_data->bytes - debtr_data->prd - 1);
>>> +    ASSERT(data->buf[data->bytes - 1] == 0);
>>> +    if ( data->buf[data->prd] != '\0' )
>>> +        console_serial_puts(&data->buf[data->prd], data->bytes - data->prd - 1);
>>
>> Seeing this getting changed yet another time I now really wonder if
>> this nul termination is really still needed now that a size is being
>> passed into the actual output function. If you got rid of this in an
>> early prereq patch, the subsequent (to that one) ones would shrink.
> 
> Yes.
> 
>>
>> Furthermore I can't help thinking that said change to pass the size
>> into the actual output functions actually broke the logic here: By
>> memset()-ing the buffer to zero, output on a subsequent invocation
>> would have been suitably truncated (in fact, until prd had wrapped,
>> I think it would have got truncated more than intended). Julien,
>> can you please look into this apparent regression?
> 
> I can do that. Resetting prd to 0 when clearing the buffer is
> required here.

I'm afraid it's not this simple: Doing so will confuse
debugtrace_printk() - consider the function then storing the
previously latched last_prd into debugtrace_prd.

>>> -    order = get_order_from_bytes(bytes);
>>> +    order = get_order_from_bytes(debugtrace_bytes);
>>>       data = alloc_xenheap_pages(order, 0);
>>>       if ( !data )
>>> -        return -ENOMEM;
>>> +    {
>>> +        printk("failed to allocate debugtrace buffer\n");
>>
>> Perhaps better to also indicate which/whose buffer?
> 
> Hmm, I'm not sure this is really required. I can add it if you want, but
> as a user of debugtrace I'd be only interested in the information
> whether I can expect all trace entries to be seen or not.

Well, if the allocation fails for a CPU, it's not impossible for
the CPU bringup to then also fail. Subsequent to this the system
would then still provide an almost complete set of debugtrace
entries (ones issued by subsequent bringup actions would be
missing), _despite_ this log message.

Jan

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

Re: [Xen-devel] [PATCH v3 1/4] xen: use common output function in debugtrace

[Jan Beulich]

On 03.09.2019 13:58, Juergen Gross wrote:
> On 03.09.19 13:47, Jan Beulich wrote:
>> On 03.09.2019 12:22, Juergen Gross wrote:
>>> On 03.09.19 12:00, Jan Beulich wrote:
>>>> On 28.08.2019 10:00, Juergen Gross wrote:
>>>>> Today dumping the debugtrace buffers is done via sercon_puts(), while
>>>>> direct printing of trace entries (after toggling output to the console)
>>>>> is using serial_puts().
>>>>>
>>>>> Use sercon_puts() in both cases, as the difference between both is not
>>>>> really making sense.
>>>>
>>>> No matter that I like this change, I'm not convinced it's correct:
>>>> There are two differences between the functions, neither of which
>>>> I could qualify as "not really making sense": Why is it obvious
>>>> that it makes no sense for the debugtrace output to bypass one or
>>>> both of serial_steal_fn() and pv_console_puts()? If you're
>>>> convinced of this, please provide the "why"-s behind the sentence
>>>> above.
>>>
>>> Okay, I'll use:
>>>
>>> Use sercon_puts() in both cases, to be consistent between dumping the
>>> buffer when switching debugtrace to console output and when printing
>>> a debugtrace entry directly to console.
>>
>> Well, this is better as an explanation indeed, but it still doesn't
>> make clear whether it wasn't in fact wanted for there to be a
>> difference in where output gets sent. I may believe that bypassing
>> pv_console_puts() wasn't intended, but the steal function bypass
>> had been there in 3.2 already, so may well have been on purpose.
> 
> There are two users of console_steal():
> 
> suspend handling - I believe it is a good idea to not use the serial
> interface while it is potentially uninitialized.

I agree here.

> gdb support - Why should that be special? Not treating the output data
> appropriate to the attached debugger will be worse than encapsulating
> it in a way the debugger can handle.

I'm not sure here - it may well have been intentional to avoid
cluttering the debugger console.

Jan

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

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

[Juergen Gross]

On 03.09.19 14:01, Jan Beulich wrote:
> On 03.09.2019 13:10, Juergen Gross wrote:
>> On 03.09.19 12:51, Jan Beulich wrote:
>>> On 28.08.2019 10:00, Juergen Gross wrote:
>>>> +static void debugtrace_dump_buffer(struct debugtrace_data_s *data,
>>>> +                                   const char *which)
>>>>    {
>>>> -    if ( !debtr_data || !debugtrace_used )
>>>> +    if ( !data )
>>>>            return;
>>>>    
>>>> -    printk("debugtrace_dump() starting\n");
>>>> +    printk("debugtrace_dump() %s buffer starting\n", which);
>>>>    
>>>>        /* Print oldest portion of the ring. */
>>>> -    ASSERT(debtr_data->buf[debtr_data->bytes - 1] == 0);
>>>> -    if ( debtr_data->buf[debtr_data->prd] != '\0' )
>>>> -        console_serial_puts(&debtr_data->buf[debtr_data->prd],
>>>> -                            debtr_data->bytes - debtr_data->prd - 1);
>>>> +    ASSERT(data->buf[data->bytes - 1] == 0);
>>>> +    if ( data->buf[data->prd] != '\0' )
>>>> +        console_serial_puts(&data->buf[data->prd], data->bytes - data->prd - 1);
>>>
>>> Seeing this getting changed yet another time I now really wonder if
>>> this nul termination is really still needed now that a size is being
>>> passed into the actual output function. If you got rid of this in an
>>> early prereq patch, the subsequent (to that one) ones would shrink.
>>
>> Yes.
>>
>>>
>>> Furthermore I can't help thinking that said change to pass the size
>>> into the actual output functions actually broke the logic here: By
>>> memset()-ing the buffer to zero, output on a subsequent invocation
>>> would have been suitably truncated (in fact, until prd had wrapped,
>>> I think it would have got truncated more than intended). Julien,
>>> can you please look into this apparent regression?
>>
>> I can do that. Resetting prd to 0 when clearing the buffer is
>> required here.
> 
> I'm afraid it's not this simple: Doing so will confuse
> debugtrace_printk() - consider the function then storing the
> previously latched last_prd into debugtrace_prd.

Sure, this has to be handled (and it is still easy to do so).

> 
>>>> -    order = get_order_from_bytes(bytes);
>>>> +    order = get_order_from_bytes(debugtrace_bytes);
>>>>        data = alloc_xenheap_pages(order, 0);
>>>>        if ( !data )
>>>> -        return -ENOMEM;
>>>> +    {
>>>> +        printk("failed to allocate debugtrace buffer\n");
>>>
>>> Perhaps better to also indicate which/whose buffer?
>>
>> Hmm, I'm not sure this is really required. I can add it if you want, but
>> as a user of debugtrace I'd be only interested in the information
>> whether I can expect all trace entries to be seen or not.
> 
> Well, if the allocation fails for a CPU, it's not impossible for
> the CPU bringup to then also fail. Subsequent to this the system
> would then still provide an almost complete set of debugtrace
> entries (ones issued by subsequent bringup actions would be
> missing), _despite_ this log message.

You seem to want the information, so I can add it.


Juergen

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

Re: [Xen-devel] [PATCH v3 1/4] xen: use common output function in debugtrace

[Juergen Gross]

On 03.09.19 14:09, Jan Beulich wrote:
> On 03.09.2019 13:58, Juergen Gross wrote:
>> On 03.09.19 13:47, Jan Beulich wrote:
>>> On 03.09.2019 12:22, Juergen Gross wrote:
>>>> On 03.09.19 12:00, Jan Beulich wrote:
>>>>> On 28.08.2019 10:00, Juergen Gross wrote:
>>>>>> Today dumping the debugtrace buffers is done via sercon_puts(), while
>>>>>> direct printing of trace entries (after toggling output to the console)
>>>>>> is using serial_puts().
>>>>>>
>>>>>> Use sercon_puts() in both cases, as the difference between both is not
>>>>>> really making sense.
>>>>>
>>>>> No matter that I like this change, I'm not convinced it's correct:
>>>>> There are two differences between the functions, neither of which
>>>>> I could qualify as "not really making sense": Why is it obvious
>>>>> that it makes no sense for the debugtrace output to bypass one or
>>>>> both of serial_steal_fn() and pv_console_puts()? If you're
>>>>> convinced of this, please provide the "why"-s behind the sentence
>>>>> above.
>>>>
>>>> Okay, I'll use:
>>>>
>>>> Use sercon_puts() in both cases, to be consistent between dumping the
>>>> buffer when switching debugtrace to console output and when printing
>>>> a debugtrace entry directly to console.
>>>
>>> Well, this is better as an explanation indeed, but it still doesn't
>>> make clear whether it wasn't in fact wanted for there to be a
>>> difference in where output gets sent. I may believe that bypassing
>>> pv_console_puts() wasn't intended, but the steal function bypass
>>> had been there in 3.2 already, so may well have been on purpose.
>>
>> There are two users of console_steal():
>>
>> suspend handling - I believe it is a good idea to not use the serial
>> interface while it is potentially uninitialized.
> 
> I agree here.
> 
>> gdb support - Why should that be special? Not treating the output data
>> appropriate to the attached debugger will be worse than encapsulating
>> it in a way the debugger can handle.
> 
> I'm not sure here - it may well have been intentional to avoid
> cluttering the debugger console.

But won't using serial_puts() clutter the debugger console? "normal"
printk() output seem to be handled in a special way with gdb active,
while just using serial_puts() with attached gdb is looking at least
suspicious to me. There seems to be a special format, e.g. text output
of the hypervisor is prepended with 'O' and the data is sent in hex
representation. I can't imagine just sending some ASCII text is the
desired approach for debugtrace output.


Juergen


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

Re: [Xen-devel] [PATCH v3 1/4] xen: use common output function in debugtrace

[Jan Beulich]

On 03.09.2019 14:22, Juergen Gross wrote:
> On 03.09.19 14:09, Jan Beulich wrote:
>> On 03.09.2019 13:58, Juergen Gross wrote:
>>> On 03.09.19 13:47, Jan Beulich wrote:
>>>> On 03.09.2019 12:22, Juergen Gross wrote:
>>>>> On 03.09.19 12:00, Jan Beulich wrote:
>>>>>> On 28.08.2019 10:00, Juergen Gross wrote:
>>>>>>> Today dumping the debugtrace buffers is done via sercon_puts(), while
>>>>>>> direct printing of trace entries (after toggling output to the console)
>>>>>>> is using serial_puts().
>>>>>>>
>>>>>>> Use sercon_puts() in both cases, as the difference between both is not
>>>>>>> really making sense.
>>>>>>
>>>>>> No matter that I like this change, I'm not convinced it's correct:
>>>>>> There are two differences between the functions, neither of which
>>>>>> I could qualify as "not really making sense": Why is it obvious
>>>>>> that it makes no sense for the debugtrace output to bypass one or
>>>>>> both of serial_steal_fn() and pv_console_puts()? If you're
>>>>>> convinced of this, please provide the "why"-s behind the sentence
>>>>>> above.
>>>>>
>>>>> Okay, I'll use:
>>>>>
>>>>> Use sercon_puts() in both cases, to be consistent between dumping the
>>>>> buffer when switching debugtrace to console output and when printing
>>>>> a debugtrace entry directly to console.
>>>>
>>>> Well, this is better as an explanation indeed, but it still doesn't
>>>> make clear whether it wasn't in fact wanted for there to be a
>>>> difference in where output gets sent. I may believe that bypassing
>>>> pv_console_puts() wasn't intended, but the steal function bypass
>>>> had been there in 3.2 already, so may well have been on purpose.
>>>
>>> There are two users of console_steal():
>>>
>>> suspend handling - I believe it is a good idea to not use the serial
>>> interface while it is potentially uninitialized.
>>
>> I agree here.
>>
>>> gdb support - Why should that be special? Not treating the output data
>>> appropriate to the attached debugger will be worse than encapsulating
>>> it in a way the debugger can handle.
>>
>> I'm not sure here - it may well have been intentional to avoid
>> cluttering the debugger console.
> 
> But won't using serial_puts() clutter the debugger console? "normal"
> printk() output seem to be handled in a special way with gdb active,
> while just using serial_puts() with attached gdb is looking at least
> suspicious to me. There seems to be a special format, e.g. text output
> of the hypervisor is prepended with 'O' and the data is sent in hex
> representation. I can't imagine just sending some ASCII text is the
> desired approach for debugtrace output.

Oh, did I get it backwards, I'm sorry. Yes, I agree. With the slightly
adjusted description
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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

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

[Juergen Gross]

On 03.09.19 13:50, Jan Beulich wrote:
> On 03.09.2019 12:31, Juergen Gross wrote:
>> On 03.09.19 12:16, Jan Beulich wrote:
>>> On 28.08.2019 10:00, Juergen Gross wrote:
>>>> +static unsigned int debugtrace_kilobytes = 128;
>>>
>>> Since you touch this anyway, add __initdata? Maybe also move it
>>> next to its integer_param()?
>>
>> This is modified in patch 4 again, and there I need it in the running
>> system for support of cpu hotplug with per-cpu buffers.
> 
> Right, I've meanwhile noticed this. Hence it's fine to keep as is.
> 
>>>> @@ -165,12 +171,14 @@ 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);
>>>> +    memset(data, '\0', bytes);
>>>>    
>>>> -    debugtrace_bytes = bytes;
>>>> +    data->bytes = bytes - sizeof(*data);
>>>> +    debtr_data = data;
>>>
>>> The allocation may end up being almost twice as big as what gets
>>> actually used this way. Wouldn't it make sense to re-calculate
>>> "bytes" from "order"?
>>
>> Yes, you are right.
> 
> Actually I wasn't, which I did notice seeing the relevant piece
> of code getting touched in patch 4:
> 
>      while ( (kbytes = (debugtrace_kilobytes & (debugtrace_kilobytes-1))) != 0 )
>          debugtrace_kilobytes = kbytes;

For kbytes < 4 you still were right.


Juergen


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