|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 05/15] xen: make it possible to disable tracing in Kconfig.
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@xxxxxxxxxx>
I'm tempted to ack it on that diffstat alone ;p
However, I've got some suggestions to improve it.
> ---
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Doug Goldstein <cardoe@xxxxxxxxxx>
> ---
> 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(-)
>
> 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.
The grammar here is a little unusual. How about extending it a little
to this?
The tracing infrastructure allows Xen to record a log of events in
per-CPU ring buffers, in binary form. These rings can be accessed in
userspace with the 'xentrace' tool, and interpreted using 'xenalyze' or
'xentrace_format'.
> +
>
> 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
You can also enclose svm_trace_vmentry() in svm.c with #ifdefary
>
> 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)
> +{
> +}
These should all be static inlines in a header file. This will avoid
forcing the compiler to insert a bunch of empty functions and have to
call them, and it allows you to modify the x86 Makefile to be
andrewcoop@andrewcoop:/local/xen.git/xen$ git diff
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 93ead6e..affeb22 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -59,7 +59,7 @@ obj-y += srat.o
obj-y += string.o
obj-y += sysctl.o
obj-y += time.o
-obj-y += trace.o
+obj-$(CONFIG_TRACE) += trace.o
obj-y += traps.o
obj-y += usercopy.o
obj-y += x86_emulate.o
If you are feeling super keen, it would be great to move trace.c into
the pv/ directory (as it is entirely PV specific), in line with cleanup
that myself and Wei are slowly doing.
If you are going to do the move, it would be better to split that out
into a separate patch ahead of introducing CONFIG_TRACE, and take the
opportunity to fix the trailing whitespace and style issues in the file.
> +#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 */
Newlines here please.
> +void __init init_trace_bufs(void)
> +{
> + opt_tbuf_size = 0;
Why do you need this? I can't see it referenced anywhere outside of
CONFIG_TRACING.
> +}
> +
> +int tb_control(xen_sysctl_tbuf_op_t *tbc)
> +{
This also should be a static inline which return -EOPNOTSUPP.
Doing so would also allow common/trace.c to become obj-$(CONFIG_TRACE)
as well.
> + 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);
> + }
> }
Is this hunk supposed to live in this patch?
>
> 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)
This needs to be a static inline, otherwise you get different behaviour
WRT evaluating its arguments.
trace_will_trace_event() also looks like it is in need of turning into a
bool function.
~Andrew
> +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__ */
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |