Re: [PATCH v4 10/10] tracing/hyper-v: trace hyperv_mmu_flush_tlb_others()

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 05 Jun 2017 18:19:08 +0200
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Steven Rostedt <rostedt@goodmis.org> writes:
> 
> > On Wed, 24 May 2017 14:04:05 +0200
> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >  
> >> Add Hyper-V tracing subsystem and trace hyperv_mmu_flush_tlb_others().
> >> Tracing is done the same way we do xen_mmu_flush_tlb_others().
> >> 
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> Acked-by: K. Y. Srinivasan <kys@microsoft.com>
> >> Tested-by: Simon Xiao <sixiao@microsoft.com>
> >> Tested-by: Srikanth Myakam <v-srm@microsoft.com>
> >> ---
> >>  MAINTAINERS                         |  1 +
> >>  arch/x86/hyperv/mmu.c               |  8 ++++++++
> >>  arch/x86/include/asm/trace/hyperv.h | 34 ++++++++++++++++++++++++++++++++++
> >>  3 files changed, 43 insertions(+)
> >>  create mode 100644 arch/x86/include/asm/trace/hyperv.h
> >> 
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 9e98464..045e10a 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -6168,6 +6168,7 @@ M:	Stephen Hemminger <sthemmin@microsoft.com>
> >>  L:	devel@linuxdriverproject.org
> >>  S:	Maintained
> >>  F:	arch/x86/include/asm/mshyperv.h
> >> +F:	arch/x86/include/asm/trace/hyperv.h
> >>  F:	arch/x86/include/uapi/asm/hyperv.h
> >>  F:	arch/x86/kernel/cpu/mshyperv.c
> >>  F:	arch/x86/hyperv
> >> diff --git a/arch/x86/hyperv/mmu.c b/arch/x86/hyperv/mmu.c
> >> index c9cecb3..f6b5211 100644
> >> --- a/arch/x86/hyperv/mmu.c
> >> +++ b/arch/x86/hyperv/mmu.c
> >> @@ -6,6 +6,10 @@
> >>  #include <asm/tlbflush.h>
> >>  #include <asm/msr.h>
> >>  #include <asm/fpu/api.h>
> >> +#include <asm/trace/hyperv.h>
> >> +
> >> +#define CREATE_TRACE_POINTS
> >> +DEFINE_TRACE(hyperv_mmu_flush_tlb_others);  
> >
> > The above looks very wrong. Why are you using "DEFINE_TRACE()" here?
> >
> > The typical case is:
> >
> > #define CREATE_TRACE_POINTS
> > #include <asm/trace/hyperv.h>
> >  
> 
> I probably got the idea wrong from Documentation/trace/tracepoints.txt:

Bah! I never was Cc'd on that file. It's totally wrong. Thanks for
pointing that out.

Ah, it was written when we had hard coded tracepoints, that use to do
that. It's very out of date, and still incorrect. 

I'll send a patch to fix it.

> 
> "In subsys/file.c (where the tracing statement must be added) :
> 
> #include <trace/events/subsys.h>
> 
> #define CREATE_TRACE_POINTS
> DEFINE_TRACE(subsys_eventname);
> 
> void somefct(void)
> {
>         ...
>         trace_subsys_eventname(arg, task);
>         ...
> }
> "
> 
> > Does your patch even work?
> >  
> 
> I'm pretty sure I tested tracing this even before sending v2 of this
> series, I'll retest before sending v7.

Even if it does work, it's still fragile as it uses an
no-longer-supported framework.

-- Steve

> >  
> >>  
> >>  /* HvFlushVirtualAddressSpace, HvFlushVirtualAddressList hypercalls */
> >>  struct hv_flush_pcpu {
> >> @@ -75,6 +79,8 @@ static void hyperv_flush_tlb_others(const struct cpumask *cpus,
> >>  	u64 status = -1ULL;
> >>  	int cpu, vcpu, gva_n, max_gvas;
> >>  
> >> +	trace_hyperv_mmu_flush_tlb_others(cpus, mm, start, end);
> >> +
> >>  	if (!pcpu_flush || !hv_hypercall_pg)
> >>  		goto do_native;
> >>  
> >> @@ -161,6 +167,8 @@ static void hyperv_flush_tlb_others_ex(const struct cpumask *cpus,
> >>  	u64 status = -1ULL;
> >>  	int nr_bank = 0, max_gvas, gva_n;
> >>  
> >> +	trace_hyperv_mmu_flush_tlb_others(cpus, mm, start, end);
> >> +
> >>  	if (!pcpu_flush_ex || !hv_hypercall_pg)
> >>  		goto do_native;
> >>  
> >> diff --git a/arch/x86/include/asm/trace/hyperv.h b/arch/x86/include/asm/trace/hyperv.h
> >> new file mode 100644
> >> index 0000000..e46a351
> >> --- /dev/null
> >> +++ b/arch/x86/include/asm/trace/hyperv.h
> >> @@ -0,0 +1,34 @@
> >> +#undef TRACE_SYSTEM
> >> +#define TRACE_SYSTEM hyperv
> >> +
> >> +#if !defined(_TRACE_HYPERV_H) || defined(TRACE_HEADER_MULTI_READ)
> >> +#define _TRACE_HYPERV_H
> >> +
> >> +#include <linux/tracepoint.h>
> >> +
> >> +#if IS_ENABLED(CONFIG_HYPERV)
> >> +
> >> +TRACE_EVENT(hyperv_mmu_flush_tlb_others,
> >> +	    TP_PROTO(const struct cpumask *cpus, struct mm_struct *mm,
> >> +		     unsigned long addr, unsigned long end),
> >> +	    TP_ARGS(cpus, mm, addr, end),
> >> +	    TP_STRUCT__entry(
> >> +		    __field(unsigned int, ncpus)
> >> +		    __field(struct mm_struct *, mm)
> >> +		    __field(unsigned long, addr)
> >> +		    __field(unsigned long, end)
> >> +		    ),
> >> +	    TP_fast_assign(__entry->ncpus = cpumask_weight(cpus);
> >> +			   __entry->mm = mm;
> >> +			   __entry->addr = addr,
> >> +			   __entry->end = end),
> >> +	    TP_printk("ncpus %d mm %p addr %lx, end %lx",
> >> +		      __entry->ncpus, __entry->mm, __entry->addr, __entry->end)
> >> +	);
> >> +
> >> +#endif /* CONFIG_HYPERV */
> >> +
> >> +#endif /* _TRACE_HYPERV_H */
> >> +
> >> +/* This part must be outside protection */
> >> +#include <trace/define_trace.h>  
>