Re: [PATCH 2 of 3] xentrace: use tbuf_size for overflow check

[George Dunlap <george.dunlap@citrix.com>]

On Wed, 2011-03-30 at 19:04 +0100, Olaf Hering wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@aepfle.de>
> # Date 1301424075 -7200
> # Node ID 3e95e737bc51c2295926e4f23389b1cb161d6d7b
> # Parent  8a2ce5e49b2c5f2e013734b5d53eae37572f4101
> xentrace: use tbuf_size for overflow check
> 
> The calculated number of per-cpu trace pages is stored in t_info and
> shared with tools like xentrace. Since its an u16 the value may overflow
> because the current check is based on u32.

Hmm -- while this is true, it's possible this may change in the future.
If we ever changed t_info.tbuf_size to be u32, then t_buf.prod/cons
would again be the limiting factor.

Should we perhaps add both checks?

> Using the u16 means each cpu could in theory use up to 256MB as trace
> buffer. However such a large allocation will currently fail on x86 due
> to the MAX_ORDER limit.

FWIW, I don't believe that there's any reason the allocations have to be
contiguous any more.  I kept them contiguous to minimize the changes to
the moving parts near a release.  But the new system has been pretty
well tested now, so I think looking at non-contiguous allocations may be
worthwhile.

> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> diff -r 8a2ce5e49b2c -r 3e95e737bc51 xen/common/trace.c
> --- a/xen/common/trace.c	Tue Mar 29 20:37:20 2011 +0200
> +++ b/xen/common/trace.c	Tue Mar 29 20:41:15 2011 +0200
> @@ -104,25 +104,26 @@
>   * calculate_tbuf_size - check to make sure that the proposed size will fit
>   * in the currently sized struct t_info and allows prod and cons to
>   * reach double the value without overflow.
> + * tbuf_size is u16, while t_buf prod/cons are u32, so using tbuf_size for
> + * overflow check is good.
> + * The t_info layout is fixed and cant be changed without breaking xentrace.
>   * Initialize t_info_pages based on number of trace pages.
>   */
>  static int calculate_tbuf_size(unsigned int pages)
>  {
> -    struct t_buf dummy;
> -    typeof(dummy.prod) size;
> +    struct t_info dummy;
> +    typeof(dummy.tbuf_size) max_pages;
>      unsigned int t_info_words;
>  
>      /* force maximum value for an unsigned type */
> -    size = -1;
> +    max_pages = -1;
>  
> -    /* max size holds up to n pages */
> -    size /= PAGE_SIZE;
> -    if ( pages > size )
> +    if ( pages > max_pages )
>      {
>          printk(XENLOG_INFO "xentrace: requested number of %u pages "
>                 "reduced to %u\n",
> -               pages, (unsigned int)size);
> -        pages = size;
> +               pages, max_pages);
> +        pages = max_pages;
>      }
>  
>      t_info_words = num_online_cpus() * pages * sizeof(uint32_t);