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

[Vitaly Kuznetsov]

Steven Rostedt  writes:

> On Fri, 09 Jun 2017 20:53:53 +0200
> Paul Bolle  wrote:
>
>> On Fri, 2017-06-09 at 14:32 -0400, Steven Rostedt wrote:
>> > I'm sure it works, but it just adds one more way of doing the same
>> > thing. I thought that was what perl was always criticized for, and why
>> > people usually prefer python. Don't get me wrong, I prefer oysters over
>> > snakes. But I just wanted to point out the lack of consistency here.  
>> 
>> A major benefit is that
>> #if IS_ENABLED(CONFIG_HYPERV)
>> 
>> is shorter than
>> #if defined(CONFIG_HYPERV) || defined(CONFIG_HYPERV_MODULE)
>> 
>> and less prone to typos.
>> 
>
> I don't believe the module version is needed here. Otherwise I would
> question the #if altogether. Which now I'm looking at it, why is it
> needed?
>
> What includes this header file that wouldn't have that set anyway? The
> only place it is included is in:
>
>  arch/x86/hyperv/mmu.c
>
> Is that compiled without CONFIG_HYPERV?

No, it is not but as was already mentioned it is valid and common to have
CONFIG_HYPERV=m (we should've probably done things differently in the past;
CONFIG_HYPERV=y/n should've been used for indicating Hyper-V support and
something like CONFIG_HYPERV_VMBUS=y/m/n to say if we want to have vmbus
as a module but...).

arch/x86/hyperv/mmu.c is compiled in vmlinux when CONFIG_HYPERV is
enabled in any way, this is updated in PATCH8 of this series:

diff --git a/arch/x86/Kbuild b/arch/x86/Kbuild
index 586b786..3e6f640 100644
--- a/arch/x86/Kbuild
+++ b/arch/x86/Kbuild
@@ -8,7 +8,7 @@ obj-$(CONFIG_KVM) += kvm/
 obj-$(CONFIG_XEN) += xen/

 # Hyper-V paravirtualization support
-obj-$(CONFIG_HYPERVISOR_GUEST) += hyperv/
+obj-$(subst m,y,$(CONFIG_HYPERV)) += hyperv/

 # lguest paravirtualization support
 obj-$(CONFIG_LGUEST_GUEST) += lguest/

(it was Andy who suggested we use 'subst', not me :-)

So we can't just change IS_ENABLED -> ifdef in this patch. We can, of
course, write " #if defined(CONFIG_HYPERV) ||
defined(CONFIG_HYPERV_MODULE)" but we were replacing it with IF_ENABLED
in the past, not sure we should do that. Dropping the #if altogether is
possible, but why having it when CONFIG_HYPERV=n?

-- 
  Vitaly

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

[Vitaly Kuznetsov]

Steven Rostedt  writes:

> On Fri, 09 Jun 2017 20:53:53 +0200
> Paul Bolle  wrote:
>
>> On Fri, 2017-06-09 at 14:32 -0400, Steven Rostedt wrote:
>> > I'm sure it works, but it just adds one more way of doing the same
>> > thing. I thought that was what perl was always criticized for, and why
>> > people usually prefer python. Don't get me wrong, I prefer oysters over
>> > snakes. But I just wanted to point out the lack of consistency here.  
>> 
>> A major benefit is that
>> #if IS_ENABLED(CONFIG_HYPERV)
>> 
>> is shorter than
>> #if defined(CONFIG_HYPERV) || defined(CONFIG_HYPERV_MODULE)
>> 
>> and less prone to typos.
>> 
>
> I don't believe the module version is needed here. Otherwise I would
> question the #if altogether. Which now I'm looking at it, why is it
> needed?
>
> What includes this header file that wouldn't have that set anyway? The
> only place it is included is in:
>
>  arch/x86/hyperv/mmu.c
>
> Is that compiled without CONFIG_HYPERV?

No, it is not but as was already mentioned it is valid and common to have
CONFIG_HYPERV=m (we should've probably done things differently in the past;
CONFIG_HYPERV=y/n should've been used for indicating Hyper-V support and
something like CONFIG_HYPERV_VMBUS=y/m/n to say if we want to have vmbus
as a module but...).

arch/x86/hyperv/mmu.c is compiled in vmlinux when CONFIG_HYPERV is
enabled in any way, this is updated in PATCH8 of this series:

diff --git a/arch/x86/Kbuild b/arch/x86/Kbuild
index 586b786..3e6f640 100644
--- a/arch/x86/Kbuild
+++ b/arch/x86/Kbuild
@@ -8,7 +8,7 @@ obj-$(CONFIG_KVM) += kvm/
 obj-$(CONFIG_XEN) += xen/

 # Hyper-V paravirtualization support
-obj-$(CONFIG_HYPERVISOR_GUEST) += hyperv/
+obj-$(subst m,y,$(CONFIG_HYPERV)) += hyperv/

 # lguest paravirtualization support
 obj-$(CONFIG_LGUEST_GUEST) += lguest/

(it was Andy who suggested we use 'subst', not me :-)

So we can't just change IS_ENABLED -> ifdef in this patch. We can, of
course, write " #if defined(CONFIG_HYPERV) ||
defined(CONFIG_HYPERV_MODULE)" but we were replacing it with IF_ENABLED
in the past, not sure we should do that. Dropping the #if altogether is
possible, but why having it when CONFIG_HYPERV=n?

-- 
  Vitaly

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

[Steven Rostedt]

On Fri, 09 Jun 2017 20:53:53 +0200
Paul Bolle  wrote:

> On Fri, 2017-06-09 at 14:32 -0400, Steven Rostedt wrote:
> > I'm sure it works, but it just adds one more way of doing the same
> > thing. I thought that was what perl was always criticized for, and why
> > people usually prefer python. Don't get me wrong, I prefer oysters over
> > snakes. But I just wanted to point out the lack of consistency here.  
> 
> A major benefit is that
> #if IS_ENABLED(CONFIG_HYPERV)
> 
> is shorter than
> #if defined(CONFIG_HYPERV) || defined(CONFIG_HYPERV_MODULE)
> 
> and less prone to typos.
> 

I don't believe the module version is needed here. Otherwise I would
question the #if altogether. Which now I'm looking at it, why is it
needed?

What includes this header file that wouldn't have that set anyway? The
only place it is included is in:

 arch/x86/hyperv/mmu.c

Is that compiled without CONFIG_HYPERV?

-- Steve

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

[Steven Rostedt]

On Fri, 09 Jun 2017 20:53:53 +0200
Paul Bolle  wrote:

> On Fri, 2017-06-09 at 14:32 -0400, Steven Rostedt wrote:
> > I'm sure it works, but it just adds one more way of doing the same
> > thing. I thought that was what perl was always criticized for, and why
> > people usually prefer python. Don't get me wrong, I prefer oysters over
> > snakes. But I just wanted to point out the lack of consistency here.  
> 
> A major benefit is that
> #if IS_ENABLED(CONFIG_HYPERV)
> 
> is shorter than
> #if defined(CONFIG_HYPERV) || defined(CONFIG_HYPERV_MODULE)
> 
> and less prone to typos.
> 

I don't believe the module version is needed here. Otherwise I would
question the #if altogether. Which now I'm looking at it, why is it
needed?

What includes this header file that wouldn't have that set anyway? The
only place it is included is in:

 arch/x86/hyperv/mmu.c

Is that compiled without CONFIG_HYPERV?

-- Steve

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

[Paul Bolle]

On Fri, 2017-06-09 at 14:32 -0400, Steven Rostedt wrote:
> I'm sure it works, but it just adds one more way of doing the same
> thing. I thought that was what perl was always criticized for, and why
> people usually prefer python. Don't get me wrong, I prefer oysters over
> snakes. But I just wanted to point out the lack of consistency here.

A major benefit is that
#if IS_ENABLED(CONFIG_HYPERV)

is shorter than
#if defined(CONFIG_HYPERV) || defined(CONFIG_HYPERV_MODULE)

and less prone to typos.


Paul Bolle

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

[Paul Bolle]

On Fri, 2017-06-09 at 14:32 -0400, Steven Rostedt wrote:
> I'm sure it works, but it just adds one more way of doing the same
> thing. I thought that was what perl was always criticized for, and why
> people usually prefer python. Don't get me wrong, I prefer oysters over
> snakes. But I just wanted to point out the lack of consistency here.

A major benefit is that
#if IS_ENABLED(CONFIG_HYPERV)

is shorter than
#if defined(CONFIG_HYPERV) || defined(CONFIG_HYPERV_MODULE)

and less prone to typos.


Paul Bolle

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

[Andy Shevchenko]

On Fri, Jun 9, 2017 at 9:32 PM, Steven Rostedt  wrote:
> On Fri, 9 Jun 2017 21:23:52 +0300
> Andy Shevchenko  wrote:

> I'm sure it works, but it just adds one more way of doing the same
> thing. I thought that was what perl was always criticized for, and why
> people usually prefer python. Don't get me wrong, I prefer oysters over
> snakes. But I just wanted to point out the lack of consistency here.

Fair enough.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jun 9, 2017 at 9:32 PM, Steven Rostedt  wrote:
> On Fri, 9 Jun 2017 21:23:52 +0300
> Andy Shevchenko  wrote:

> I'm sure it works, but it just adds one more way of doing the same
> thing. I thought that was what perl was always criticized for, and why
> people usually prefer python. Don't get me wrong, I prefer oysters over
> snakes. But I just wanted to point out the lack of consistency here.

Fair enough.

-- 
With Best Regards,
Andy Shevchenko

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

[Steven Rostedt]

On Fri, 9 Jun 2017 21:23:52 +0300
Andy Shevchenko  wrote:

> On Fri, Jun 9, 2017 at 9:04 PM, Steven Rostedt  wrote:
> > On Fri,  9 Jun 2017 15:27:36 +0200
> > Vitaly Kuznetsov  wrote:  
> 
> 
> >> +#if IS_ENABLED(CONFIG_HYPERV)  
> >
> > Hmm, this is new to me. I know you can use IS_ENABLED() inside C code,
> > but I've never seen it used for preprocessor directives. This usually
> > is just:
> >
> > #ifdef CONFIG_HYPERV
> >
> > Other than that, this looks fine to me.  
> 
> That is the magic of IS_ENABLED(), IS_BUILTIN() macros.
> So, the code above is fine as is.
> 

I'm sure it works, but it just adds one more way of doing the same
thing. I thought that was what perl was always criticized for, and why
people usually prefer python. Don't get me wrong, I prefer oysters over
snakes. But I just wanted to point out the lack of consistency here.

-- Steve

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

[Steven Rostedt]

On Fri, 9 Jun 2017 21:23:52 +0300
Andy Shevchenko  wrote:

> On Fri, Jun 9, 2017 at 9:04 PM, Steven Rostedt  wrote:
> > On Fri,  9 Jun 2017 15:27:36 +0200
> > Vitaly Kuznetsov  wrote:  
> 
> 
> >> +#if IS_ENABLED(CONFIG_HYPERV)  
> >
> > Hmm, this is new to me. I know you can use IS_ENABLED() inside C code,
> > but I've never seen it used for preprocessor directives. This usually
> > is just:
> >
> > #ifdef CONFIG_HYPERV
> >
> > Other than that, this looks fine to me.  
> 
> That is the magic of IS_ENABLED(), IS_BUILTIN() macros.
> So, the code above is fine as is.
> 

I'm sure it works, but it just adds one more way of doing the same
thing. I thought that was what perl was always criticized for, and why
people usually prefer python. Don't get me wrong, I prefer oysters over
snakes. But I just wanted to point out the lack of consistency here.

-- Steve

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

[Andy Shevchenko]

On Fri, Jun 9, 2017 at 9:04 PM, Steven Rostedt  wrote:
> On Fri,  9 Jun 2017 15:27:36 +0200
> Vitaly Kuznetsov  wrote:


>> +#if IS_ENABLED(CONFIG_HYPERV)
>
> Hmm, this is new to me. I know you can use IS_ENABLED() inside C code,
> but I've never seen it used for preprocessor directives. This usually
> is just:
>
> #ifdef CONFIG_HYPERV
>
> Other than that, this looks fine to me.

That is the magic of IS_ENABLED(), IS_BUILTIN() macros.
So, the code above is fine as is.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jun 9, 2017 at 9:04 PM, Steven Rostedt  wrote:
> On Fri,  9 Jun 2017 15:27:36 +0200
> Vitaly Kuznetsov  wrote:


>> +#if IS_ENABLED(CONFIG_HYPERV)
>
> Hmm, this is new to me. I know you can use IS_ENABLED() inside C code,
> but I've never seen it used for preprocessor directives. This usually
> is just:
>
> #ifdef CONFIG_HYPERV
>
> Other than that, this looks fine to me.

That is the magic of IS_ENABLED(), IS_BUILTIN() macros.
So, the code above is fine as is.

-- 
With Best Regards,
Andy Shevchenko

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

[Steven Rostedt]

