thr3ads.net - Xen devel - [Xen-devel] [PATCH 0 of 5] xentrace updates [Mar 2011]

If this information is useful, please help other people find it:
Share via:

Olaf Hering

2011-Mar-22 19:21 UTC

[Xen-devel] [PATCH 0 of 5] xentrace updates

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-22 19:21 UTC

head link

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

# 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;
 }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-22 19:21 UTC

head link

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

# 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;
 }
 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-22 19:21 UTC

head link

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

# 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;

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-22 19:21 UTC

head link

[Xen-devel] [PATCH 4 of 5] xentrace: update comments

# 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);

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-22 19:21 UTC

head link

[Xen-devel] [PATCH 5 of 5] xentrace: use consistent printk prefix

# 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;
     }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2011-Mar-23 10:12 UTC

head link

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

>>> 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;
>  }


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2011-Mar-23 10:14 UTC

head link

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

>>> 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 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2011-Mar-23 10:22 UTC

head link

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

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-23 11:18 UTC

head link

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-23 11:20 UTC

head link

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2011-Mar-23 12:33 UTC

head link

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

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2011-Mar-23 12:45 UTC

head link

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

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-23 12:46 UTC

head link

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-23 13:16 UTC

head link

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2011-Mar-23 13:35 UTC

head link

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

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
 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

George Dunlap

2011-Mar-23 14:47 UTC

head link

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

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

George Dunlap

2011-Mar-23 14:56 UTC

head link

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

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Jan Beulich

2011-Mar-23 16:20 UTC

head link

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

>>> 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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

George Dunlap

2011-Mar-23 17:02 UTC

head link

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

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
> 
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-23 17:11 UTC

head link

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

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-23 17:54 UTC

head link

[Xen-devel] [PATCH 0 of 5] xentrace updates

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.

v2:
use PFN_UP() macro as suggested by Jan
use local t_info_pages variable for calculation as suggested by George
use xentrace: prefix instead of Xen trace buffers:

Olaf

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-23 17:54 UTC

head link

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

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1300900084 -3600
# Node ID 14ac28e4656d0c235c5edf119426b1bcf3bf33d4
# Parent  8e1c737b2c44249dd1c0e4e1b8978d5d35020226
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 8e1c737b2c44 -r 14ac28e4656d xen/common/trace.c
--- a/xen/common/trace.c	Wed Mar 23 15:24:19 2011 +0000
+++ b/xen/common/trace.c	Wed Mar 23 18:08:04 2011 +0100
@@ -29,6 +29,7 @@
 #include <xen/init.h>
 #include <xen/mm.h>
 #include <xen/percpu.h>
+#include <xen/pfn.h>
 #include <xen/cpu.h>
 #include <asm/atomic.h>
 #include <public/sysctl.h>
@@ -109,6 +110,7 @@
 {
     struct t_buf dummy;
     typeof(dummy.prod) size;
+    unsigned int t_info_bytes;
 
     /* force maximum value for an unsigned type */
     size = -1;
@@ -122,11 +124,9 @@
         pages = size;
     }
 
-    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 )
-        t_info_pages++;
+    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);
     return pages;
 }
 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-23 17:54 UTC

head link

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

# 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;
 }
 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-23 17:54 UTC

head link

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

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

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

diff -r f9b7516023d3 -r 98f7ec77974c xen/common/trace.c
--- a/xen/common/trace.c	Wed Mar 23 18:08:05 2011 +0100
+++ b/xen/common/trace.c	Wed Mar 23 18:08:06 2011 +0100
@@ -119,7 +119,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;

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-23 17:54 UTC

head link

[Xen-devel] [PATCH 4 of 5] xentrace: update comments

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1300900088 -3600
# Node ID d77f76dd6a6528e10cb3449d3202162108b30c4a
# Parent  98f7ec77974cd132505c662b244c55deae712a07
xentrace: update comments

Fix a typo, remove redundant comment.

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

diff -r 98f7ec77974c -r d77f76dd6a65 xen/common/trace.c
--- a/xen/common/trace.c	Wed Mar 23 18:08:06 2011 +0100
+++ b/xen/common/trace.c	Wed Mar 23 18:08:08 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);

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-23 17:54 UTC

head link

[Xen-devel] [PATCH 5 of 5] xentrace: use consistent printk prefix

# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1300902426 -3600
# Node ID 9b4ccefe58426d40c7ccc727106056383c24dc41
# Parent  d77f76dd6a6528e10cb3449d3202162108b30c4a
xentrace: use consistent printk prefix

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

diff -r d77f76dd6a65 -r 9b4ccefe5842 xen/common/trace.c
--- a/xen/common/trace.c	Wed Mar 23 18:08:08 2011 +0100
+++ b/xen/common/trace.c	Wed Mar 23 18:47:06 2011 +0100
@@ -119,8 +119,8 @@
     size /= PAGE_SIZE;
     if ( pages > size )
     {
-        printk(XENLOG_INFO "%s: requested number of %u pages reduced to
%u\n",
-               __func__, pages, (unsigned int)size);
+        printk(XENLOG_INFO "xentrace: requested number of %u pages reduced
to %u\n",
+               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 "xentrace: 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 "xentrace: p%d mfn %"PRIx32" offset
%d\n",
                cpu, mfn, offset);
         offset+=i;
 
@@ -225,7 +225,7 @@
 
     register_cpu_notifier(&cpu_nfb);
 
-    printk("Xen trace buffers: initialised\n");
+    printk("xentrace: initialised\n");
     wmb(); /* above must be visible before tb_init_done flag set */
     tb_init_done = 1;
 
@@ -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 "xentrace: 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 "xentrace: 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 "xentrace: tb_set_size from %d to %d not
implemented\n",
                      opt_tbuf_size, pages);
         return -EINVAL;
     }
@@ -309,8 +309,7 @@
 {
     if ( opt_tbuf_size && alloc_trace_bufs(opt_tbuf_size) )
     {
-        printk(XENLOG_INFO "Xen trace buffers: "
-                 "allocation size %d failed, disabling\n",
+        printk(XENLOG_INFO "xentrace: allocation size %d failed,
disabling\n",
                  opt_tbuf_size);
         opt_tbuf_size = 0;
     }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Christoph Egger

2011-Mar-24 12:09 UTC

head link

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

This patch does not compile for me. There''s no <xen/pfn.h>.

Christoph


On 03/23/11 18:54, Olaf Hering wrote:> # HG changeset patch
> # User Olaf Hering<olaf@aepfle.de>
> # Date 1300900084 -3600
> # Node ID 14ac28e4656d0c235c5edf119426b1bcf3bf33d4
> # Parent  8e1c737b2c44249dd1c0e4e1b8978d5d35020226
> 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 8e1c737b2c44 -r 14ac28e4656d xen/common/trace.c
> --- a/xen/common/trace.c	Wed Mar 23 15:24:19 2011 +0000
> +++ b/xen/common/trace.c	Wed Mar 23 18:08:04 2011 +0100
> @@ -29,6 +29,7 @@
>   #include<xen/init.h>
>   #include<xen/mm.h>
>   #include<xen/percpu.h>
> +#include<xen/pfn.h>
>   #include<xen/cpu.h>
>   #include<asm/atomic.h>
>   #include<public/sysctl.h>
> @@ -109,6 +110,7 @@
>   {
>       struct t_buf dummy;
>       typeof(dummy.prod) size;
> +    unsigned int t_info_bytes;
>
>       /* force maximum value for an unsigned type */
>       size = -1;
> @@ -122,11 +124,9 @@
>           pages = size;
>       }
>
> -    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 )
> -        t_info_pages++;
> +    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);
>       return pages;
>   }
-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Christoph Egger

2011-Mar-24 12:18 UTC

head link

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

On 03/24/11 13:09, Christoph Egger wrote:>
> This patch does not compile for me. There''s no<xen/pfn.h>.
Removing the first hunk manually makes this patch compile for me.
It makes xentrace usable again for me.

Christoph>
>
> On 03/23/11 18:54, Olaf Hering wrote:
>> # HG changeset patch
>> # User Olaf Hering<olaf@aepfle.de>
>> # Date 1300900084 -3600
>> # Node ID 14ac28e4656d0c235c5edf119426b1bcf3bf33d4
>> # Parent  8e1c737b2c44249dd1c0e4e1b8978d5d35020226
>> 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 8e1c737b2c44 -r 14ac28e4656d xen/common/trace.c
>> --- a/xen/common/trace.c	Wed Mar 23 15:24:19 2011 +0000
>> +++ b/xen/common/trace.c	Wed Mar 23 18:08:04 2011 +0100
>> @@ -29,6 +29,7 @@
>>    #include<xen/init.h>
>>    #include<xen/mm.h>
>>    #include<xen/percpu.h>
>> +#include<xen/pfn.h>
>>    #include<xen/cpu.h>
>>    #include<asm/atomic.h>
>>    #include<public/sysctl.h>
>> @@ -109,6 +110,7 @@
>>    {
>>        struct t_buf dummy;
>>        typeof(dummy.prod) size;
>> +    unsigned int t_info_bytes;
>>
>>        /* force maximum value for an unsigned type */
>>        size = -1;
>> @@ -122,11 +124,9 @@
>>            pages = size;
>>        }
>>
>> -    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 )
>> -        t_info_pages++;
>> +    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);
>>        return pages;
>>    }
>

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Einsteinring 24, 85689 Dornach b. Muenchen
Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2011-Mar-24 12:34 UTC

head link

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

You need c/s 23074


On 24/03/2011 12:09, "Christoph Egger" <Christoph.Egger@amd.com>
wrote:
> 
> This patch does not compile for me. There''s no <xen/pfn.h>.
> 
> Christoph
> 
> 
> On 03/23/11 18:54, Olaf Hering wrote:
>> # HG changeset patch
>> # User Olaf Hering<olaf@aepfle.de>
>> # Date 1300900084 -3600
>> # Node ID 14ac28e4656d0c235c5edf119426b1bcf3bf33d4
>> # Parent  8e1c737b2c44249dd1c0e4e1b8978d5d35020226
>> 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 8e1c737b2c44 -r 14ac28e4656d xen/common/trace.c
>> --- a/xen/common/trace.c Wed Mar 23 15:24:19 2011 +0000
>> +++ b/xen/common/trace.c Wed Mar 23 18:08:04 2011 +0100
>> @@ -29,6 +29,7 @@
>>   #include<xen/init.h>
>>   #include<xen/mm.h>
>>   #include<xen/percpu.h>
>> +#include<xen/pfn.h>
>>   #include<xen/cpu.h>
>>   #include<asm/atomic.h>
>>   #include<public/sysctl.h>
>> @@ -109,6 +110,7 @@
>>   {
>>       struct t_buf dummy;
>>       typeof(dummy.prod) size;
>> +    unsigned int t_info_bytes;
>> 
>>       /* force maximum value for an unsigned type */
>>       size = -1;
>> @@ -122,11 +124,9 @@
>>           pages = size;
>>       }
>> 
>> -    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 )
>> -        t_info_pages++;
>> +    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);
>>       return pages;
>>   }


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

George Dunlap

2011-Mar-24 15:47 UTC

head link

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

On Wed, 2011-03-23 at 17:54 +0000, Olaf Hering wrote:> -    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 )
> -        t_info_pages++;
> +    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);
Hmm, still not quite following the spirit of the idea -- that
t_info_bytes should be bytes, not words (as it is in the first
instance).  I think I''d prefer making it one assignment:

    t_info_bytes = ( num_online_cpus() * pages + t_info_first_offset )
                     * sizeof(uint32_t);

But if you don''t like that, to keep consistent, we should do this:
    t_info_words = num_online_cpus() * pages + t_info_first_offset;
    t_info_bytes = t_info_words * sizeof(uint32_t);
    t_info_pages = PFN_UP(t_info_bytes);
 
Then it''s really clear when looking at it what the inputs and outputs
of
each line is supposed to be.

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Olaf Hering

2011-Mar-24 16:03 UTC

head link

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

On Thu, Mar 24, George Dunlap wrote:
> Then it''s really clear when looking at it what the inputs and
outputs of
> each line is supposed to be.
George,

lets not overdo things. The formula I came up with, based on the initial
code, is even slightly incorrect. It may allocate more than needed.
Each cpu has some pages/mfns stored as uint32_t. That list is stored
with an offset at tinfo. So its more like:
num_online_cpus() * pages * sizeof(uint32_t) + t_info_first_offset;
And that number of bytes is now correctly converted to pages with
PFN_UP. 

        t_info_words = num_online_cpus() * pages * sizeof(uint32_t);
        t_info_pages = PFN_UP(t_info_first_offset + t_info_words);

(Modulo things I missed.)

Olaf

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Keir Fraser

2011-Mar-24 16:04 UTC

head link

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

On 24/03/2011 15:47, "George Dunlap"
<george.dunlap@eu.citrix.com> wrote:
> On Wed, 2011-03-23 at 17:54 +0000, Olaf Hering wrote:
>> -    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 )
>> -        t_info_pages++;
>> +    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);
> 
> Hmm, still not quite following the spirit of the idea -- that
> t_info_bytes should be bytes, not words (as it is in the first
> instance).  I think I''d prefer making it one assignment:
> 
>     t_info_bytes = ( num_online_cpus() * pages + t_info_first_offset )
>                      * sizeof(uint32_t);
> 
> But if you don''t like that, to keep consistent, we should do this:
>     t_info_words = num_online_cpus() * pages + t_info_first_offset;
>     t_info_bytes = t_info_words * sizeof(uint32_t);
>     t_info_pages = PFN_UP(t_info_bytes);
>  
> Then it''s really clear when looking at it what the inputs and
outputs of
> each line is supposed to be.
I''ll clean this up and apply the whole series.

 -- Keir
>  -George
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Xen devel - Mar 2011 - [PATCH 0 of 5] xentrace updates

[Xen-devel] [PATCH 0 of 5] xentrace updates

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

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

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

[Xen-devel] [PATCH 4 of 5] xentrace: update comments

[Xen-devel] [PATCH 5 of 5] xentrace: use consistent printk prefix

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

[Xen-devel] [PATCH 0 of 5] xentrace updates

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

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

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

[Xen-devel] [PATCH 4 of 5] xentrace: update comments

[Xen-devel] [PATCH 5 of 5] xentrace: use consistent printk prefix

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

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

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

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

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

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