* [PATCH 0 of 5] xentrace updates
@ 2011-03-22 19:21 Olaf Hering
2011-03-22 19:21 ` [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case Olaf Hering
` (4 more replies)
0 siblings, 5 replies; 22+ messages in thread
From: Olaf Hering @ 2011-03-22 19:21 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap
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
^ permalink raw reply [flat|nested] 22+ messages in thread* [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case 2011-03-22 19:21 [PATCH 0 of 5] xentrace updates Olaf Hering @ 2011-03-22 19:21 ` Olaf Hering 2011-03-23 10:12 ` Jan Beulich 2011-03-22 19:21 ` [PATCH 2 of 5] xentrace: print calculated numbers in calculate_tbuf_size() Olaf Hering ` (3 subsequent siblings) 4 siblings, 1 reply; 22+ messages in thread From: Olaf Hering @ 2011-03-22 19:21 UTC (permalink / raw) To: xen-devel; +Cc: George Dunlap # 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; } ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case 2011-03-22 19:21 ` [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case Olaf Hering @ 2011-03-23 10:12 ` Jan Beulich 2011-03-23 10:22 ` Keir Fraser 0 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2011-03-23 10:12 UTC (permalink / raw) To: Olaf Hering; +Cc: xen-devel, George Dunlap >>> 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; > } ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case 2011-03-23 10:12 ` Jan Beulich @ 2011-03-23 10:22 ` Keir Fraser 2011-03-23 11:20 ` Olaf Hering 0 siblings, 1 reply; 22+ messages in thread From: Keir Fraser @ 2011-03-23 10:22 UTC (permalink / raw) To: Jan Beulich, Olaf Hering; +Cc: xen-devel, George Dunlap 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case 2011-03-23 10:22 ` Keir Fraser @ 2011-03-23 11:20 ` Olaf Hering 2011-03-23 12:33 ` Keir Fraser 2011-03-23 12:45 ` Jan Beulich 0 siblings, 2 replies; 22+ messages in thread From: Olaf Hering @ 2011-03-23 11:20 UTC (permalink / raw) To: Keir Fraser; +Cc: xen-devel, George Dunlap, Jan Beulich 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case 2011-03-23 11:20 ` Olaf Hering @ 2011-03-23 12:33 ` Keir Fraser 2011-03-23 12:46 ` Olaf Hering 2011-03-23 12:45 ` Jan Beulich 1 sibling, 1 reply; 22+ messages in thread From: Keir Fraser @ 2011-03-23 12:33 UTC (permalink / raw) To: Olaf Hering; +Cc: xen-devel, George Dunlap, Jan Beulich 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case 2011-03-23 12:33 ` Keir Fraser @ 2011-03-23 12:46 ` Olaf Hering 2011-03-23 14:56 ` George Dunlap 0 siblings, 1 reply; 22+ messages in thread From: Olaf Hering @ 2011-03-23 12:46 UTC (permalink / raw) To: Keir Fraser; +Cc: xen-devel, George Dunlap, Jan Beulich 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case 2011-03-23 12:46 ` Olaf Hering @ 2011-03-23 14:56 ` George Dunlap 0 siblings, 0 replies; 22+ messages in thread From: George Dunlap @ 2011-03-23 14:56 UTC (permalink / raw) To: Olaf Hering; +Cc: xen-devel@lists.xensource.com, Keir Fraser, Jan Beulich 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case 2011-03-23 11:20 ` Olaf Hering 2011-03-23 12:33 ` Keir Fraser @ 2011-03-23 12:45 ` Jan Beulich 2011-03-23 13:16 ` Olaf Hering 1 sibling, 1 reply; 22+ messages in thread From: Jan Beulich @ 2011-03-23 12:45 UTC (permalink / raw) To: Olaf Hering; +Cc: xen-devel, Keir Fraser, George Dunlap >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case 2011-03-23 12:45 ` Jan Beulich @ 2011-03-23 13:16 ` Olaf Hering 2011-03-23 13:35 ` Keir Fraser 0 siblings, 1 reply; 22+ messages in thread From: Olaf Hering @ 2011-03-23 13:16 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Keir Fraser, George Dunlap 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case 2011-03-23 13:16 ` Olaf Hering @ 2011-03-23 13:35 ` Keir Fraser 0 siblings, 0 replies; 22+ messages in thread From: Keir Fraser @ 2011-03-23 13:35 UTC (permalink / raw) To: Olaf Hering, Jan Beulich; +Cc: xen-devel, George Dunlap 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2 of 5] xentrace: print calculated numbers in calculate_tbuf_size() 2011-03-22 19:21 [PATCH 0 of 5] xentrace updates Olaf Hering 2011-03-22 19:21 ` [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case Olaf Hering @ 2011-03-22 19:21 ` Olaf Hering 2011-03-22 19:21 ` [PATCH 3 of 5] xentrace: remove gdprintk usage since they are not in guest context Olaf Hering ` (2 subsequent siblings) 4 siblings, 0 replies; 22+ messages in thread From: Olaf Hering @ 2011-03-22 19:21 UTC (permalink / raw) To: xen-devel; +Cc: George Dunlap # 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; } ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3 of 5] xentrace: remove gdprintk usage since they are not in guest context 2011-03-22 19:21 [PATCH 0 of 5] xentrace updates Olaf Hering 2011-03-22 19:21 ` [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case Olaf Hering 2011-03-22 19:21 ` [PATCH 2 of 5] xentrace: print calculated numbers in calculate_tbuf_size() Olaf Hering @ 2011-03-22 19:21 ` Olaf Hering 2011-03-22 19:21 ` [PATCH 4 of 5] xentrace: update comments Olaf Hering 2011-03-22 19:21 ` [PATCH 5 of 5] xentrace: use consistent printk prefix Olaf Hering 4 siblings, 0 replies; 22+ messages in thread From: Olaf Hering @ 2011-03-22 19:21 UTC (permalink / raw) To: xen-devel; +Cc: George Dunlap # 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; ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4 of 5] xentrace: update comments 2011-03-22 19:21 [PATCH 0 of 5] xentrace updates Olaf Hering ` (2 preceding siblings ...) 2011-03-22 19:21 ` [PATCH 3 of 5] xentrace: remove gdprintk usage since they are not in guest context Olaf Hering @ 2011-03-22 19:21 ` Olaf Hering 2011-03-22 19:21 ` [PATCH 5 of 5] xentrace: use consistent printk prefix Olaf Hering 4 siblings, 0 replies; 22+ messages in thread From: Olaf Hering @ 2011-03-22 19:21 UTC (permalink / raw) To: xen-devel; +Cc: George Dunlap # 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); ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 5 of 5] xentrace: use consistent printk prefix 2011-03-22 19:21 [PATCH 0 of 5] xentrace updates Olaf Hering ` (3 preceding siblings ...) 2011-03-22 19:21 ` [PATCH 4 of 5] xentrace: update comments Olaf Hering @ 2011-03-22 19:21 ` Olaf Hering 2011-03-23 10:14 ` Jan Beulich 4 siblings, 1 reply; 22+ messages in thread From: Olaf Hering @ 2011-03-22 19:21 UTC (permalink / raw) To: xen-devel; +Cc: George Dunlap # 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; } ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5 of 5] xentrace: use consistent printk prefix 2011-03-22 19:21 ` [PATCH 5 of 5] xentrace: use consistent printk prefix Olaf Hering @ 2011-03-23 10:14 ` Jan Beulich 2011-03-23 11:18 ` Olaf Hering 0 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2011-03-23 10:14 UTC (permalink / raw) To: Olaf Hering; +Cc: xen-devel, George Dunlap >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5 of 5] xentrace: use consistent printk prefix 2011-03-23 10:14 ` Jan Beulich @ 2011-03-23 11:18 ` Olaf Hering 2011-03-23 14:47 ` George Dunlap 0 siblings, 1 reply; 22+ messages in thread From: Olaf Hering @ 2011-03-23 11:18 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, George Dunlap 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5 of 5] xentrace: use consistent printk prefix 2011-03-23 11:18 ` Olaf Hering @ 2011-03-23 14:47 ` George Dunlap 2011-03-23 16:20 ` Jan Beulich 0 siblings, 1 reply; 22+ messages in thread From: George Dunlap @ 2011-03-23 14:47 UTC (permalink / raw) To: Olaf Hering; +Cc: xen-devel@lists.xensource.com, Jan Beulich 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5 of 5] xentrace: use consistent printk prefix 2011-03-23 14:47 ` George Dunlap @ 2011-03-23 16:20 ` Jan Beulich 2011-03-23 17:02 ` George Dunlap 0 siblings, 1 reply; 22+ messages in thread From: Jan Beulich @ 2011-03-23 16:20 UTC (permalink / raw) To: Olaf Hering, George Dunlap; +Cc: xen-devel@lists.xensource.com >>> 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5 of 5] xentrace: use consistent printk prefix 2011-03-23 16:20 ` Jan Beulich @ 2011-03-23 17:02 ` George Dunlap 2011-03-23 17:11 ` Olaf Hering 0 siblings, 1 reply; 22+ messages in thread From: George Dunlap @ 2011-03-23 17:02 UTC (permalink / raw) To: Jan Beulich; +Cc: Olaf Hering, xen-devel@lists.xensource.com 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 > > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5 of 5] xentrace: use consistent printk prefix 2011-03-23 17:02 ` George Dunlap @ 2011-03-23 17:11 ` Olaf Hering 0 siblings, 0 replies; 22+ messages in thread From: Olaf Hering @ 2011-03-23 17:11 UTC (permalink / raw) To: George Dunlap; +Cc: xen-devel@lists.xensource.com, Jan Beulich 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 0 of 5] xentrace updates @ 2011-03-23 17:54 Olaf Hering 2011-03-23 17:54 ` [PATCH 5 of 5] xentrace: use consistent printk prefix Olaf Hering 0 siblings, 1 reply; 22+ messages in thread From: Olaf Hering @ 2011-03-23 17:54 UTC (permalink / raw) To: xen-devel; +Cc: George Dunlap 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 5 of 5] xentrace: use consistent printk prefix 2011-03-23 17:54 [PATCH 0 of 5] xentrace updates Olaf Hering @ 2011-03-23 17:54 ` Olaf Hering 0 siblings, 0 replies; 22+ messages in thread From: Olaf Hering @ 2011-03-23 17:54 UTC (permalink / raw) To: xen-devel; +Cc: George Dunlap # 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; } ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2011-03-23 17:54 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-22 19:21 [PATCH 0 of 5] xentrace updates Olaf Hering 2011-03-22 19:21 ` [PATCH 1 of 5] xentrace: fix t_info_pages calculation for the default case Olaf Hering 2011-03-23 10:12 ` Jan Beulich 2011-03-23 10:22 ` Keir Fraser 2011-03-23 11:20 ` Olaf Hering 2011-03-23 12:33 ` Keir Fraser 2011-03-23 12:46 ` Olaf Hering 2011-03-23 14:56 ` George Dunlap 2011-03-23 12:45 ` Jan Beulich 2011-03-23 13:16 ` Olaf Hering 2011-03-23 13:35 ` Keir Fraser 2011-03-22 19:21 ` [PATCH 2 of 5] xentrace: print calculated numbers in calculate_tbuf_size() Olaf Hering 2011-03-22 19:21 ` [PATCH 3 of 5] xentrace: remove gdprintk usage since they are not in guest context Olaf Hering 2011-03-22 19:21 ` [PATCH 4 of 5] xentrace: update comments Olaf Hering 2011-03-22 19:21 ` [PATCH 5 of 5] xentrace: use consistent printk prefix Olaf Hering 2011-03-23 10:14 ` Jan Beulich 2011-03-23 11:18 ` Olaf Hering 2011-03-23 14:47 ` George Dunlap 2011-03-23 16:20 ` Jan Beulich 2011-03-23 17:02 ` George Dunlap 2011-03-23 17:11 ` Olaf Hering -- strict thread matches above, loose matches on Subject: below -- 2011-03-23 17:54 [PATCH 0 of 5] xentrace updates Olaf Hering 2011-03-23 17:54 ` [PATCH 5 of 5] xentrace: use consistent printk prefix Olaf Hering
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).