On Fri,  9 Jun 2017 15:27:36 +0200
Vitaly Kuznetsov  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 
> ---
>  MAINTAINERS |  1 +
>  arch/x86/hyperv/mmu.c   |  7 +++
>  arch/x86/include/asm/trace/hyperv.h | 38 
> +
>  3 files changed, 46 insertions(+)
>  create mode 100644 arch/x86/include/asm/trace/hyperv.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e3f44fd..001614b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6168,6 +6168,7 @@ M:  Stephen Hemminger 
>  L:   de...@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 0712111..cb35453 100644
> --- a/arch/x86/hyperv/mmu.c
> +++ b/arch/x86/hyperv/mmu.c
> @@ -8,6 +8,9 @@
>  #include 
>  #include 
>  
> +#define CREATE_TRACE_POINTS
> +#include 
> +
>  /* HvFlushVirtualAddressSpace, HvFlushVirtualAddressList hypercalls */
>  struct hv_flush_pcpu {
>   u64 address_space;
> @@ -103,6 +106,8 @@ static void hyperv_flush_tlb_others(const struct cpumask 
> *cpus,
>   u64 status = U64_MAX;
>   unsigned long flags;
>  
> + trace_hyperv_mmu_flush_tlb_others(cpus, mm, start, end);
> +
>   if (!pcpu_flush || !hv_hypercall_pg)
>   goto do_native;
>  
> @@ -172,6 +177,8 @@ static void hyperv_flush_tlb_others_ex(const struct 
> cpumask *cpus,
>   u64 status = U64_MAX;
>   unsigned long flags;
>  
> + 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 000..e44ece1
> --- /dev/null
> +++ b/arch/x86/include/asm/trace/hyperv.h
> @@ -0,0 +1,38 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM hyperv
> +
> +#if !defined(_TRACE_HYPERV_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_HYPERV_H
> +
> +#include 
> +
> +#if IS_ENABLED(CONFIG_HYPERV)

Hmm, this is new to me. I know you can use IS_ENABLED() inside C code,
but I've never seen it used for preprocessor directives. This usually
is just:

#ifdef CONFIG_HYPERV

Other than that, this looks fine to me.

-- Steve

> +
> +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 */
> +
> +#undef TRACE_INCLUDE_PATH
> +#define TRACE_INCLUDE_PATH asm/trace/
> +#undef TRACE_INCLUDE_FILE
> +#define TRACE_INCLUDE_FILE hyperv
> +#endif /* _TRACE_HYPERV_H */
> +
> +/* This part must be outside protection */
> +#include 

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

[Steven Rostedt]

On Fri,  9 Jun 2017 15:27:36 +0200
Vitaly Kuznetsov  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 
> ---
>  MAINTAINERS |  1 +
>  arch/x86/hyperv/mmu.c   |  7 +++
>  arch/x86/include/asm/trace/hyperv.h | 38 
> +
>  3 files changed, 46 insertions(+)
>  create mode 100644 arch/x86/include/asm/trace/hyperv.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e3f44fd..001614b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6168,6 +6168,7 @@ M:  Stephen Hemminger 
>  L:   de...@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 0712111..cb35453 100644
> --- a/arch/x86/hyperv/mmu.c
> +++ b/arch/x86/hyperv/mmu.c
> @@ -8,6 +8,9 @@
>  #include 
>  #include 
>  
> +#define CREATE_TRACE_POINTS
> +#include 
> +
>  /* HvFlushVirtualAddressSpace, HvFlushVirtualAddressList hypercalls */
>  struct hv_flush_pcpu {
>   u64 address_space;
> @@ -103,6 +106,8 @@ static void hyperv_flush_tlb_others(const struct cpumask 
> *cpus,
>   u64 status = U64_MAX;
>   unsigned long flags;
>  
> + trace_hyperv_mmu_flush_tlb_others(cpus, mm, start, end);
> +
>   if (!pcpu_flush || !hv_hypercall_pg)
>   goto do_native;
>  
> @@ -172,6 +177,8 @@ static void hyperv_flush_tlb_others_ex(const struct 
> cpumask *cpus,
>   u64 status = U64_MAX;
>   unsigned long flags;
>  
> + 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 000..e44ece1
> --- /dev/null
> +++ b/arch/x86/include/asm/trace/hyperv.h
> @@ -0,0 +1,38 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM hyperv
> +
> +#if !defined(_TRACE_HYPERV_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_HYPERV_H
> +
> +#include 
> +
> +#if IS_ENABLED(CONFIG_HYPERV)

Hmm, this is new to me. I know you can use IS_ENABLED() inside C code,
but I've never seen it used for preprocessor directives. This usually
is just:

#ifdef CONFIG_HYPERV

Other than that, this looks fine to me.

-- Steve

> +
> +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 */
> +
> +#undef TRACE_INCLUDE_PATH
> +#define TRACE_INCLUDE_PATH asm/trace/
> +#undef TRACE_INCLUDE_FILE
> +#define TRACE_INCLUDE_FILE hyperv
> +#endif /* _TRACE_HYPERV_H */
> +
> +/* This part must be outside protection */
> +#include