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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel