Skip to contentSkip to conversation

xentrace updates

PATCH New Olaf Hering 4 participants 12 messages Mar 23, 2011 – Mar 24, 2011

lore
Share & tools
lore

Olaf Hering

author
#0 lore raw

PATCH 1/1 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

author
#1 lore raw

PATCH 1/1 xentrace: fix t_info_pages calculation for the default case

# HG changeset patch
# User Olaf Hering <[email protected]>
# 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 <[email protected]>
xen/common/trace.c +5 -5
diff -r 8e1c737b2c44 -r 14ac28e4656d xen/common/trace.c
@@ -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;
 }
author
#8 lore raw

PATCH 1/1 xentrace: print calculated numbers in calculate_tbuf_size()

In reply to Olaf Hering

# HG changeset patch
# User Olaf Hering <[email protected]>
# 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 <[email protected]>
xen/common/trace.c +2 -0
diff -r 14ac28e4656d -r f9b7516023d3 xen/common/trace.c
@@ -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;
 }
author
#9 lore raw

PATCH 1/1 xentrace: remove gdprintk usage since they are not in guest context

In reply to Olaf Hering

# HG changeset patch
# User Olaf Hering <[email protected]>
# Date 1300900086 -3600
# Node ID 98f7ec77974cd132505c662b244c55deae712a07
# Parent f9b7516023d3897e545e41ecf6950a798bea0703
xentrace: remove gdprintk usage since they are not in guest context

Signed-off-by Olaf Hering <[email protected]>
xen/common/trace.c +3 -3
diff -r f9b7516023d3 -r 98f7ec77974c xen/common/trace.c
@@ -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;
author
#10 lore raw

PATCH 1/1 xentrace: update comments

In reply to Olaf Hering

# HG changeset patch
# User Olaf Hering <[email protected]>
# Date 1300900088 -3600
# Node ID d77f76dd6a6528e10cb3449d3202162108b30c4a # Parent 98f7ec77974cd132505c662b244c55deae712a07
xentrace: update comments

Fix a typo, remove redundant comment.

Signed-off-by Olaf Hering <[email protected]>
xen/common/trace.c +1 -2
diff -r 98f7ec77974c -r d77f76dd6a65 xen/common/trace.c
@@ -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);
author
#11 lore raw

PATCH 1/1 xentrace: use consistent printk prefix

In reply to Olaf Hering

# HG changeset patch
# User Olaf Hering <[email protected]>
# Date 1300902426 -3600
# Node ID 9b4ccefe58426d40c7ccc727106056383c24dc41 # Parent d77f76dd6a6528e10cb3449d3202162108b30c4a
xentrace: use consistent printk prefix

Signed-off-by Olaf Hering <[email protected]>
xen/common/trace.c +9 -10
diff -r d77f76dd6a65 -r 9b4ccefe5842 xen/common/trace.c
@@ -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;
     }
#2 lore raw

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

In reply to Olaf Hering

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

Christoph

Quoted 46 lines · Olaf Hering

# HG changeset patch
# User Olaf Hering<[email protected]>
# 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<[email protected]>

diff -r 8e1c737b2c44 -r 14ac28e4656d xen/common/trace.c

xen/common/trace.c +5 -5
@@ -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;
  }
Signature · 6 lines
---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
#3 lore raw

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

Quoted 2 lines · Christoph Egger

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

Quoted 50 lines · Olaf Hering
Quoted 46 lines

# HG changeset patch
# User Olaf Hering<[email protected]>
# 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<[email protected]>

diff -r 8e1c737b2c44 -r 14ac28e4656d xen/common/trace.c

xen/common/trace.c +5 -5
@@ -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;
   }
Signature · 6 lines
---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
#4 lore raw

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

In reply to Christoph Egger

You need c/s 23074

Quoted 54 lines · Christoph Egger

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

Christoph

Quoted 47 lines · Olaf Hering

# HG changeset patch
# User Olaf Hering<[email protected]>
# 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<[email protected]>

diff -r 8e1c737b2c44 -r 14ac28e4656d xen/common/trace.c

xen/common/trace.c Wed Mar 23 18:08:04 2011 +0100 renamed from xen/common/trace.c Wed Mar 23 15:24:19 2011 +0000 +5 -5
@@ -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;
  }
#5 lore raw

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

In reply to Olaf Hering

xen/common/trace.c line 129

Quoted 8 lines · Olaf Hering
xen/common/trace.c +3 -5
-    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

author
#6 lore raw

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

Quoted 2 lines · George Dunlap

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

#7 lore raw

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

In reply to George Dunlap
Quoted 17 lines · George Dunlap

xen/common/trace.c line 129

Quoted 8 lines · Olaf Hering
xen/common/trace.c +3 -5
-    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);

Quoted 7 lines

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

Quoted 7 lines

-George

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel