* [RFC][PATCH] eliminate extra tb_init_done check
@ 2008-10-15 6:20 Lu, Guanqun
2008-10-15 7:17 ` Keir Fraser
0 siblings, 1 reply; 3+ messages in thread
From: Lu, Guanqun @ 2008-10-15 6:20 UTC (permalink / raw)
To: xen-devel@lists.xensource.com
[-- Attachment #1: Type: text/plain, Size: 755 bytes --]
Hi,
As c/s 18441 does the following changes:
- ASSERT(tb_init_done);
+ if( !tb_init_done )
+ return;
in the function __trace_var in the file xen/common/trace.c.
We don't need to check the variable tb_init_done before we invoke __trace_var.
This patch simplies such logic:
if ( tb_init_done )
__trace_var(...)
to
__trace_var(...)
.
Two corner conditions are left untouched. One is the assembly in entry.S,
the other is the check of tb_init_done not immediately followed by __trace_var.
Or more aggressively, we can eliminate all the extra checks, make tb_init_done
a static variable, and rename __trace_var to trace_var which looks more like
a right interface name.
--
Guanqun
[-- Attachment #2: eliminate-extra-tb_init_done-check.patch --]
[-- Type: application/octet-stream, Size: 17999 bytes --]
diff -r 6c3e90f6a117 xen/arch/x86/mm/shadow/common.c
--- a/xen/arch/x86/mm/shadow/common.c Wed Oct 15 13:29:20 2008 +0800
+++ b/xen/arch/x86/mm/shadow/common.c Wed Oct 15 14:00:22 2008 +0800
@@ -694,12 +694,9 @@
static inline void trace_resync(int event, mfn_t gmfn)
{
- if ( tb_init_done )
- {
- /* Convert gmfn to gfn */
- unsigned long gfn = mfn_to_gfn(current->domain, gmfn);
- __trace_var(event, 0/*!tsc*/, sizeof(gfn), (unsigned char*)&gfn);
- }
+ /* Convert gmfn to gfn */
+ unsigned long gfn = mfn_to_gfn(current->domain, gmfn);
+ __trace_var(event, 0/*!tsc*/, sizeof(gfn), (unsigned char*)&gfn);
}
/* Pull all the entries on an out-of-sync page back into sync. */
@@ -1320,15 +1317,12 @@
static inline void trace_shadow_prealloc_unpin(struct domain *d, mfn_t smfn)
{
- if ( tb_init_done )
- {
- /* Convert smfn to gfn */
- unsigned long gfn;
- ASSERT(mfn_valid(smfn));
- gfn = mfn_to_gfn(d, _mfn(mfn_to_shadow_page(smfn)->backpointer));
- __trace_var(TRC_SHADOW_PREALLOC_UNPIN, 0/*!tsc*/,
- sizeof(gfn), (unsigned char*)&gfn);
- }
+ /* Convert smfn to gfn */
+ unsigned long gfn;
+ ASSERT(mfn_valid(smfn));
+ gfn = mfn_to_gfn(d, _mfn(mfn_to_shadow_page(smfn)->backpointer));
+ __trace_var(TRC_SHADOW_PREALLOC_UNPIN, 0/*!tsc*/,
+ sizeof(gfn), (unsigned char*)&gfn);
}
/* Make sure there are at least count order-sized pages
@@ -2239,12 +2233,9 @@
static inline void trace_shadow_wrmap_bf(mfn_t gmfn)
{
- if ( tb_init_done )
- {
- /* Convert gmfn to gfn */
- unsigned long gfn = mfn_to_gfn(current->domain, gmfn);
- __trace_var(TRC_SHADOW_WRMAP_BF, 0/*!tsc*/, sizeof(gfn), (unsigned char*)&gfn);
- }
+ /* Convert gmfn to gfn */
+ unsigned long gfn = mfn_to_gfn(current->domain, gmfn);
+ __trace_var(TRC_SHADOW_WRMAP_BF, 0/*!tsc*/, sizeof(gfn), (unsigned char*)&gfn);
}
/**************************************************************************/
diff -r 6c3e90f6a117 xen/arch/x86/mm/shadow/multi.c
--- a/xen/arch/x86/mm/shadow/multi.c Wed Oct 15 13:29:20 2008 +0800
+++ b/xen/arch/x86/mm/shadow/multi.c Wed Oct 15 14:00:22 2008 +0800
@@ -3035,85 +3035,73 @@
static inline void trace_shadow_gen(u32 event, guest_va_t va)
{
- if ( tb_init_done )
- {
- event |= (GUEST_PAGING_LEVELS-2)<<8;
- __trace_var(event, 0/*!tsc*/, sizeof(va), (unsigned char*)&va);
- }
+ event |= (GUEST_PAGING_LEVELS-2)<<8;
+ __trace_var(event, 0/*!tsc*/, sizeof(va), (unsigned char*)&va);
}
static inline void trace_shadow_fixup(guest_l1e_t gl1e,
guest_va_t va)
{
- if ( tb_init_done )
- {
- struct {
- /* for PAE, guest_l1e may be 64 while guest_va may be 32;
- so put it first for alignment sake. */
- guest_l1e_t gl1e;
- guest_va_t va;
- u32 flags;
- } __attribute__((packed)) d;
- u32 event;
-
- event = TRC_SHADOW_FIXUP | ((GUEST_PAGING_LEVELS-2)<<8);
-
- d.gl1e = gl1e;
- d.va = va;
- d.flags = this_cpu(trace_shadow_path_flags);
-
- __trace_var(event, 0/*!tsc*/, sizeof(d), (unsigned char*)&d);
- }
+ struct {
+ /* for PAE, guest_l1e may be 64 while guest_va may be 32;
+ so put it first for alignment sake. */
+ guest_l1e_t gl1e;
+ guest_va_t va;
+ u32 flags;
+ } __attribute__((packed)) d;
+ u32 event;
+
+ event = TRC_SHADOW_FIXUP | ((GUEST_PAGING_LEVELS-2)<<8);
+
+ d.gl1e = gl1e;
+ d.va = va;
+ d.flags = this_cpu(trace_shadow_path_flags);
+
+ __trace_var(event, 0/*!tsc*/, sizeof(d), (unsigned char*)&d);
}
static inline void trace_not_shadow_fault(guest_l1e_t gl1e,
guest_va_t va)
{
- if ( tb_init_done )
- {
- struct {
- /* for PAE, guest_l1e may be 64 while guest_va may be 32;
- so put it first for alignment sake. */
- guest_l1e_t gl1e;
- guest_va_t va;
- u32 flags;
- } __attribute__((packed)) d;
- u32 event;
-
- event = TRC_SHADOW_NOT_SHADOW | ((GUEST_PAGING_LEVELS-2)<<8);
-
- d.gl1e = gl1e;
- d.va = va;
- d.flags = this_cpu(trace_shadow_path_flags);
-
- __trace_var(event, 0/*!tsc*/, sizeof(d), (unsigned char*)&d);
- }
+ struct {
+ /* for PAE, guest_l1e may be 64 while guest_va may be 32;
+ so put it first for alignment sake. */
+ guest_l1e_t gl1e;
+ guest_va_t va;
+ u32 flags;
+ } __attribute__((packed)) d;
+ u32 event;
+
+ event = TRC_SHADOW_NOT_SHADOW | ((GUEST_PAGING_LEVELS-2)<<8);
+
+ d.gl1e = gl1e;
+ d.va = va;
+ d.flags = this_cpu(trace_shadow_path_flags);
+
+ __trace_var(event, 0/*!tsc*/, sizeof(d), (unsigned char*)&d);
}
static inline void trace_shadow_emulate_other(u32 event,
guest_va_t va,
gfn_t gfn)
{
- if ( tb_init_done )
- {
- struct {
- /* for PAE, guest_l1e may be 64 while guest_va may be 32;
- so put it first for alignment sake. */
-#if GUEST_PAGING_LEVELS == 2
- u32 gfn;
-#else
- u64 gfn;
-#endif
- guest_va_t va;
- } __attribute__((packed)) d;
-
- event |= ((GUEST_PAGING_LEVELS-2)<<8);
-
- d.gfn=gfn_x(gfn);
- d.va = va;
-
- __trace_var(event, 0/*!tsc*/, sizeof(d), (unsigned char*)&d);
- }
+ struct {
+ /* for PAE, guest_l1e may be 64 while guest_va may be 32;
+ so put it first for alignment sake. */
+#if GUEST_PAGING_LEVELS == 2
+ u32 gfn;
+#else
+ u64 gfn;
+#endif
+ guest_va_t va;
+ } __attribute__((packed)) d;
+
+ event |= ((GUEST_PAGING_LEVELS-2)<<8);
+
+ d.gfn=gfn_x(gfn);
+ d.va = va;
+
+ __trace_var(event, 0/*!tsc*/, sizeof(d), (unsigned char*)&d);
}
#if GUEST_PAGING_LEVELS == 3
@@ -3124,29 +3112,26 @@
static inline void trace_shadow_emulate(guest_l1e_t gl1e, unsigned long va)
{
- if ( tb_init_done )
- {
- struct {
- /* for PAE, guest_l1e may be 64 while guest_va may be 32;
- so put it first for alignment sake. */
- guest_l1e_t gl1e, write_val;
- guest_va_t va;
- unsigned flags:29, emulation_count:3;
- } __attribute__((packed)) d;
- u32 event;
-
- event = TRC_SHADOW_EMULATE | ((GUEST_PAGING_LEVELS-2)<<8);
-
- d.gl1e = gl1e;
- d.write_val.l1 = this_cpu(trace_emulate_write_val);
- d.va = va;
-#if GUEST_PAGING_LEVELS == 3
- d.emulation_count = this_cpu(trace_extra_emulation_count);
-#endif
- d.flags = this_cpu(trace_shadow_path_flags);
-
- __trace_var(event, 0/*!tsc*/, sizeof(d), (unsigned char*)&d);
- }
+ struct {
+ /* for PAE, guest_l1e may be 64 while guest_va may be 32;
+ so put it first for alignment sake. */
+ guest_l1e_t gl1e, write_val;
+ guest_va_t va;
+ unsigned flags:29, emulation_count:3;
+ } __attribute__((packed)) d;
+ u32 event;
+
+ event = TRC_SHADOW_EMULATE | ((GUEST_PAGING_LEVELS-2)<<8);
+
+ d.gl1e = gl1e;
+ d.write_val.l1 = this_cpu(trace_emulate_write_val);
+ d.va = va;
+#if GUEST_PAGING_LEVELS == 3
+ d.emulation_count = this_cpu(trace_extra_emulation_count);
+#endif
+ d.flags = this_cpu(trace_shadow_path_flags);
+
+ __trace_var(event, 0/*!tsc*/, sizeof(d), (unsigned char*)&d);
}
/**************************************************************************/
diff -r 6c3e90f6a117 xen/arch/x86/trace.c
--- a/xen/arch/x86/trace.c Wed Oct 15 13:29:20 2008 +0800
+++ b/xen/arch/x86/trace.c Wed Oct 15 14:00:22 2008 +0800
@@ -46,7 +46,7 @@
}
}
-void __trace_pv_trap(int trapnr, unsigned long eip,
+void trace_pv_trap(int trapnr, unsigned long eip,
int use_error_code, unsigned error_code)
{
#ifdef __x86_64__
@@ -89,7 +89,7 @@
}
}
-void __trace_pv_page_fault(unsigned long addr, unsigned error_code)
+void trace_pv_page_fault(unsigned long addr, unsigned error_code)
{
unsigned long eip = guest_cpu_user_regs()->eip;
@@ -124,7 +124,7 @@
}
}
-void __trace_trap_one_addr(unsigned event, unsigned long va)
+void trace_trap_one_addr(unsigned event, unsigned long va)
{
#ifdef __x86_64__
if ( is_pv_32on64_vcpu(current) )
@@ -140,7 +140,7 @@
}
}
-void __trace_trap_two_addr(unsigned event, unsigned long va1,
+void trace_trap_two_addr(unsigned event, unsigned long va1,
unsigned long va2)
{
#ifdef __x86_64__
@@ -166,7 +166,7 @@
}
}
-void __trace_ptwr_emulation(unsigned long addr, l1_pgentry_t npte)
+void trace_ptwr_emulation(unsigned long addr, l1_pgentry_t npte)
{
unsigned long eip = guest_cpu_user_regs()->eip;
diff -r 6c3e90f6a117 xen/common/schedule.c
--- a/xen/common/schedule.c Wed Oct 15 13:29:20 2008 +0800
+++ b/xen/common/schedule.c Wed Oct 15 14:00:22 2008 +0800
@@ -67,9 +67,6 @@
{
struct { uint32_t vcpu:16, domain:16; } d;
uint32_t event;
-
- if ( likely(!tb_init_done) )
- return;
d.vcpu = v->vcpu_id;
d.domain = v->domain->domain_id;
diff -r 6c3e90f6a117 xen/include/asm-x86/hvm/trace.h
--- a/xen/include/asm-x86/hvm/trace.h Wed Oct 15 13:29:20 2008 +0800
+++ b/xen/include/asm-x86/hvm/trace.h Wed Oct 15 14:00:22 2008 +0800
@@ -58,7 +58,7 @@
#define HVMTRACE_ND(evt, cycles, count, d1, d2, d3, d4, d5, d6) \
do { \
- if ( unlikely(tb_init_done) && DO_TRC_HVM_ ## evt ) \
+ if ( DO_TRC_HVM_ ## evt ) \
{ \
struct { \
u32 d[6]; \
diff -r 6c3e90f6a117 xen/include/asm-x86/trace.h
--- a/xen/include/asm-x86/trace.h Wed Oct 15 13:29:20 2008 +0800
+++ b/xen/include/asm-x86/trace.h Wed Oct 15 14:00:22 2008 +0800
@@ -3,44 +3,12 @@
#include <asm/page.h>
-void __trace_pv_trap(int trapnr, unsigned long eip,
- int use_error_code, unsigned error_code);
-static inline void trace_pv_trap(int trapnr, unsigned long eip,
- int use_error_code, unsigned error_code)
-{
- if ( unlikely(tb_init_done) )
- __trace_pv_trap(trapnr, eip, use_error_code, error_code);
-}
-
-void __trace_pv_page_fault(unsigned long addr, unsigned error_code);
-static inline void trace_pv_page_fault(unsigned long addr,
- unsigned error_code)
-{
- if ( unlikely(tb_init_done) )
- __trace_pv_page_fault(addr, error_code);
-}
-
-void __trace_trap_one_addr(unsigned event, unsigned long va);
-static inline void trace_trap_one_addr(unsigned event, unsigned long va)
-{
- if ( unlikely(tb_init_done) )
- __trace_trap_one_addr(event, va);
-}
-
-void __trace_trap_two_addr(unsigned event, unsigned long va1,
- unsigned long va2);
-static inline void trace_trap_two_addr(unsigned event, unsigned long va1,
- unsigned long va2)
-{
- if ( unlikely(tb_init_done) )
- __trace_trap_two_addr(event, va1, va2);
-}
-
-void __trace_ptwr_emulation(unsigned long addr, l1_pgentry_t npte);
-static inline void trace_ptwr_emulation(unsigned long addr, l1_pgentry_t npte)
-{
- if ( unlikely(tb_init_done) )
- __trace_ptwr_emulation(addr, npte);
-}
+void trace_pv_trap(int trapnr, unsigned long eip,
+ int use_error_code, unsigned error_code);
+void trace_pv_page_fault(unsigned long addr, unsigned error_code);
+void trace_trap_one_addr(unsigned event, unsigned long va);
+void trace_trap_two_addr(unsigned event, unsigned long va1,
+ unsigned long va2);
+void trace_ptwr_emulation(unsigned long addr, l1_pgentry_t npte);
#endif /* __ASM_TRACE_H__ */
diff -r 6c3e90f6a117 xen/include/xen/trace.h
--- a/xen/include/xen/trace.h Wed Oct 15 13:29:20 2008 +0800
+++ b/xen/include/xen/trace.h Wed Oct 15 14:00:22 2008 +0800
@@ -41,8 +41,7 @@
static inline void trace_var(u32 event, int cycles, int extra,
unsigned char *extra_data)
{
- if ( unlikely(tb_init_done) )
- __trace_var(event, cycles, extra, extra_data);
+ __trace_var(event, cycles, extra, extra_data);
}
/* Convenience macros for calling the trace function. */
@@ -53,62 +52,47 @@
#define TRACE_1D(_e,d1) \
do { \
- if ( unlikely(tb_init_done) ) \
- { \
- u32 _d[1]; \
- _d[0] = d1; \
- __trace_var(_e, 1, sizeof(*_d), (unsigned char *)_d); \
- } \
+ u32 _d[1]; \
+ _d[0] = d1; \
+ __trace_var(_e, 1, sizeof(*_d), (unsigned char *)_d); \
} while ( 0 )
#define TRACE_2D(_e,d1,d2) \
do { \
- if ( unlikely(tb_init_done) ) \
- { \
- u32 _d[2]; \
- _d[0] = d1; \
- _d[1] = d2; \
- __trace_var(_e, 1, sizeof(*_d)*2, (unsigned char *)_d); \
- } \
+ u32 _d[2]; \
+ _d[0] = d1; \
+ _d[1] = d2; \
+ __trace_var(_e, 1, sizeof(*_d)*2, (unsigned char *)_d); \
} while ( 0 )
#define TRACE_3D(_e,d1,d2,d3) \
do { \
- if ( unlikely(tb_init_done) ) \
- { \
- u32 _d[3]; \
- _d[0] = d1; \
- _d[1] = d2; \
- _d[2] = d3; \
- __trace_var(_e, 1, sizeof(*_d)*3, (unsigned char *)_d); \
- } \
+ u32 _d[3]; \
+ _d[0] = d1; \
+ _d[1] = d2; \
+ _d[2] = d3; \
+ __trace_var(_e, 1, sizeof(*_d)*3, (unsigned char *)_d); \
} while ( 0 )
#define TRACE_4D(_e,d1,d2,d3,d4) \
do { \
- if ( unlikely(tb_init_done) ) \
- { \
- u32 _d[4]; \
- _d[0] = d1; \
- _d[1] = d2; \
- _d[2] = d3; \
- _d[3] = d4; \
- __trace_var(_e, 1, sizeof(*_d)*4, (unsigned char *)_d); \
- } \
+ u32 _d[4]; \
+ _d[0] = d1; \
+ _d[1] = d2; \
+ _d[2] = d3; \
+ _d[3] = d4; \
+ __trace_var(_e, 1, sizeof(*_d)*4, (unsigned char *)_d); \
} while ( 0 )
#define TRACE_5D(_e,d1,d2,d3,d4,d5) \
do { \
- if ( unlikely(tb_init_done) ) \
- { \
- u32 _d[5]; \
- _d[0] = d1; \
- _d[1] = d2; \
- _d[2] = d3; \
- _d[3] = d4; \
- _d[4] = d5; \
- __trace_var(_e, 1, sizeof(*_d)*5, (unsigned char *)_d); \
- } \
+ u32 _d[5]; \
+ _d[0] = d1; \
+ _d[1] = d2; \
+ _d[2] = d3; \
+ _d[3] = d4; \
+ _d[4] = d5; \
+ __trace_var(_e, 1, sizeof(*_d)*5, (unsigned char *)_d); \
} while ( 0 )
#endif /* __XEN_TRACE_H__ */
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC][PATCH] eliminate extra tb_init_done check
2008-10-15 6:20 [RFC][PATCH] eliminate extra tb_init_done check Lu, Guanqun
@ 2008-10-15 7:17 ` Keir Fraser
2008-10-15 15:54 ` George Dunlap
0 siblings, 1 reply; 3+ messages in thread
From: Keir Fraser @ 2008-10-15 7:17 UTC (permalink / raw)
To: Lu, Guanqun, xen-devel@lists.xensource.com; +Cc: George Dunlap
On 15/10/08 07:20, "Lu, Guanqun" <guanqun.lu@intel.com> wrote:
> Two corner conditions are left untouched. One is the assembly in entry.S,
> the other is the check of tb_init_done not immediately followed by
> __trace_var.
>
> Or more aggressively, we can eliminate all the extra checks, make tb_init_done
> a static variable, and rename __trace_var to trace_var which looks more like
> a right interface name.
The macros check tb_init_done before calling __trace_var() to try and reduce
the cost of the common case (tracing disabled) as far as possible. Hence we
avoid a function call and computation of some arguments to that function.
I don't know if we've actually measured teh performance win from this. If we
have, George would know about it.
-- Keir
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC][PATCH] eliminate extra tb_init_done check
2008-10-15 7:17 ` Keir Fraser
@ 2008-10-15 15:54 ` George Dunlap
0 siblings, 0 replies; 3+ messages in thread
From: George Dunlap @ 2008-10-15 15:54 UTC (permalink / raw)
To: Keir Fraser; +Cc: Lu, Guanqun, xen-devel@lists.xensource.com
Well, I just tested ddk-build running inside a w2k3 box. This normally
touches a lot of shadow code, which has a lot of tracing in it.
For each configuration, I ran the build once to warm up the disk cache,
then three more times for actual testing.
No patch: 156s, 161s, 156s
Patch: 161s, 156s, 156s
So I'm not seeing a big impact from this change.
Personally, I think I'd leave it as-is, because I don't like the idea of
marshalling all that code and then not doing anything, but I think
that's just a taste thing at this point. :-)
-George
Keir Fraser wrote:
> On 15/10/08 07:20, "Lu, Guanqun" <guanqun.lu@intel.com> wrote:
>
>> Two corner conditions are left untouched. One is the assembly in entry.S,
>> the other is the check of tb_init_done not immediately followed by
>> __trace_var.
>>
>> Or more aggressively, we can eliminate all the extra checks, make tb_init_done
>> a static variable, and rename __trace_var to trace_var which looks more like
>> a right interface name.
>
> The macros check tb_init_done before calling __trace_var() to try and reduce
> the cost of the common case (tracing disabled) as far as possible. Hence we
> avoid a function call and computation of some arguments to that function.
>
> I don't know if we've actually measured teh performance win from this. If we
> have, George would know about it.
>
> -- Keir
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-10-15 15:54 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-10-15 6:20 [RFC][PATCH] eliminate extra tb_init_done check Lu, Guanqun
2008-10-15 7:17 ` Keir Fraser
2008-10-15 15:54 ` George Dunlap
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.