* [PATCH 1 of 4] xentrace: reduce trace buffer size to something mfn_offset can reach
2011-05-10 14:32 [PATCH 0 of 4] xentrace [v2]: non-contiguous allocation of per-cpu buffer Olaf Hering
@ 2011-05-10 14:32 ` Olaf Hering
2011-05-26 10:05 ` George Dunlap
2011-05-10 14:32 ` [PATCH 2 of 4] xentrace: fix type of offset to avoid ouf-of-bounds access Olaf Hering
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Olaf Hering @ 2011-05-10 14:32 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap
# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1305037380 -7200
# Node ID 8ac937fa527b28243227193bf4749feb3a234c2c
# Parent 19452acd23045f40c4e18437f0a60f016757e5bd
xentrace: reduce trace buffer size to something mfn_offset can reach
The start of the array which holds the list of mfns for each cpus
tracebuffer is stored in an unsigned short. This limits the total amount
of pages for each cpu as the number of active cpus increases.
Update the math in calculate_tbuf_size() to apply also this rule to the
max number of trace pages. Without this change the index can overflow.
Signed-off-by: Olaf Hering <olaf@aepfle.de>
diff -r 19452acd2304 -r 8ac937fa527b xen/common/trace.c
--- a/xen/common/trace.c Fri May 06 11:15:35 2011 +0100
+++ b/xen/common/trace.c Tue May 10 16:23:00 2011 +0200
@@ -112,11 +112,14 @@ static int calculate_tbuf_size(unsigned
typeof(dummy_size.prod) max_size;
struct t_info dummy_pages;
typeof(dummy_pages.tbuf_size) max_pages;
+ typeof(dummy_pages.mfn_offset[0]) max_mfn_offset;
+ unsigned int max_cpus = num_online_cpus();
unsigned int t_info_words;
/* force maximum value for an unsigned type */
max_size = -1;
max_pages = -1;
+ max_mfn_offset = -1;
/* max size holds up to n pages */
max_size /= PAGE_SIZE;
@@ -124,6 +127,18 @@ static int calculate_tbuf_size(unsigned
if ( max_size < max_pages )
max_pages = max_size;
+ /*
+ * max mfn_offset holds up to n pages per cpu
+ * The array of mfns for the highest cpu can start at the maximum value
+ * mfn_offset can hold. So reduce the number of cpus and also the mfn_offset.
+ */
+ max_mfn_offset -= t_info_first_offset - 1;
+ max_cpus--;
+ if ( max_cpus )
+ max_mfn_offset /= max_cpus;
+ if ( max_mfn_offset < max_pages )
+ max_pages = max_mfn_offset;
+
if ( pages > max_pages )
{
printk(XENLOG_INFO "xentrace: requested number of %u pages "
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1 of 4] xentrace: reduce trace buffer size to something mfn_offset can reach
2011-05-10 14:32 ` [PATCH 1 of 4] xentrace: reduce trace buffer size to something mfn_offset can reach Olaf Hering
@ 2011-05-26 10:05 ` George Dunlap
0 siblings, 0 replies; 10+ messages in thread
From: George Dunlap @ 2011-05-26 10:05 UTC (permalink / raw)
To: Olaf Hering; +Cc: xen-devel, George Dunlap
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
Sorry for the delay!
-George
On Tue, May 10, 2011 at 3:32 PM, Olaf Hering <olaf@aepfle.de> wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@aepfle.de>
> # Date 1305037380 -7200
> # Node ID 8ac937fa527b28243227193bf4749feb3a234c2c
> # Parent 19452acd23045f40c4e18437f0a60f016757e5bd
> xentrace: reduce trace buffer size to something mfn_offset can reach
>
> The start of the array which holds the list of mfns for each cpus
> tracebuffer is stored in an unsigned short. This limits the total amount
> of pages for each cpu as the number of active cpus increases.
>
> Update the math in calculate_tbuf_size() to apply also this rule to the
> max number of trace pages. Without this change the index can overflow.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
>
> diff -r 19452acd2304 -r 8ac937fa527b xen/common/trace.c
> --- a/xen/common/trace.c Fri May 06 11:15:35 2011 +0100
> +++ b/xen/common/trace.c Tue May 10 16:23:00 2011 +0200
> @@ -112,11 +112,14 @@ static int calculate_tbuf_size(unsigned
> typeof(dummy_size.prod) max_size;
> struct t_info dummy_pages;
> typeof(dummy_pages.tbuf_size) max_pages;
> + typeof(dummy_pages.mfn_offset[0]) max_mfn_offset;
> + unsigned int max_cpus = num_online_cpus();
> unsigned int t_info_words;
>
> /* force maximum value for an unsigned type */
> max_size = -1;
> max_pages = -1;
> + max_mfn_offset = -1;
>
> /* max size holds up to n pages */
> max_size /= PAGE_SIZE;
> @@ -124,6 +127,18 @@ static int calculate_tbuf_size(unsigned
> if ( max_size < max_pages )
> max_pages = max_size;
>
> + /*
> + * max mfn_offset holds up to n pages per cpu
> + * The array of mfns for the highest cpu can start at the maximum value
> + * mfn_offset can hold. So reduce the number of cpus and also the mfn_offset.
> + */
> + max_mfn_offset -= t_info_first_offset - 1;
> + max_cpus--;
> + if ( max_cpus )
> + max_mfn_offset /= max_cpus;
> + if ( max_mfn_offset < max_pages )
> + max_pages = max_mfn_offset;
> +
> if ( pages > max_pages )
> {
> printk(XENLOG_INFO "xentrace: requested number of %u pages "
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2 of 4] xentrace: fix type of offset to avoid ouf-of-bounds access
2011-05-10 14:32 [PATCH 0 of 4] xentrace [v2]: non-contiguous allocation of per-cpu buffer Olaf Hering
2011-05-10 14:32 ` [PATCH 1 of 4] xentrace: reduce trace buffer size to something mfn_offset can reach Olaf Hering
@ 2011-05-10 14:32 ` Olaf Hering
2011-05-26 10:05 ` George Dunlap
2011-05-10 14:32 ` [PATCH 3 of 4] xentrace: update __insert_record() to copy the trace record to individual mfns Olaf Hering
` (2 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Olaf Hering @ 2011-05-10 14:32 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap
# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1305037381 -7200
# Node ID 575bf78214ef193e44806aa9766e084d721783b5
# Parent 8ac937fa527b28243227193bf4749feb3a234c2c
xentrace: fix type of offset to avoid ouf-of-bounds access
Update the type of the local offset variable to match the type where
this variable is stored. Also update the type of t_info_first_offset because
it has also a limited range.
Signed-off-by: Olaf Hering <olaf@aepfle.de>
diff -r 8ac937fa527b -r 575bf78214ef xen/common/trace.c
--- a/xen/common/trace.c Tue May 10 16:23:00 2011 +0200
+++ b/xen/common/trace.c Tue May 10 16:23:01 2011 +0200
@@ -106,7 +106,7 @@ static uint32_t calc_tinfo_first_offset(
* The t_info layout is fixed and cant be changed without breaking xentrace.
* Initialize t_info_pages based on number of trace pages.
*/
-static int calculate_tbuf_size(unsigned int pages, uint32_t t_info_first_offset)
+static int calculate_tbuf_size(unsigned int pages, uint16_t t_info_first_offset)
{
struct t_buf dummy_size;
typeof(dummy_size.prod) max_size;
@@ -170,8 +170,8 @@ static int alloc_trace_bufs(unsigned int
int i, cpu, order;
/* Start after a fixed-size array of NR_CPUS */
uint32_t *t_info_mfn_list;
- uint32_t t_info_first_offset;
- int offset;
+ uint16_t t_info_first_offset;
+ uint16_t offset;
if ( t_info )
return -EBUSY;
@@ -179,7 +179,7 @@ static int alloc_trace_bufs(unsigned int
if ( pages == 0 )
return -EINVAL;
- /* Calculate offset in u32 of first mfn */
+ /* Calculate offset in units of u32 of first mfn */
t_info_first_offset = calc_tinfo_first_offset();
pages = calculate_tbuf_size(pages, t_info_first_offset);
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2 of 4] xentrace: fix type of offset to avoid ouf-of-bounds access
2011-05-10 14:32 ` [PATCH 2 of 4] xentrace: fix type of offset to avoid ouf-of-bounds access Olaf Hering
@ 2011-05-26 10:05 ` George Dunlap
0 siblings, 0 replies; 10+ messages in thread
From: George Dunlap @ 2011-05-26 10:05 UTC (permalink / raw)
To: Olaf Hering; +Cc: xen-devel, George Dunlap
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
On Tue, May 10, 2011 at 3:32 PM, Olaf Hering <olaf@aepfle.de> wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@aepfle.de>
> # Date 1305037381 -7200
> # Node ID 575bf78214ef193e44806aa9766e084d721783b5
> # Parent 8ac937fa527b28243227193bf4749feb3a234c2c
> xentrace: fix type of offset to avoid ouf-of-bounds access
>
> Update the type of the local offset variable to match the type where
> this variable is stored. Also update the type of t_info_first_offset because
> it has also a limited range.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
>
> diff -r 8ac937fa527b -r 575bf78214ef xen/common/trace.c
> --- a/xen/common/trace.c Tue May 10 16:23:00 2011 +0200
> +++ b/xen/common/trace.c Tue May 10 16:23:01 2011 +0200
> @@ -106,7 +106,7 @@ static uint32_t calc_tinfo_first_offset(
> * The t_info layout is fixed and cant be changed without breaking xentrace.
> * Initialize t_info_pages based on number of trace pages.
> */
> -static int calculate_tbuf_size(unsigned int pages, uint32_t t_info_first_offset)
> +static int calculate_tbuf_size(unsigned int pages, uint16_t t_info_first_offset)
> {
> struct t_buf dummy_size;
> typeof(dummy_size.prod) max_size;
> @@ -170,8 +170,8 @@ static int alloc_trace_bufs(unsigned int
> int i, cpu, order;
> /* Start after a fixed-size array of NR_CPUS */
> uint32_t *t_info_mfn_list;
> - uint32_t t_info_first_offset;
> - int offset;
> + uint16_t t_info_first_offset;
> + uint16_t offset;
>
> if ( t_info )
> return -EBUSY;
> @@ -179,7 +179,7 @@ static int alloc_trace_bufs(unsigned int
> if ( pages == 0 )
> return -EINVAL;
>
> - /* Calculate offset in u32 of first mfn */
> + /* Calculate offset in units of u32 of first mfn */
> t_info_first_offset = calc_tinfo_first_offset();
>
> pages = calculate_tbuf_size(pages, t_info_first_offset);
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3 of 4] xentrace: update __insert_record() to copy the trace record to individual mfns
2011-05-10 14:32 [PATCH 0 of 4] xentrace [v2]: non-contiguous allocation of per-cpu buffer Olaf Hering
2011-05-10 14:32 ` [PATCH 1 of 4] xentrace: reduce trace buffer size to something mfn_offset can reach Olaf Hering
2011-05-10 14:32 ` [PATCH 2 of 4] xentrace: fix type of offset to avoid ouf-of-bounds access Olaf Hering
@ 2011-05-10 14:32 ` Olaf Hering
2011-05-26 10:06 ` George Dunlap
2011-05-10 14:32 ` [PATCH 4 of 4] xentrace: allocate non-contiguous per-cpu trace buffers Olaf Hering
2011-05-20 8:36 ` [PATCH 0 of 4] xentrace [v2]: non-contiguous allocation of per-cpu buffer Keir Fraser
4 siblings, 1 reply; 10+ messages in thread
From: Olaf Hering @ 2011-05-10 14:32 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap
# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1305037539 -7200
# Node ID 1a45e40add8b407532374c34f20bad51707808cf
# Parent 575bf78214ef193e44806aa9766e084d721783b5
xentrace: update __insert_record() to copy the trace record to individual mfns
Update __insert_record() to copy the trace record to individual mfns.
This is a prereq before changing the per-cpu allocation from contiguous
to non-contiguous allocation.
v2:
update offset calculation to use shift and mask
update type of mfn_offset to match type of data source
Signed-off-by: Olaf Hering <olaf@aepfle.de>
diff -r 575bf78214ef -r 1a45e40add8b xen/common/trace.c
--- a/xen/common/trace.c Tue May 10 16:23:01 2011 +0200
+++ b/xen/common/trace.c Tue May 10 16:25:39 2011 +0200
@@ -52,7 +52,6 @@ static struct t_info *t_info;
static unsigned int t_info_pages;
static DEFINE_PER_CPU_READ_MOSTLY(struct t_buf *, t_bufs);
-static DEFINE_PER_CPU_READ_MOSTLY(unsigned char *, t_data);
static DEFINE_PER_CPU_READ_MOSTLY(spinlock_t, t_lock);
static u32 data_size __read_mostly;
@@ -208,7 +207,6 @@ static int alloc_trace_bufs(unsigned int
per_cpu(t_bufs, cpu) = buf = rawbuf;
buf->cons = buf->prod = 0;
- per_cpu(t_data, cpu) = (unsigned char *)(buf + 1);
}
offset = t_info_first_offset;
@@ -472,10 +470,16 @@ static inline u32 calc_bytes_avail(const
return data_size - calc_unconsumed_bytes(buf);
}
-static inline struct t_rec *next_record(const struct t_buf *buf,
- uint32_t *next)
+static unsigned char *next_record(const struct t_buf *buf, uint32_t *next,
+ unsigned char **next_page,
+ uint32_t *offset_in_page)
{
u32 x = buf->prod, cons = buf->cons;
+ uint16_t per_cpu_mfn_offset;
+ uint32_t per_cpu_mfn_nr;
+ uint32_t *mfn_list;
+ uint32_t mfn;
+ unsigned char *this_page;
barrier(); /* must read buf->prod and buf->cons only once */
*next = x;
@@ -487,7 +491,27 @@ static inline struct t_rec *next_record(
ASSERT(x < data_size);
- return (struct t_rec *)&this_cpu(t_data)[x];
+ /* add leading header to get total offset of next record */
+ x += sizeof(struct t_buf);
+ *offset_in_page = x & ~PAGE_MASK;
+
+ /* offset into array of mfns */
+ per_cpu_mfn_nr = x >> PAGE_SHIFT;
+ per_cpu_mfn_offset = t_info->mfn_offset[smp_processor_id()];
+ mfn_list = (uint32_t *)t_info;
+ mfn = mfn_list[per_cpu_mfn_offset + per_cpu_mfn_nr];
+ this_page = mfn_to_virt(mfn);
+ if (per_cpu_mfn_nr + 1 >= opt_tbuf_size)
+ {
+ /* reached end of buffer? */
+ *next_page = NULL;
+ }
+ else
+ {
+ mfn = mfn_list[per_cpu_mfn_offset + per_cpu_mfn_nr + 1];
+ *next_page = mfn_to_virt(mfn);
+ }
+ return this_page;
}
static inline void __insert_record(struct t_buf *buf,
@@ -497,28 +521,37 @@ static inline void __insert_record(struc
unsigned int rec_size,
const void *extra_data)
{
- struct t_rec *rec;
+ struct t_rec split_rec, *rec;
uint32_t *dst;
+ unsigned char *this_page, *next_page;
unsigned int extra_word = extra / sizeof(u32);
unsigned int local_rec_size = calc_rec_size(cycles, extra);
uint32_t next;
+ uint32_t offset;
+ uint32_t remaining;
BUG_ON(local_rec_size != rec_size);
BUG_ON(extra & 3);
- rec = next_record(buf, &next);
- if ( !rec )
+ this_page = next_record(buf, &next, &next_page, &offset);
+ if ( !this_page )
return;
- /* Double-check once more that we have enough space.
- * Don't bugcheck here, in case the userland tool is doing
- * something stupid. */
- if ( (unsigned char *)rec + rec_size > this_cpu(t_data) + data_size )
+
+ remaining = PAGE_SIZE - offset;
+
+ if ( unlikely(rec_size > remaining) )
{
- if ( printk_ratelimit() )
+ if ( next_page == NULL )
+ {
+ /* access beyond end of buffer */
printk(XENLOG_WARNING
- "%s: size=%08x prod=%08x cons=%08x rec=%u\n",
- __func__, data_size, next, buf->cons, rec_size);
- return;
+ "%s: size=%08x prod=%08x cons=%08x rec=%u remaining=%u\n",
+ __func__, data_size, next, buf->cons, rec_size, remaining);
+ return;
+ }
+ rec = &split_rec;
+ } else {
+ rec = (struct t_rec*)(this_page + offset);
}
rec->event = event;
@@ -535,6 +568,12 @@ static inline void __insert_record(struc
if ( extra_data && extra )
memcpy(dst, extra_data, extra);
+ if ( unlikely(rec_size > remaining) )
+ {
+ memcpy(this_page + offset, rec, remaining);
+ memcpy(next_page, (char *)rec + remaining, rec_size - remaining);
+ }
+
wmb();
next += rec_size;
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 3 of 4] xentrace: update __insert_record() to copy the trace record to individual mfns
2011-05-10 14:32 ` [PATCH 3 of 4] xentrace: update __insert_record() to copy the trace record to individual mfns Olaf Hering
@ 2011-05-26 10:06 ` George Dunlap
0 siblings, 0 replies; 10+ messages in thread
From: George Dunlap @ 2011-05-26 10:06 UTC (permalink / raw)
To: Olaf Hering; +Cc: xen-devel, George Dunlap
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
On Tue, May 10, 2011 at 3:32 PM, Olaf Hering <olaf@aepfle.de> wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@aepfle.de>
> # Date 1305037539 -7200
> # Node ID 1a45e40add8b407532374c34f20bad51707808cf
> # Parent 575bf78214ef193e44806aa9766e084d721783b5
> xentrace: update __insert_record() to copy the trace record to individual mfns
>
> Update __insert_record() to copy the trace record to individual mfns.
> This is a prereq before changing the per-cpu allocation from contiguous
> to non-contiguous allocation.
>
> v2:
> update offset calculation to use shift and mask
> update type of mfn_offset to match type of data source
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
>
> diff -r 575bf78214ef -r 1a45e40add8b xen/common/trace.c
> --- a/xen/common/trace.c Tue May 10 16:23:01 2011 +0200
> +++ b/xen/common/trace.c Tue May 10 16:25:39 2011 +0200
> @@ -52,7 +52,6 @@ static struct t_info *t_info;
> static unsigned int t_info_pages;
>
> static DEFINE_PER_CPU_READ_MOSTLY(struct t_buf *, t_bufs);
> -static DEFINE_PER_CPU_READ_MOSTLY(unsigned char *, t_data);
> static DEFINE_PER_CPU_READ_MOSTLY(spinlock_t, t_lock);
> static u32 data_size __read_mostly;
>
> @@ -208,7 +207,6 @@ static int alloc_trace_bufs(unsigned int
>
> per_cpu(t_bufs, cpu) = buf = rawbuf;
> buf->cons = buf->prod = 0;
> - per_cpu(t_data, cpu) = (unsigned char *)(buf + 1);
> }
>
> offset = t_info_first_offset;
> @@ -472,10 +470,16 @@ static inline u32 calc_bytes_avail(const
> return data_size - calc_unconsumed_bytes(buf);
> }
>
> -static inline struct t_rec *next_record(const struct t_buf *buf,
> - uint32_t *next)
> +static unsigned char *next_record(const struct t_buf *buf, uint32_t *next,
> + unsigned char **next_page,
> + uint32_t *offset_in_page)
> {
> u32 x = buf->prod, cons = buf->cons;
> + uint16_t per_cpu_mfn_offset;
> + uint32_t per_cpu_mfn_nr;
> + uint32_t *mfn_list;
> + uint32_t mfn;
> + unsigned char *this_page;
>
> barrier(); /* must read buf->prod and buf->cons only once */
> *next = x;
> @@ -487,7 +491,27 @@ static inline struct t_rec *next_record(
>
> ASSERT(x < data_size);
>
> - return (struct t_rec *)&this_cpu(t_data)[x];
> + /* add leading header to get total offset of next record */
> + x += sizeof(struct t_buf);
> + *offset_in_page = x & ~PAGE_MASK;
> +
> + /* offset into array of mfns */
> + per_cpu_mfn_nr = x >> PAGE_SHIFT;
> + per_cpu_mfn_offset = t_info->mfn_offset[smp_processor_id()];
> + mfn_list = (uint32_t *)t_info;
> + mfn = mfn_list[per_cpu_mfn_offset + per_cpu_mfn_nr];
> + this_page = mfn_to_virt(mfn);
> + if (per_cpu_mfn_nr + 1 >= opt_tbuf_size)
> + {
> + /* reached end of buffer? */
> + *next_page = NULL;
> + }
> + else
> + {
> + mfn = mfn_list[per_cpu_mfn_offset + per_cpu_mfn_nr + 1];
> + *next_page = mfn_to_virt(mfn);
> + }
> + return this_page;
> }
>
> static inline void __insert_record(struct t_buf *buf,
> @@ -497,28 +521,37 @@ static inline void __insert_record(struc
> unsigned int rec_size,
> const void *extra_data)
> {
> - struct t_rec *rec;
> + struct t_rec split_rec, *rec;
> uint32_t *dst;
> + unsigned char *this_page, *next_page;
> unsigned int extra_word = extra / sizeof(u32);
> unsigned int local_rec_size = calc_rec_size(cycles, extra);
> uint32_t next;
> + uint32_t offset;
> + uint32_t remaining;
>
> BUG_ON(local_rec_size != rec_size);
> BUG_ON(extra & 3);
>
> - rec = next_record(buf, &next);
> - if ( !rec )
> + this_page = next_record(buf, &next, &next_page, &offset);
> + if ( !this_page )
> return;
> - /* Double-check once more that we have enough space.
> - * Don't bugcheck here, in case the userland tool is doing
> - * something stupid. */
> - if ( (unsigned char *)rec + rec_size > this_cpu(t_data) + data_size )
> +
> + remaining = PAGE_SIZE - offset;
> +
> + if ( unlikely(rec_size > remaining) )
> {
> - if ( printk_ratelimit() )
> + if ( next_page == NULL )
> + {
> + /* access beyond end of buffer */
> printk(XENLOG_WARNING
> - "%s: size=%08x prod=%08x cons=%08x rec=%u\n",
> - __func__, data_size, next, buf->cons, rec_size);
> - return;
> + "%s: size=%08x prod=%08x cons=%08x rec=%u remaining=%u\n",
> + __func__, data_size, next, buf->cons, rec_size, remaining);
> + return;
> + }
> + rec = &split_rec;
> + } else {
> + rec = (struct t_rec*)(this_page + offset);
> }
>
> rec->event = event;
> @@ -535,6 +568,12 @@ static inline void __insert_record(struc
> if ( extra_data && extra )
> memcpy(dst, extra_data, extra);
>
> + if ( unlikely(rec_size > remaining) )
> + {
> + memcpy(this_page + offset, rec, remaining);
> + memcpy(next_page, (char *)rec + remaining, rec_size - remaining);
> + }
> +
> wmb();
>
> next += rec_size;
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4 of 4] xentrace: allocate non-contiguous per-cpu trace buffers
2011-05-10 14:32 [PATCH 0 of 4] xentrace [v2]: non-contiguous allocation of per-cpu buffer Olaf Hering
` (2 preceding siblings ...)
2011-05-10 14:32 ` [PATCH 3 of 4] xentrace: update __insert_record() to copy the trace record to individual mfns Olaf Hering
@ 2011-05-10 14:32 ` Olaf Hering
2011-05-26 10:06 ` George Dunlap
2011-05-20 8:36 ` [PATCH 0 of 4] xentrace [v2]: non-contiguous allocation of per-cpu buffer Keir Fraser
4 siblings, 1 reply; 10+ messages in thread
From: Olaf Hering @ 2011-05-10 14:32 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap
# HG changeset patch
# User Olaf Hering <olaf@aepfle.de>
# Date 1305037567 -7200
# Node ID 7f6a118d237e60ca6f30723db687b06a8ef3c3e9
# Parent 1a45e40add8b407532374c34f20bad51707808cf
xentrace: allocate non-contiguous per-cpu trace buffers
Signed-off-by: Olaf Hering <olaf@aepfle.de>
diff -r 1a45e40add8b -r 7f6a118d237e xen/common/trace.c
--- a/xen/common/trace.c Tue May 10 16:25:39 2011 +0200
+++ b/xen/common/trace.c Tue May 10 16:26:07 2011 +0200
@@ -166,7 +166,7 @@ static int calculate_tbuf_size(unsigned
*/
static int alloc_trace_bufs(unsigned int pages)
{
- int i, cpu, order;
+ int i, cpu;
/* Start after a fixed-size array of NR_CPUS */
uint32_t *t_info_mfn_list;
uint16_t t_info_first_offset;
@@ -182,34 +182,11 @@ static int alloc_trace_bufs(unsigned int
t_info_first_offset = calc_tinfo_first_offset();
pages = calculate_tbuf_size(pages, t_info_first_offset);
- order = get_order_from_pages(pages);
t_info = alloc_xenheap_pages(get_order_from_pages(t_info_pages), 0);
if ( t_info == NULL )
- goto out_dealloc;
+ goto out_dealloc_t_info;
- /*
- * First, allocate buffers for all of the cpus. If any
- * fails, deallocate what you have so far and exit.
- */
- for_each_online_cpu(cpu)
- {
- void *rawbuf;
- struct t_buf *buf;
-
- if ( (rawbuf = alloc_xenheap_pages(
- order, MEMF_bits(32 + PAGE_SHIFT))) == NULL )
- {
- printk(XENLOG_INFO "xentrace: memory allocation failed "
- "on cpu %d\n", cpu);
- goto out_dealloc;
- }
-
- per_cpu(t_bufs, cpu) = buf = rawbuf;
- buf->cons = buf->prod = 0;
- }
-
- offset = t_info_first_offset;
t_info_mfn_list = (uint32_t *)t_info;
for(i = 0; i < t_info_pages; i++)
@@ -219,27 +196,53 @@ static int alloc_trace_bufs(unsigned int
t_info->tbuf_size = pages;
/*
- * Now share the pages so xentrace can map them, and write them in
- * the global t_info structure.
+ * Allocate buffers for all of the cpus.
+ * If any fails, deallocate what you have so far and exit.
*/
for_each_online_cpu(cpu)
{
- void *rawbuf = per_cpu(t_bufs, cpu);
- struct page_info *p = virt_to_page(rawbuf);
- uint32_t mfn = virt_to_mfn(rawbuf);
+ offset = t_info_first_offset + (cpu * pages);
+ t_info->mfn_offset[cpu] = offset;
for ( i = 0; i < pages; i++ )
{
- share_xen_page_with_privileged_guests(p + i, XENSHARE_writable);
+ void *p = alloc_xenheap_pages(0, MEMF_bits(32 + PAGE_SHIFT));
+ if ( !p )
+ {
+ printk(XENLOG_INFO "xentrace: memory allocation failed "
+ "on cpu %d after %d pages\n", cpu, i);
+ t_info_mfn_list[offset + i] = 0;
+ goto out_dealloc;
+ }
+ t_info_mfn_list[offset + i] = virt_to_mfn(p);
+ }
+ }
- t_info_mfn_list[offset + i]=mfn + i;
- }
- t_info->mfn_offset[cpu]=offset;
- printk(XENLOG_INFO "xentrace: p%d mfn %"PRIx32" offset %d\n",
- cpu, mfn, offset);
- offset+=i;
+ /*
+ * Initialize buffers for all of the cpus.
+ */
+ for_each_online_cpu(cpu)
+ {
+ struct t_buf *buf;
+ struct page_info *pg;
spin_lock_init(&per_cpu(t_lock, cpu));
+
+ offset = t_info->mfn_offset[cpu];
+
+ /* Initialize the buffer metadata */
+ per_cpu(t_bufs, cpu) = buf = mfn_to_virt(t_info_mfn_list[offset]);
+ buf->cons = buf->prod = 0;
+
+ printk(XENLOG_INFO "xentrace: p%d mfn %x offset %u\n",
+ cpu, t_info_mfn_list[offset], offset);
+
+ /* Now share the trace pages */
+ for ( i = 0; i < pages; i++ )
+ {
+ pg = mfn_to_page(t_info_mfn_list[offset + i]);
+ share_xen_page_with_privileged_guests(pg, XENSHARE_writable);
+ }
}
data_size = (pages * PAGE_SIZE - sizeof(struct t_buf));
@@ -255,14 +258,19 @@ static int alloc_trace_bufs(unsigned int
out_dealloc:
for_each_online_cpu(cpu)
{
- void *rawbuf = per_cpu(t_bufs, cpu);
- per_cpu(t_bufs, cpu) = NULL;
- if ( rawbuf )
+ offset = t_info->mfn_offset[cpu];
+ if ( !offset )
+ continue;
+ for ( i = 0; i < pages; i++ )
{
- ASSERT(!(virt_to_page(rawbuf)->count_info & PGC_allocated));
- free_xenheap_pages(rawbuf, order);
+ uint32_t mfn = t_info_mfn_list[offset + i];
+ if ( !mfn )
+ break;
+ ASSERT(!(mfn_to_page(mfn)->count_info & PGC_allocated));
+ free_xenheap_pages(mfn_to_virt(mfn), 0);
}
}
+out_dealloc_t_info:
free_xenheap_pages(t_info, get_order_from_pages(t_info_pages));
t_info = NULL;
printk(XENLOG_WARNING "xentrace: allocation failed! Tracing disabled.\n");
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 4 of 4] xentrace: allocate non-contiguous per-cpu trace buffers
2011-05-10 14:32 ` [PATCH 4 of 4] xentrace: allocate non-contiguous per-cpu trace buffers Olaf Hering
@ 2011-05-26 10:06 ` George Dunlap
0 siblings, 0 replies; 10+ messages in thread
From: George Dunlap @ 2011-05-26 10:06 UTC (permalink / raw)
To: Olaf Hering; +Cc: xen-devel, George Dunlap
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
On Tue, May 10, 2011 at 3:32 PM, Olaf Hering <olaf@aepfle.de> wrote:
> # HG changeset patch
> # User Olaf Hering <olaf@aepfle.de>
> # Date 1305037567 -7200
> # Node ID 7f6a118d237e60ca6f30723db687b06a8ef3c3e9
> # Parent 1a45e40add8b407532374c34f20bad51707808cf
> xentrace: allocate non-contiguous per-cpu trace buffers
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
>
> diff -r 1a45e40add8b -r 7f6a118d237e xen/common/trace.c
> --- a/xen/common/trace.c Tue May 10 16:25:39 2011 +0200
> +++ b/xen/common/trace.c Tue May 10 16:26:07 2011 +0200
> @@ -166,7 +166,7 @@ static int calculate_tbuf_size(unsigned
> */
> static int alloc_trace_bufs(unsigned int pages)
> {
> - int i, cpu, order;
> + int i, cpu;
> /* Start after a fixed-size array of NR_CPUS */
> uint32_t *t_info_mfn_list;
> uint16_t t_info_first_offset;
> @@ -182,34 +182,11 @@ static int alloc_trace_bufs(unsigned int
> t_info_first_offset = calc_tinfo_first_offset();
>
> pages = calculate_tbuf_size(pages, t_info_first_offset);
> - order = get_order_from_pages(pages);
>
> t_info = alloc_xenheap_pages(get_order_from_pages(t_info_pages), 0);
> if ( t_info == NULL )
> - goto out_dealloc;
> + goto out_dealloc_t_info;
>
> - /*
> - * First, allocate buffers for all of the cpus. If any
> - * fails, deallocate what you have so far and exit.
> - */
> - for_each_online_cpu(cpu)
> - {
> - void *rawbuf;
> - struct t_buf *buf;
> -
> - if ( (rawbuf = alloc_xenheap_pages(
> - order, MEMF_bits(32 + PAGE_SHIFT))) == NULL )
> - {
> - printk(XENLOG_INFO "xentrace: memory allocation failed "
> - "on cpu %d\n", cpu);
> - goto out_dealloc;
> - }
> -
> - per_cpu(t_bufs, cpu) = buf = rawbuf;
> - buf->cons = buf->prod = 0;
> - }
> -
> - offset = t_info_first_offset;
> t_info_mfn_list = (uint32_t *)t_info;
>
> for(i = 0; i < t_info_pages; i++)
> @@ -219,27 +196,53 @@ static int alloc_trace_bufs(unsigned int
> t_info->tbuf_size = pages;
>
> /*
> - * Now share the pages so xentrace can map them, and write them in
> - * the global t_info structure.
> + * Allocate buffers for all of the cpus.
> + * If any fails, deallocate what you have so far and exit.
> */
> for_each_online_cpu(cpu)
> {
> - void *rawbuf = per_cpu(t_bufs, cpu);
> - struct page_info *p = virt_to_page(rawbuf);
> - uint32_t mfn = virt_to_mfn(rawbuf);
> + offset = t_info_first_offset + (cpu * pages);
> + t_info->mfn_offset[cpu] = offset;
>
> for ( i = 0; i < pages; i++ )
> {
> - share_xen_page_with_privileged_guests(p + i, XENSHARE_writable);
> + void *p = alloc_xenheap_pages(0, MEMF_bits(32 + PAGE_SHIFT));
> + if ( !p )
> + {
> + printk(XENLOG_INFO "xentrace: memory allocation failed "
> + "on cpu %d after %d pages\n", cpu, i);
> + t_info_mfn_list[offset + i] = 0;
> + goto out_dealloc;
> + }
> + t_info_mfn_list[offset + i] = virt_to_mfn(p);
> + }
> + }
>
> - t_info_mfn_list[offset + i]=mfn + i;
> - }
> - t_info->mfn_offset[cpu]=offset;
> - printk(XENLOG_INFO "xentrace: p%d mfn %"PRIx32" offset %d\n",
> - cpu, mfn, offset);
> - offset+=i;
> + /*
> + * Initialize buffers for all of the cpus.
> + */
> + for_each_online_cpu(cpu)
> + {
> + struct t_buf *buf;
> + struct page_info *pg;
>
> spin_lock_init(&per_cpu(t_lock, cpu));
> +
> + offset = t_info->mfn_offset[cpu];
> +
> + /* Initialize the buffer metadata */
> + per_cpu(t_bufs, cpu) = buf = mfn_to_virt(t_info_mfn_list[offset]);
> + buf->cons = buf->prod = 0;
> +
> + printk(XENLOG_INFO "xentrace: p%d mfn %x offset %u\n",
> + cpu, t_info_mfn_list[offset], offset);
> +
> + /* Now share the trace pages */
> + for ( i = 0; i < pages; i++ )
> + {
> + pg = mfn_to_page(t_info_mfn_list[offset + i]);
> + share_xen_page_with_privileged_guests(pg, XENSHARE_writable);
> + }
> }
>
> data_size = (pages * PAGE_SIZE - sizeof(struct t_buf));
> @@ -255,14 +258,19 @@ static int alloc_trace_bufs(unsigned int
> out_dealloc:
> for_each_online_cpu(cpu)
> {
> - void *rawbuf = per_cpu(t_bufs, cpu);
> - per_cpu(t_bufs, cpu) = NULL;
> - if ( rawbuf )
> + offset = t_info->mfn_offset[cpu];
> + if ( !offset )
> + continue;
> + for ( i = 0; i < pages; i++ )
> {
> - ASSERT(!(virt_to_page(rawbuf)->count_info & PGC_allocated));
> - free_xenheap_pages(rawbuf, order);
> + uint32_t mfn = t_info_mfn_list[offset + i];
> + if ( !mfn )
> + break;
> + ASSERT(!(mfn_to_page(mfn)->count_info & PGC_allocated));
> + free_xenheap_pages(mfn_to_virt(mfn), 0);
> }
> }
> +out_dealloc_t_info:
> free_xenheap_pages(t_info, get_order_from_pages(t_info_pages));
> t_info = NULL;
> printk(XENLOG_WARNING "xentrace: allocation failed! Tracing disabled.\n");
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0 of 4] xentrace [v2]: non-contiguous allocation of per-cpu buffer
2011-05-10 14:32 [PATCH 0 of 4] xentrace [v2]: non-contiguous allocation of per-cpu buffer Olaf Hering
` (3 preceding siblings ...)
2011-05-10 14:32 ` [PATCH 4 of 4] xentrace: allocate non-contiguous per-cpu trace buffers Olaf Hering
@ 2011-05-20 8:36 ` Keir Fraser
4 siblings, 0 replies; 10+ messages in thread
From: Keir Fraser @ 2011-05-20 8:36 UTC (permalink / raw)
To: Olaf Hering, xen-devel; +Cc: George Dunlap
On 10/05/2011 15:32, "Olaf Hering" <olaf@aepfle.de> wrote:
> This series implements non-contiguous trace buffers.
I haven't seen an Ack for these yet.
-- Keir
> There was an overflow in the offset calculation because it was stored in
> an int. Also the calculation of the per-cpu buffer did not take the type
> of the offset value into account. This was the reason why I ran into the
> checks in bogus(). The crash in out_dealloc: was caused by the lack of
> check for the offset value.
>
> Regarding the math added to next_record() another array of pointers could be
> added to remove the mfn_to_virt() calls. In my testing 9343 trace pages on 8
> cpus are used. Such an array would required additional (9343*8*8)/1024=583Kb.
> Which is not too much, given the amount of 291MB tracebuffers.
> Such a change is not part of this series.
>
> Olaf
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread