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

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

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

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

[Christoph Egger]

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

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

[Christoph Egger]

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

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

[Keir Fraser]

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

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

[George Dunlap]

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

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

[Olaf Hering]

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

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

[Keir Fraser]

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

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

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

[PATCH 4 of 5] xentrace: update comments

[Olaf Hering]

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

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

[Olaf Hering]

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

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