Re: [Xen-devel] [PATCH 1/3] xen: move debugtrace coding to common/debugtrace.c

[Jan Beulich <JBeulich@xxxxxxxx>]

On 28.07.2019 10:40, Juergen Gross wrote:
> @@ -1155,178 +1155,6 @@ int printk_ratelimit(void)
>       return __printk_ratelimit(printk_ratelimit_ms, printk_ratelimit_burst);
>   }
>   
> -/*
> - * **************************************************************
> - * *************** Serial console ring buffer *******************
> - * **************************************************************
> - */

I acknowledge that this comment has at best been displaced from what
it would properly belong to, so is indeed perhaps best dropped. But ...

> -#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);
> -    sercon_puts(&debugtrace_buf[debugtrace_prd]);
> -
> -    /* Print youngest portion of the ring. */
> -    debugtrace_buf[debugtrace_prd] = '\0';
> -    sercon_puts(&debugtrace_buf[0]);
> -
> -    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;
> -
> -    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);
> -    vsnprintf(buf, sizeof(buf), fmt, args);
> -    va_end(args);
> -
> -    if ( debugtrace_send_to_console )
> -    {
> -        snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);
> -        serial_puts(sercon_handle, cntbuf);
> -        serial_puts(sercon_handle, buf);
> -    }
> -    else
> -    {
> -        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 ***************

... what about this one? There's only panic() between it and the next
such comment, and I don't think the "Debugging/tracing" part of it
are applicable (anymore).

> --- a/xen/include/xen/console.h
> +++ b/xen/include/xen/console.h
> @@ -48,4 +48,8 @@ int console_resume(void);
>   
>   extern int8_t opt_console_xen;
>   
> +/* Issue string via serial line. */
> +extern int sercon_handle;
> +void sercon_puts(const char *s);

I guess avoiding their exposure was one of the reasons the debug trace
code lived in the place you move it from. I'm unconvinced non-console
code is actually supposed to make use of either, but I'm not opposed
enough to nak the change. I don't think though the comment fits well
with the variable declaration.

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