[PATCH 0 of 5] xentrace updates

[PATCH 0 of 5] xentrace updates

[Olaf Hering]

The previous change for dynamic tracebuffer allocation had a bug where the
default buffer size would lead to an allocation failure.
Sorry for that, I tested only large buffer sizes.
The following patches update the printk in trace.c as well.

Olaf

[PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case

[Olaf Hering]

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1300813685 -3600
# Node ID 08df96398bff82a8924a37eda6ddffd1ada3f407
# Parent  c81f0ef5a77d90fbf108d3efe489d08df45b63c2
xentrace: fix t_info_pages calculation for the default case

The default tracebuffer size of 32 pages was not tested with the previous patch.
As a result, t_info_pages will become zero and alloc_xenheap_pages() fails.
Catch this case and allocate at least one page.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r c81f0ef5a77d -r 08df96398bff xen/common/trace.c
--- a/xen/common/trace.c	Mon Mar 21 14:52:27 2011 +0000
+++ b/xen/common/trace.c	Tue Mar 22 18:08:05 2011 +0100
@@ -125,7 +125,7 @@
     t_info_pages = num_online_cpus() * pages + t_info_first_offset;
     t_info_pages *= sizeof(uint32_t);
     t_info_pages /= PAGE_SIZE;
-    if ( t_info_pages % PAGE_SIZE )
+    if ( t_info_pages % PAGE_SIZE || t_info_pages == 0 )
         t_info_pages++;
     return pages;
 }

[PATCH 2 of 5] xentrace: print calculated numbers in calculate_tbuf_size()

[Olaf Hering]

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1300813690 -3600
# Node ID d61854ab2758f691818678c2d03e18e0e165802a
# Parent  08df96398bff82a8924a37eda6ddffd1ada3f407
xentrace: print calculated numbers in calculate_tbuf_size()

Print number of pages to allocate for per-cpu tracebuffer and metadata
to ease debugging when allocation fails.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r 08df96398bff -r d61854ab2758 xen/common/trace.c
--- a/xen/common/trace.c	Tue Mar 22 18:08:05 2011 +0100
+++ b/xen/common/trace.c	Tue Mar 22 18:08:10 2011 +0100
@@ -127,6 +127,8 @@
     t_info_pages /= PAGE_SIZE;
     if ( t_info_pages % PAGE_SIZE || t_info_pages == 0 )
         t_info_pages++;
+    printk(XENLOG_INFO "Xen trace buffers: requesting %u t_info pages for %u trace pages on %u cpus\n",
+               t_info_pages, pages, num_online_cpus());
     return pages;
 }

[PATCH 3 of 5] xentrace: remove gdprintk usage since they are not in guest context

[Olaf Hering]

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1300813695 -3600
# Node ID 364ccfbfb31f3280a5ac32f8f50cf5c84087eb39
# Parent  d61854ab2758f691818678c2d03e18e0e165802a
xentrace: remove gdprintk usage since they are not in guest context

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r d61854ab2758 -r 364ccfbfb31f xen/common/trace.c
--- a/xen/common/trace.c	Tue Mar 22 18:08:10 2011 +0100
+++ b/xen/common/trace.c	Tue Mar 22 18:08:15 2011 +0100
@@ -117,7 +117,7 @@
     size /= PAGE_SIZE;
     if ( pages > size )
     {
-        gdprintk(XENLOG_INFO, "%s: requested number of %u pages reduced to %u\n",
+        printk(XENLOG_INFO "%s: requested number of %u pages reduced to %u\n",
                __func__, pages, (unsigned int)size);
         pages = size;
     }
@@ -265,7 +265,7 @@
      */
     if ( opt_tbuf_size && pages != opt_tbuf_size )
     {
-        gdprintk(XENLOG_INFO, "tb_set_size from %d to %d not implemented\n",
+        printk(XENLOG_INFO "tb_set_size from %d to %d not implemented\n",
                      opt_tbuf_size, pages);
         return -EINVAL;
     }
@@ -310,7 +310,7 @@
 {
     if ( opt_tbuf_size && alloc_trace_bufs(opt_tbuf_size) )
     {
-        gdprintk(XENLOG_INFO, "Xen trace buffers: "
+        printk(XENLOG_INFO "Xen trace buffers: "
                  "allocation size %d failed, disabling\n",
                  opt_tbuf_size);
         opt_tbuf_size = 0;

[PATCH 4 of 5] xentrace: update comments

[Olaf Hering]

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1300813699 -3600
# Node ID e58f6949e76a2786c4f5a99a0da44ee58743b4df
# Parent  364ccfbfb31f3280a5ac32f8f50cf5c84087eb39
xentrace: update comments

Fix a typo, remove redundant comment.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r 364ccfbfb31f -r e58f6949e76a xen/common/trace.c
--- a/xen/common/trace.c	Tue Mar 22 18:08:15 2011 +0100
+++ b/xen/common/trace.c	Tue Mar 22 18:08:19 2011 +0100
@@ -196,12 +196,11 @@
     t_info->tbuf_size = pages;
 
     /*
-     * Now share the pages to xentrace can map them, and write them in
+     * Now share the pages so xentrace can map them, and write them in
      * the global t_info structure.
      */
     for_each_online_cpu(cpu)
     {
-        /* Share pages so that xentrace can map them. */
         void *rawbuf = per_cpu(t_bufs, cpu);
         struct page_info *p = virt_to_page(rawbuf);
         uint32_t mfn = virt_to_mfn(rawbuf);

[PATCH 5 of 5] xentrace: use consistent printk prefix

[Olaf Hering]

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1300813820 -3600
# Node ID dcbae547cce81f10c243d54bd35fd139615313cb
# Parent  e58f6949e76a2786c4f5a99a0da44ee58743b4df
xentrace: use consistent printk prefix

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r e58f6949e76a -r dcbae547cce8 xen/common/trace.c
--- a/xen/common/trace.c	Tue Mar 22 18:08:19 2011 +0100
+++ b/xen/common/trace.c	Tue Mar 22 18:10:20 2011 +0100
@@ -117,7 +117,7 @@
     size /= PAGE_SIZE;
     if ( pages > size )
     {
-        printk(XENLOG_INFO "%s: requested number of %u pages reduced to %u\n",
+        printk(XENLOG_INFO "Xen trace buffers: %s: requested number of %u pages reduced to %u\n",
                __func__, pages, (unsigned int)size);
         pages = size;
     }
@@ -177,7 +177,7 @@
         if ( (rawbuf = alloc_xenheap_pages(
                 order, MEMF_bits(32 + PAGE_SHIFT))) == NULL )
         {
-            printk("Xen trace buffers: memory allocation failed on cpu %d\n", cpu);
+            printk(XENLOG_INFO "Xen trace buffers: memory allocation failed on cpu %d\n", cpu);
             goto out_dealloc;
         }
 
@@ -212,7 +212,7 @@
             t_info_mfn_list[offset + i]=mfn + i;
         }
         t_info->mfn_offset[cpu]=offset;
-        printk(XENLOG_INFO "p%d mfn %"PRIx32" offset %d\n",
+        printk(XENLOG_INFO "Xen trace buffers: p%d mfn %"PRIx32" offset %d\n",
                cpu, mfn, offset);
         offset+=i;
 
@@ -236,7 +236,7 @@
     {
         void *rawbuf = per_cpu(t_bufs, cpu);
         per_cpu(t_bufs, cpu) = NULL;
-        printk("Xen trace buffers: cpu %d p %p\n", cpu, rawbuf);
+        printk(XENLOG_DEBUG "Xen trace buffers: cpu %d p %p\n", cpu, rawbuf);
         if ( rawbuf )
         {
             ASSERT(!(virt_to_page(rawbuf)->count_info & PGC_allocated));
@@ -245,7 +245,7 @@
     }
     free_xenheap_pages(t_info, get_order_from_pages(t_info_pages));
     t_info = NULL;
-    printk("Xen trace buffers: allocation failed! Tracing disabled.\n");
+    printk(XENLOG_WARNING "Xen trace buffers: allocation failed! Tracing disabled.\n");
     return -ENOMEM;
 }
 
@@ -264,7 +264,7 @@
      */
     if ( opt_tbuf_size && pages != opt_tbuf_size )
     {
-        printk(XENLOG_INFO "tb_set_size from %d to %d not implemented\n",
+        printk(XENLOG_INFO "Xen trace buffers: tb_set_size from %d to %d not implemented\n",
                      opt_tbuf_size, pages);
         return -EINVAL;
     }

Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case

[Jan Beulich]

>>> On 22.03.11 at 20:21, Olaf Hering <olaf@aepfle.de> wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@aepfle.de>
> # Date 1300813685 -3600
> # Node ID 08df96398bff82a8924a37eda6ddffd1ada3f407
> # Parent  c81f0ef5a77d90fbf108d3efe489d08df45b63c2
> xentrace: fix t_info_pages calculation for the default case
> 
> The default tracebuffer size of 32 pages was not tested with the previous 
> patch.
> As a result, t_info_pages will become zero and alloc_xenheap_pages() fails.
> Catch this case and allocate at least one page.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> diff -r c81f0ef5a77d -r 08df96398bff xen/common/trace.c
> --- a/xen/common/trace.c	Mon Mar 21 14:52:27 2011 +0000
> +++ b/xen/common/trace.c	Tue Mar 22 18:08:05 2011 +0100
> @@ -125,7 +125,7 @@
>      t_info_pages = num_online_cpus() * pages + t_info_first_offset;
>      t_info_pages *= sizeof(uint32_t);
>      t_info_pages /= PAGE_SIZE;
> -    if ( t_info_pages % PAGE_SIZE )
> +    if ( t_info_pages % PAGE_SIZE || t_info_pages == 0 )

While certainly not having a significant effect, to the unsuspecting
reader this looks like a bug - is it really meant to be a remainder
operation on the *result* of a division (rather than on the original
dividend)? Couldn't you just (ab)use PFN_UP() here?

Jan

>          t_info_pages++;
>      return pages;
>  }

Re: [PATCH 5 of 5] xentrace: use consistent printk prefix

[Jan Beulich]

>>> On 22.03.11 at 20:21, Olaf Hering <olaf@aepfle.de> wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@aepfle.de>
> # Date 1300813820 -3600
> # Node ID dcbae547cce81f10c243d54bd35fd139615313cb
> # Parent  e58f6949e76a2786c4f5a99a0da44ee58743b4df
> xentrace: use consistent printk prefix

Why "Xen trace buffers: " and not e.g. "xtb: "?

Jan

> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> diff -r e58f6949e76a -r dcbae547cce8 xen/common/trace.c
> --- a/xen/common/trace.c	Tue Mar 22 18:08:19 2011 +0100
> +++ b/xen/common/trace.c	Tue Mar 22 18:10:20 2011 +0100
> @@ -117,7 +117,7 @@
>      size /= PAGE_SIZE;
>      if ( pages > size )
>      {
> -        printk(XENLOG_INFO "%s: requested number of %u pages reduced to 
> %u\n",
> +        printk(XENLOG_INFO "Xen trace buffers: %s: requested number of %u 
> pages reduced to %u\n",
>                 __func__, pages, (unsigned int)size);
>          pages = size;
>      }
> @@ -177,7 +177,7 @@
>          if ( (rawbuf = alloc_xenheap_pages(
>                  order, MEMF_bits(32 + PAGE_SHIFT))) == NULL )
>          {
> -            printk("Xen trace buffers: memory allocation failed on cpu 
> %d\n", cpu);
> +            printk(XENLOG_INFO "Xen trace buffers: memory allocation failed 
> on cpu %d\n", cpu);
>              goto out_dealloc;
>          }
>  
> @@ -212,7 +212,7 @@
>              t_info_mfn_list[offset + i]=mfn + i;
>          }
>          t_info->mfn_offset[cpu]=offset;
> -        printk(XENLOG_INFO "p%d mfn %"PRIx32" offset %d\n",
> +        printk(XENLOG_INFO "Xen trace buffers: p%d mfn %"PRIx32" offset 
> %d\n",
>                 cpu, mfn, offset);
>          offset+=i;
>  
> @@ -236,7 +236,7 @@
>      {
>          void *rawbuf = per_cpu(t_bufs, cpu);
>          per_cpu(t_bufs, cpu) = NULL;
> -        printk("Xen trace buffers: cpu %d p %p\n", cpu, rawbuf);
> +        printk(XENLOG_DEBUG "Xen trace buffers: cpu %d p %p\n", cpu, 
> rawbuf);
>          if ( rawbuf )
>          {
>              ASSERT(!(virt_to_page(rawbuf)->count_info & PGC_allocated));
> @@ -245,7 +245,7 @@
>      }
>      free_xenheap_pages(t_info, get_order_from_pages(t_info_pages));
>      t_info = NULL;
> -    printk("Xen trace buffers: allocation failed! Tracing disabled.\n");
> +    printk(XENLOG_WARNING "Xen trace buffers: allocation failed! Tracing 
> disabled.\n");
>      return -ENOMEM;
>  }
>  
> @@ -264,7 +264,7 @@
>       */
>      if ( opt_tbuf_size && pages != opt_tbuf_size )
>      {
> -        printk(XENLOG_INFO "tb_set_size from %d to %d not implemented\n",
> +        printk(XENLOG_INFO "Xen trace buffers: tb_set_size from %d to %d 
> not implemented\n",
>                       opt_tbuf_size, pages);
>          return -EINVAL;
>      }
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel 

Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case

[Keir Fraser]

On 23/03/2011 10:12, "Jan Beulich" <JBeulich@novell.com> wrote:

>>      t_info_pages /= PAGE_SIZE;
>> -    if ( t_info_pages % PAGE_SIZE )
>> +    if ( t_info_pages % PAGE_SIZE || t_info_pages == 0 )
> 
> While certainly not having a significant effect, to the unsuspecting
> reader this looks like a bug - is it really meant to be a remainder
> operation on the *result* of a division (rather than on the original
> dividend)? Couldn't you just (ab)use PFN_UP() here?

By which you mean to replace the division and subsequent if statement with
t_info_pages = PFN_UP(t_info_pages).

That would make sense to me. The suggested change in the patch series looks
nonsensical.

 -- Keir

Re: [PATCH 5 of 5] xentrace: use consistent printk prefix

[Olaf Hering]

On Wed, Mar 23, Jan Beulich wrote:

> >>> On 22.03.11 at 20:21, Olaf Hering <olaf@aepfle.de> wrote:
> > # HG changeset patch
> > # User Olaf Hering <olaf@aepfle.de>
> > # Date 1300813820 -3600
> > # Node ID dcbae547cce81f10c243d54bd35fd139615313cb
> > # Parent  e58f6949e76a2786c4f5a99a0da44ee58743b4df
> > xentrace: use consistent printk prefix
> 
> Why "Xen trace buffers: " and not e.g. "xtb: "?

Its used in other printk calls already.
George, would a change to xtb: be ok for you for all existing printk calls?

Olaf

Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case

[Olaf Hering]

On Wed, Mar 23, Keir Fraser wrote:

> On 23/03/2011 10:12, "Jan Beulich" <JBeulich@novell.com> wrote:
> 
> >>      t_info_pages /= PAGE_SIZE;
> >> -    if ( t_info_pages % PAGE_SIZE )
> >> +    if ( t_info_pages % PAGE_SIZE || t_info_pages == 0 )
> > 
> > While certainly not having a significant effect, to the unsuspecting
> > reader this looks like a bug - is it really meant to be a remainder
> > operation on the *result* of a division (rather than on the original
> > dividend)? Couldn't you just (ab)use PFN_UP() here?
> 
> By which you mean to replace the division and subsequent if statement with
> t_info_pages = PFN_UP(t_info_pages).

I did not know about PFN_UP() until now, using it would work as well.

Olaf

Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case

[Keir Fraser]

On 23/03/2011 11:20, "Olaf Hering" <olaf@aepfle.de> wrote:

>>>>      t_info_pages /= PAGE_SIZE;
>>>> -    if ( t_info_pages % PAGE_SIZE )
>>>> +    if ( t_info_pages % PAGE_SIZE || t_info_pages == 0 )
>>> 
>>> While certainly not having a significant effect, to the unsuspecting
>>> reader this looks like a bug - is it really meant to be a remainder
>>> operation on the *result* of a division (rather than on the original
>>> dividend)? Couldn't you just (ab)use PFN_UP() here?
>> 
>> By which you mean to replace the division and subsequent if statement with
>> t_info_pages = PFN_UP(t_info_pages).
> 
> I did not know about PFN_UP() until now, using it would work as well.

As opposed to the existing code (even including your latest patch) which
doesn't work properly. You need to respin at least your patch 1/5.

 -- Keir

Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case

[Jan Beulich]

>>> On 23.03.11 at 12:20, Olaf Hering <olaf@aepfle.de> wrote:
> On Wed, Mar 23, Keir Fraser wrote:
> 
>> On 23/03/2011 10:12, "Jan Beulich" <JBeulich@novell.com> wrote:
>> 
>> >>      t_info_pages /= PAGE_SIZE;
>> >> -    if ( t_info_pages % PAGE_SIZE )
>> >> +    if ( t_info_pages % PAGE_SIZE || t_info_pages == 0 )
>> > 
>> > While certainly not having a significant effect, to the unsuspecting
>> > reader this looks like a bug - is it really meant to be a remainder
>> > operation on the *result* of a division (rather than on the original
>> > dividend)? Couldn't you just (ab)use PFN_UP() here?
>> 
>> By which you mean to replace the division and subsequent if statement with
>> t_info_pages = PFN_UP(t_info_pages).
> 
> I did not know about PFN_UP() until now, using it would work as well.

Note that one caveat with using it here is that you'd first need to
move its definition from include/asm-x86/page.h to somewhere
under include/xen/ - perhaps having a pfn.h like Linux does would
make sense.

Jan

Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case

[Olaf Hering]

On Wed, Mar 23, Keir Fraser wrote:

> On 23/03/2011 11:20, "Olaf Hering" <olaf@aepfle.de> wrote:
> 
> >>>>      t_info_pages /= PAGE_SIZE;
> >>>> -    if ( t_info_pages % PAGE_SIZE )
> >>>> +    if ( t_info_pages % PAGE_SIZE || t_info_pages == 0 )
> >>> 
> >>> While certainly not having a significant effect, to the unsuspecting
> >>> reader this looks like a bug - is it really meant to be a remainder
> >>> operation on the *result* of a division (rather than on the original
> >>> dividend)? Couldn't you just (ab)use PFN_UP() here?
> >> 
> >> By which you mean to replace the division and subsequent if statement with
> >> t_info_pages = PFN_UP(t_info_pages).
> > 
> > I did not know about PFN_UP() until now, using it would work as well.
> 
> As opposed to the existing code (even including your latest patch) which
> doesn't work properly. You need to respin at least your patch 1/5.

I will send the bugfix now.

Olaf

Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case

[Olaf Hering]

On Wed, Mar 23, Jan Beulich wrote:

> >>> On 23.03.11 at 12:20, Olaf Hering <olaf@aepfle.de> wrote:

> > I did not know about PFN_UP() until now, using it would work as well.
> 
> Note that one caveat with using it here is that you'd first need to
> move its definition from include/asm-x86/page.h to somewhere
> under include/xen/ - perhaps having a pfn.h like Linux does would
> make sense.

Yes, if thats ok with Keir I will do that.
Otherwise I could just open code the functionality for t_info_pages.

Olaf

Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case

[Keir Fraser]

On 23/03/2011 13:16, "Olaf Hering" <olaf@aepfle.de> wrote:

> On Wed, Mar 23, Jan Beulich wrote:
> 
>>>>> On 23.03.11 at 12:20, Olaf Hering <olaf@aepfle.de> wrote:
> 
>>> I did not know about PFN_UP() until now, using it would work as well.
>> 
>> Note that one caveat with using it here is that you'd first need to
>> move its definition from include/asm-x86/page.h to somewhere
>> under include/xen/ - perhaps having a pfn.h like Linux does would
>> make sense.
> 
> Yes, if thats ok with Keir I will do that.
> Otherwise I could just open code the functionality for t_info_pages.

I've done it already as c/s 23074, so you can just re-spin your patch on top
of that.

 -- Keir

Re: [PATCH 5 of 5] xentrace: use consistent printk prefix

[George Dunlap]

I'd prefer something a tiny bit more descriptive, like "xentrace:", but
I would acquiesce to xtb if someone felt strongly about it.

 -George

On Wed, 2011-03-23 at 11:18 +0000, Olaf Hering wrote:
> On Wed, Mar 23, Jan Beulich wrote:
> 
> > >>> On 22.03.11 at 20:21, Olaf Hering <olaf@aepfle.de> wrote:
> > > # HG changeset patch
> > > # User Olaf Hering <olaf@aepfle.de>
> > > # Date 1300813820 -3600
> > > # Node ID dcbae547cce81f10c243d54bd35fd139615313cb
> > > # Parent  e58f6949e76a2786c4f5a99a0da44ee58743b4df
> > > xentrace: use consistent printk prefix
> > 
> > Why "Xen trace buffers: " and not e.g. "xtb: "?
> 
> Its used in other printk calls already.
> George, would a change to xtb: be ok for you for all existing printk calls?
> 
> Olaf

Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case

[George Dunlap]

Actually, I think the source of this bug was the re-definition of what
the variable meant in the middle.  I think the Right Thing, to avoid
bugs like this in the future, would be to assign separate variables,
thus:

t_info_bytes= [calculation]
t_info_pages = t_info_bytes / PAGE_SIZE
if(t_info_bytes % PAGE_SIZE)
 t_info_pages++;

The compiler should optimize away unused stuff, and even if not, a byte
here is small cost to make the logic more readable.

 -George

On Wed, 2011-03-23 at 12:46 +0000, Olaf Hering wrote:
> On Wed, Mar 23, Keir Fraser wrote:
> 
> > On 23/03/2011 11:20, "Olaf Hering" <olaf@aepfle.de> wrote:
> > 
> > >>>>      t_info_pages /= PAGE_SIZE;
> > >>>> -    if ( t_info_pages % PAGE_SIZE )
> > >>>> +    if ( t_info_pages % PAGE_SIZE || t_info_pages == 0 )
> > >>> 
> > >>> While certainly not having a significant effect, to the unsuspecting
> > >>> reader this looks like a bug - is it really meant to be a remainder
> > >>> operation on the *result* of a division (rather than on the original
> > >>> dividend)? Couldn't you just (ab)use PFN_UP() here?
> > >> 
> > >> By which you mean to replace the division and subsequent if statement with
> > >> t_info_pages = PFN_UP(t_info_pages).
> > > 
> > > I did not know about PFN_UP() until now, using it would work as well.
> > 
> > As opposed to the existing code (even including your latest patch) which
> > doesn't work properly. You need to respin at least your patch 1/5.
> 
> I will send the bugfix now.
> 
> Olaf

Re: [PATCH 5 of 5] xentrace: use consistent printk prefix

[Jan Beulich]

>>> On 23.03.11 at 15:47, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> I'd prefer something a tiny bit more descriptive, like "xentrace:", but
> I would acquiesce to xtb if someone felt strongly about it.

I was nagging just because "Xen trace buffers: " seemed overly long
to me. I'm fine with your preference.

Jan

>  -George
> 
> On Wed, 2011-03-23 at 11:18 +0000, Olaf Hering wrote:
>> On Wed, Mar 23, Jan Beulich wrote:
>> 
>> > >>> On 22.03.11 at 20:21, Olaf Hering <olaf@aepfle.de> wrote:
>> > > # HG changeset patch
>> > > # User Olaf Hering <olaf@aepfle.de>
>> > > # Date 1300813820 -3600
>> > > # Node ID dcbae547cce81f10c243d54bd35fd139615313cb
>> > > # Parent  e58f6949e76a2786c4f5a99a0da44ee58743b4df
>> > > xentrace: use consistent printk prefix
>> > 
>> > Why "Xen trace buffers: " and not e.g. "xtb: "?
>> 
>> Its used in other printk calls already.
>> George, would a change to xtb: be ok for you for all existing printk calls?
>> 
>> Olaf

Re: [PATCH 5 of 5] xentrace: use consistent printk prefix

[George Dunlap]

On Wed, 2011-03-23 at 16:20 +0000, Jan Beulich wrote:
> >>> On 23.03.11 at 15:47, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> > I'd prefer something a tiny bit more descriptive, like "xentrace:", but
> > I would acquiesce to xtb if someone felt strongly about it.
> 
> I was nagging just because "Xen trace buffers: " seemed overly long
> to me. I'm fine with your preference.

I agree that "Xen trace buffers" is too long -- debug output on a serial
line is somewhat precious when your machine is crashing. :-)

Olaf, are you OK with making the prefix consistently "xentrace"?

 -George


> 
> Jan
> 
> >  -George
> > 
> > On Wed, 2011-03-23 at 11:18 +0000, Olaf Hering wrote:
> >> On Wed, Mar 23, Jan Beulich wrote:
> >> 
> >> > >>> On 22.03.11 at 20:21, Olaf Hering <olaf@aepfle.de> wrote:
> >> > > # HG changeset patch
> >> > > # User Olaf Hering <olaf@aepfle.de>
> >> > > # Date 1300813820 -3600
> >> > > # Node ID dcbae547cce81f10c243d54bd35fd139615313cb
> >> > > # Parent  e58f6949e76a2786c4f5a99a0da44ee58743b4df
> >> > > xentrace: use consistent printk prefix
> >> > 
> >> > Why "Xen trace buffers: " and not e.g. "xtb: "?
> >> 
> >> Its used in other printk calls already.
> >> George, would a change to xtb: be ok for you for all existing printk calls?
> >> 
> >> Olaf
> 
> 
> 

Re: [PATCH 5 of 5] xentrace: use consistent printk prefix

[Olaf Hering]

On Wed, Mar 23, George Dunlap wrote:

> Olaf, are you OK with making the prefix consistently "xentrace"?

Yes, I made this change and test the series right now.

Olaf

[PATCH 2 of 5] xentrace: print calculated numbers in calculate_tbuf_size()

[Olaf Hering]

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1300900085 -3600
# Node ID f9b7516023d3897e545e41ecf6950a798bea0703
# Parent  14ac28e4656d0c235c5edf119426b1bcf3bf33d4
xentrace: print calculated numbers in calculate_tbuf_size()

Print number of pages to allocate for per-cpu tracebuffer and metadata
to ease debugging when allocation fails.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

diff -r 14ac28e4656d -r f9b7516023d3 xen/common/trace.c
--- a/xen/common/trace.c	Wed Mar 23 18:08:04 2011 +0100
+++ b/xen/common/trace.c	Wed Mar 23 18:08:05 2011 +0100
@@ -127,6 +127,8 @@
     t_info_bytes = num_online_cpus() * pages + t_info_first_offset;
     t_info_bytes *= sizeof(uint32_t);
     t_info_pages = PFN_UP(t_info_bytes);
+    printk(XENLOG_INFO "xentrace: requesting %u t_info pages for %u trace pages on %u cpus\n",
+               t_info_pages, pages, num_online_cpus());
     return pages;
 }