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

[Vitaly Kuznetsov <vkuznets@redhat.com>]

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:

"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.


> -- 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>

-- 
  Vitaly