Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1657683 > unrolled thread

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

Started bySteven Rostedt <rostedt@goodmis.org>
First post2017-06-05 18:00 +0200
Last post2017-06-07 16:10 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 10/10] tracing/hyper-v: trace  hyperv_mmu_flush_tlb_others() Steven Rostedt <rostedt@goodmis.org> - 2017-06-05 18:00 +0200
    Re: [PATCH v4 10/10] tracing/hyper-v: trace hyperv_mmu_flush_tlb_others() Vitaly Kuznetsov <vkuznets@redhat.com> - 2017-06-05 18:20 +0200
      Re: [PATCH v4 10/10] tracing/hyper-v: trace  hyperv_mmu_flush_tlb_others() Steven Rostedt <rostedt@goodmis.org> - 2017-06-07 16:00 +0200
        Re: [PATCH v4 10/10] tracing/hyper-v: trace hyperv_mmu_flush_tlb_others() Vitaly Kuznetsov <vkuznets@redhat.com> - 2017-06-07 16:10 +0200

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

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-06-05 18:00 +0200
SubjectRe: [PATCH v4 10/10] tracing/hyper-v: trace hyperv_mmu_flush_tlb_others()
Message-ID<tP2A9-4My-3@gated-at.bofh.it>
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>

Does your patch even work?

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

[toc] | [next] | [standalone]

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

FromVitaly Kuznetsov <vkuznets@redhat.com>
Date2017-06-05 18:20 +0200
SubjectRe: [PATCH v4 10/10] tracing/hyper-v: trace hyperv_mmu_flush_tlb_others()
Message-ID<tP2Ty-59z-59@gated-at.bofh.it>
In reply to#1657683
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

[toc] | [prev] | [next] | [standalone]

#1659827

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-06-07 16:00 +0200
Message-ID<tPJF7-7mI-15@gated-at.bofh.it>
In reply to#1657716
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>  
>

[toc] | [prev] | [next] | [standalone]

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

FromVitaly Kuznetsov <vkuznets@redhat.com>
Date2017-06-07 16:10 +0200
SubjectRe: [PATCH v4 10/10] tracing/hyper-v: trace hyperv_mmu_flush_tlb_others()
Message-ID<tPJOP-7Fm-29@gated-at.bofh.it>
In reply to#1659827
Steven Rostedt <rostedt@goodmis.org> writes:

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

I hope I fixed it in v7 of my patchset, it would be great if you could
take a look. Thanks!

-- 
  Vitaly

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web