| Message ID | 149633844944.12814.3257610267149025065.stgit@Solace.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 01.06.17 at 19:34, <dario.faggioli@citrix.com> wrote: > --- a/xen/Kconfig.debug > +++ b/xen/Kconfig.debug > @@ -98,6 +98,14 @@ config PERF_ARRAYS > ---help--- > Enables software performance counter array histograms. > > +config TRACING > + bool "Tracing" > + default y default DEBUG (you don't want to suggest turning it on for a release build) > --- a/xen/common/trace.c > +++ b/xen/common/trace.c > @@ -48,6 +48,11 @@ static unsigned int opt_tevt_mask; > integer_param("tbuf_size", opt_tbuf_size); > integer_param("tevt_mask", opt_tevt_mask); > > +#ifdef CONFIG_TRACING > +/* a flag recording whether initialization has been done */ > +/* or more properly, if the tbuf subsystem is enabled right now */ > +int tb_init_done __read_mostly; Switch to bool at once? And in any event correct the comment style please. > --- a/xen/drivers/cpufreq/utility.c > +++ b/xen/drivers/cpufreq/utility.c > @@ -362,11 +362,14 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, > > if (cpu_online(policy->cpu) && cpufreq_driver->target) > { > - unsigned int prev_freq = policy->cur; > + uint32_t d[2] = { policy->cur, 0 }; Notwithstanding Andrew's question about the placement of this hunk the ", 0" seems pointless to me. > @@ -33,6 +37,7 @@ void init_trace_bufs(void); > /* used to retrieve the physical address of the trace buffers */ > int tb_control(struct xen_sysctl_tbuf_op *tbc); > > +#ifdef CONFIG_TRACING > int trace_will_trace_event(u32 event); Please have a blank line between these, two of them around the matching #else, and one ahead of the #endif. Jan
On 07/06/17 16:14, Jan Beulich wrote: >>>> On 01.06.17 at 19:34, <dario.faggioli@citrix.com> wrote: >> --- a/xen/Kconfig.debug >> +++ b/xen/Kconfig.debug >> @@ -98,6 +98,14 @@ config PERF_ARRAYS >> ---help--- >> Enables software performance counter array histograms. >> >> +config TRACING >> + bool "Tracing" >> + default y > > default DEBUG (you don't want to suggest turning it on for a > release build) In the past I've found it pretty useful to have on in release builds of XenServer, to analyze performance problems a customer was having, and sometimes to see where the guest was crashing. -George
On 01/06/17 18:34, Dario Faggioli wrote: > And compile it out of the hypervisor entirely. > > Code and other sections' sizes change as follows. > > Output of `size`: > vanilla patched-Y patched-N > text 1929007 1929007 1902783 > data 337784 337784 337688 > bss 1310464 1310464 1310336 > > Output of `size -A`: > vanilla patched-Y patched-N > .text 1372602 1372602 1348026 > .rodata 312152 312152 310680 > .init.text 244209 244209 244033 > .init.data 224576 224576 224576 > .data 57472 57472 57376 > .bss 1310464 1310464 1310336 > Total 23026516 23027008 22858069 > > No functional change intended. > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> Thanks Dario -- patch looks good overall; I agree with all of Jan and Andy's comments that I haven't responded to. :-) -George
>>> On 08.06.17 at 17:16, <george.dunlap@citrix.com> wrote: > On 07/06/17 16:14, Jan Beulich wrote: >>>>> On 01.06.17 at 19:34, <dario.faggioli@citrix.com> wrote: >>> --- a/xen/Kconfig.debug >>> +++ b/xen/Kconfig.debug >>> @@ -98,6 +98,14 @@ config PERF_ARRAYS >>> ---help--- >>> Enables software performance counter array histograms. >>> >>> +config TRACING >>> + bool "Tracing" >>> + default y >> >> default DEBUG (you don't want to suggest turning it on for a >> release build) > > In the past I've found it pretty useful to have on in release builds of > XenServer, to analyze performance problems a customer was having, and > sometimes to see where the guest was crashing. Well, that's your choice. I'm only asking to default to off in release builds, not to prevent enabling it. Jan
On 08/06/17 16:35, Jan Beulich wrote: >>>> On 08.06.17 at 17:16, <george.dunlap@citrix.com> wrote: >> On 07/06/17 16:14, Jan Beulich wrote: >>>>>> On 01.06.17 at 19:34, <dario.faggioli@citrix.com> wrote: >>>> --- a/xen/Kconfig.debug >>>> +++ b/xen/Kconfig.debug >>>> @@ -98,6 +98,14 @@ config PERF_ARRAYS >>>> ---help--- >>>> Enables software performance counter array histograms. >>>> >>>> +config TRACING >>>> + bool "Tracing" >>>> + default y >>> >>> default DEBUG (you don't want to suggest turning it on for a >>> release build) >> >> In the past I've found it pretty useful to have on in release builds of >> XenServer, to analyze performance problems a customer was having, and >> sometimes to see where the guest was crashing. > > Well, that's your choice. I'm only asking to default to off in release > builds, not to prevent enabling it. Of course; the question is, as you say, what we're "suggesting"; and what people will experience if they just go with the default. If it were up to me I'd leave it on by default, but I don't have a strong opinion. -George
>>> On 08.06.17 at 17:37, <george.dunlap@citrix.com> wrote: > On 08/06/17 16:35, Jan Beulich wrote: >>>>> On 08.06.17 at 17:16, <george.dunlap@citrix.com> wrote: >>> On 07/06/17 16:14, Jan Beulich wrote: >>>>>>> On 01.06.17 at 19:34, <dario.faggioli@citrix.com> wrote: >>>>> --- a/xen/Kconfig.debug >>>>> +++ b/xen/Kconfig.debug >>>>> @@ -98,6 +98,14 @@ config PERF_ARRAYS >>>>> ---help--- >>>>> Enables software performance counter array histograms. >>>>> >>>>> +config TRACING >>>>> + bool "Tracing" >>>>> + default y >>>> >>>> default DEBUG (you don't want to suggest turning it on for a >>>> release build) >>> >>> In the past I've found it pretty useful to have on in release builds of >>> XenServer, to analyze performance problems a customer was having, and >>> sometimes to see where the guest was crashing. >> >> Well, that's your choice. I'm only asking to default to off in release >> builds, not to prevent enabling it. > > Of course; the question is, as you say, what we're "suggesting"; and > what people will experience if they just go with the default. > > If it were up to me I'd leave it on by default, but I don't have a > strong opinion. If you look at that section in Kconfig.debug, nothing defaults to y, and I think this is appropriate for debug options in general. I additionally think that defaults suggested in EXPERT mode should match the values automatically chosen in non-EXPERT mode. Jan
diff --git a/xen/Kconfig.debug b/xen/Kconfig.debug index 689f297..374c1c0 100644 --- a/xen/Kconfig.debug +++ b/xen/Kconfig.debug @@ -98,6 +98,14 @@ config PERF_ARRAYS ---help--- Enables software performance counter array histograms. +config TRACING + bool "Tracing" + default y + ---help--- + Enables collecting traces of events occurring in the hypervisor + in per-CPU ring buffers. The 'xentrace' tool can be used to read + the buffers and dump the content on the disk. + config VERBOSE_DEBUG bool "Verbose debug messages" diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S index a4ab40a..ea4a106 100644 --- a/xen/arch/x86/hvm/svm/entry.S +++ b/xen/arch/x86/hvm/svm/entry.S @@ -61,10 +61,12 @@ __UNLIKELY_END(nsvm_hap) call svm_asid_handle_vmrun +#ifdef CONFIG_TRACING cmpb $0,tb_init_done(%rip) UNLIKELY_START(nz, svm_trace) call svm_trace_vmentry UNLIKELY_END(svm_trace) +#endif mov VCPU_svm_vmcb(%rbx),%rcx mov UREGS_rax(%rsp),%rax diff --git a/xen/arch/x86/trace.c b/xen/arch/x86/trace.c index 4a953c5..411f798 100644 --- a/xen/arch/x86/trace.c +++ b/xen/arch/x86/trace.c @@ -1,3 +1,4 @@ +#ifdef CONFIG_TRACING #include <xen/init.h> #include <xen/kernel.h> #include <xen/lib.h> @@ -157,3 +158,25 @@ void __trace_ptwr_emulation(unsigned long addr, l1_pgentry_t npte) __trace_var(event, 1/*tsc*/, sizeof(d), &d); } } +#else /* !CONFIG_TRACING */ +#include <xen/kernel.h> +#include <xen/trace.h> + +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 /* CONFIG_TRACING */ diff --git a/xen/common/trace.c b/xen/common/trace.c index f29cd4c..2c18462 100644 --- a/xen/common/trace.c +++ b/xen/common/trace.c @@ -48,6 +48,11 @@ static unsigned int opt_tevt_mask; integer_param("tbuf_size", opt_tbuf_size); integer_param("tevt_mask", opt_tevt_mask); +#ifdef CONFIG_TRACING +/* a flag recording whether initialization has been done */ +/* or more properly, if the tbuf subsystem is enabled right now */ +int tb_init_done __read_mostly; + /* Pointers to the meta-data objects for all system trace buffers */ static struct t_info *t_info; static unsigned int t_info_pages; @@ -64,10 +69,6 @@ static u32 t_buf_highwater; static DEFINE_PER_CPU(unsigned long, lost_records); static DEFINE_PER_CPU(unsigned long, lost_records_first_tsc); -/* a flag recording whether initialization has been done */ -/* or more properly, if the tbuf subsystem is enabled right now */ -int tb_init_done __read_mostly; - /* which CPUs tracing is enabled on */ static cpumask_t tb_cpu_mask; @@ -868,6 +869,36 @@ void __trace_hypercall(uint32_t event, unsigned long op, __trace_var(event, 1, sizeof(uint32_t) * (1 + (a - d.args)), &d); } +#else /* !CONFIG_TRACING */ +void __init init_trace_bufs(void) +{ + opt_tbuf_size = 0; +} + +int tb_control(xen_sysctl_tbuf_op_t *tbc) +{ + static DEFINE_SPINLOCK(lock); + int rc = 0; + + spin_lock(&lock); + + switch ( tbc->cmd ) + { + case XEN_SYSCTL_TBUFOP_get_info: + tbc->evt_mask = 0; + tbc->buffer_mfn = 0; + tbc->size = 0; + break; + default: + rc = -ENOSYS; + break; + } + + spin_unlock(&lock); + + return rc; +} +#endif /* CONFIG_TRACING */ /* * Local variables: diff --git a/xen/drivers/cpufreq/utility.c b/xen/drivers/cpufreq/utility.c index 53879fe..b686a9d 100644 --- a/xen/drivers/cpufreq/utility.c +++ b/xen/drivers/cpufreq/utility.c @@ -362,11 +362,14 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, if (cpu_online(policy->cpu) && cpufreq_driver->target) { - unsigned int prev_freq = policy->cur; + uint32_t d[2] = { policy->cur, 0 }; retval = cpufreq_driver->target(policy, target_freq, relation); if ( retval == 0 ) - TRACE_2D(TRC_PM_FREQ_CHANGE, prev_freq/1000, policy->cur/1000); + { + d[1] = policy->cur/1000; + trace_var(TRC_PM_FREQ_CHANGE, 1, sizeof(d), d); + } } return retval; diff --git a/xen/include/xen/trace.h b/xen/include/xen/trace.h index 12966ea..d1b6c70 100644 --- a/xen/include/xen/trace.h +++ b/xen/include/xen/trace.h @@ -21,7 +21,11 @@ #ifndef __XEN_TRACE_H__ #define __XEN_TRACE_H__ +#ifdef CONFIG_TRACING extern int tb_init_done; +#else +#define tb_init_done 0 +#endif #include <public/sysctl.h> #include <public/trace.h> @@ -33,6 +37,7 @@ void init_trace_bufs(void); /* used to retrieve the physical address of the trace buffers */ int tb_control(struct xen_sysctl_tbuf_op *tbc); +#ifdef CONFIG_TRACING int trace_will_trace_event(u32 event); void __trace_var(u32 event, bool_t cycles, unsigned int extra, const void *); @@ -113,7 +118,7 @@ void __trace_hypercall(uint32_t event, unsigned long op, } \ } while ( 0 ) -#define TRACE_6D(_e,d1,d2,d3,d4,d5,d6) \ +#define TRACE_6D(_e,d1,d2,d3,d4,d5,d6) \ do { \ if ( unlikely(tb_init_done) ) \ { \ @@ -127,5 +132,28 @@ void __trace_hypercall(uint32_t event, unsigned long op, __trace_var(_e, 1, sizeof(_d), _d); \ } \ } while ( 0 ) +#else /* !CONFIG_TRACING */ +#define trace_will_trace_event(u) (0) +static inline void __trace_var(u32 event, bool_t cycles, unsigned int extra, + const void *extra_data) +{ +} +static inline void trace_var(u32 event, int cycles, int extra, + const void *extra_data) +{ +} +static inline void __trace_hypercall(uint32_t event, unsigned long op, + const xen_ulong_t *args) +{ +} + +#define TRACE_0D(e) do {} while ( 0 ) +#define TRACE_1D(e,d1) do {} while ( 0 ) +#define TRACE_2D(e,d1,d2) do {} while ( 0 ) +#define TRACE_3D(e,d1,d2,d3) do {} while ( 0 ) +#define TRACE_4D(e,d1,d2,d3,d4) do {} while ( 0 ) +#define TRACE_5D(e,d1,d2,d3,d4,d5) do {} while ( 0 ) +#define TRACE_6D(e,d1,d2,d3,d4,d5,d6) do {} while ( 0 ) +#endif /* CONFIG_TRACING */ #endif /* __XEN_TRACE_H__ */
And compile it out of the hypervisor entirely. Code and other sections' sizes change as follows. Output of `size`: vanilla patched-Y patched-N text 1929007 1929007 1902783 data 337784 337784 337688 bss 1310464 1310464 1310336 Output of `size -A`: vanilla patched-Y patched-N .text 1372602 1372602 1348026 .rodata 312152 312152 310680 .init.text 244209 244209 244033 .init.data 224576 224576 224576 .data 57472 57472 57376 .bss 1310464 1310464 1310336 Total 23026516 23027008 22858069 No functional change intended. Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> --- Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Doug Goldstein <cardoe@cardoe.com> --- xen/Kconfig.debug | 8 ++++++++ xen/arch/x86/hvm/svm/entry.S | 2 ++ xen/arch/x86/trace.c | 23 +++++++++++++++++++++++ xen/common/trace.c | 39 +++++++++++++++++++++++++++++++++++---- xen/drivers/cpufreq/utility.c | 7 +++++-- xen/include/xen/trace.h | 30 +++++++++++++++++++++++++++++- 6 files changed, 102 insertions(+), 7 deletions(-)