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

Vitaly Kuznetsov
2017-06-05T16:19:17Z

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