* [PATCH] vfio_pci: Add local source directory as include
@ 2019-01-04 19:57 Laura Abbott
2019-01-07 7:26 ` Alexey Kardashevskiy
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Laura Abbott @ 2019-01-04 19:57 UTC (permalink / raw)
To: Alexey Kardashevskiy, Alex Williamson, Michael Ellerman
Cc: Laura Abbott, kvm, Linux Kernel Mailing List
Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
subdriver") introduced a trace.h file in the local directory but
missed adding the local include path, resulting in compilation
failures with tracepoints:
In file included from drivers/vfio/pci/trace.h:102,
from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
#include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
Fix this by adjusting the include path.
Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
I'd still like to echo my sentiment that this should not be a def_bool.
We hit this error on our internal testing and we couldn't even turn
off the driver until we fixed this.
---
drivers/vfio/pci/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
index 9662c063a6b1..08d4676a8495 100644
--- a/drivers/vfio/pci/Makefile
+++ b/drivers/vfio/pci/Makefile
@@ -1,3 +1,4 @@
+ccflags-y += -I$(src)
vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
--
2.20.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-04 19:57 [PATCH] vfio_pci: Add local source directory as include Laura Abbott
@ 2019-01-07 7:26 ` Alexey Kardashevskiy
2019-01-07 8:58 ` Michael Ellerman
2019-01-07 11:12 ` Cornelia Huck
2 siblings, 0 replies; 16+ messages in thread
From: Alexey Kardashevskiy @ 2019-01-07 7:26 UTC (permalink / raw)
To: Laura Abbott, Alex Williamson, Michael Ellerman
Cc: kvm, Linux Kernel Mailing List, David Gibson
On 05/01/2019 06:57, Laura Abbott wrote:
> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> subdriver") introduced a trace.h file in the local directory but
> missed adding the local include path, resulting in compilation
> failures with tracepoints:
>
> In file included from drivers/vfio/pci/trace.h:102,
> from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>
> Fix this by adjusting the include path.
>
> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> Signed-off-by: Laura Abbott <labbott@redhat.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> I'd still like to echo my sentiment that this should not be a def_bool.
> We hit this error on our internal testing and we couldn't even turn
> off the driver until we fixed this.
> ---
> drivers/vfio/pci/Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index 9662c063a6b1..08d4676a8495 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -1,3 +1,4 @@
> +ccflags-y += -I$(src)
>
> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>
--
Alexey
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-04 19:57 [PATCH] vfio_pci: Add local source directory as include Laura Abbott
2019-01-07 7:26 ` Alexey Kardashevskiy
@ 2019-01-07 8:58 ` Michael Ellerman
2019-01-07 10:12 ` Masahiro Yamada
2019-01-07 20:06 ` Laura Abbott
2019-01-07 11:12 ` Cornelia Huck
2 siblings, 2 replies; 16+ messages in thread
From: Michael Ellerman @ 2019-01-07 8:58 UTC (permalink / raw)
To: Laura Abbott, Alexey Kardashevskiy, Alex Williamson
Cc: Laura Abbott, kvm, Linux Kernel Mailing List
Laura Abbott <labbott@redhat.com> writes:
> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> subdriver") introduced a trace.h file in the local directory but
> missed adding the local include path, resulting in compilation
> failures with tracepoints:
>
> In file included from drivers/vfio/pci/trace.h:102,
> from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>
> Fix this by adjusting the include path.
>
> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> I'd still like to echo my sentiment that this should not be a def_bool.
> We hit this error on our internal testing and we couldn't even turn
> off the driver until we fixed this.
I assume there's some reason you can't commit a patch to your tree to
change it to bool, or turn it off entirely? That would change the SHA
which is perhaps reason enough.
In general we have far too many options and most of them never get
turned off (or on), so it just creates testing/bug surface for not much
benefit. This is one that will probably be turned on in all distro
kernels for example.
But I have no real objection to making it user configurable.
Alex I assume you'll merge this fix via the vfio tree?
cheers
> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index 9662c063a6b1..08d4676a8495 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -1,3 +1,4 @@
> +ccflags-y += -I$(src)
>
> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> --
> 2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-07 8:58 ` Michael Ellerman
@ 2019-01-07 10:12 ` Masahiro Yamada
2019-01-07 11:07 ` Cornelia Huck
2019-01-07 20:06 ` Laura Abbott
1 sibling, 1 reply; 16+ messages in thread
From: Masahiro Yamada @ 2019-01-07 10:12 UTC (permalink / raw)
To: Michael Ellerman
Cc: Laura Abbott, Alexey Kardashevskiy, Alex Williamson, kvm,
Linux Kernel Mailing List
On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Laura Abbott <labbott@redhat.com> writes:
> > Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> > subdriver") introduced a trace.h file in the local directory but
> > missed adding the local include path, resulting in compilation
> > failures with tracepoints:
> >
> > In file included from drivers/vfio/pci/trace.h:102,
> > from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> > ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> > #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> >
> > Fix this by adjusting the include path.
> >
> > Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> > Signed-off-by: Laura Abbott <labbott@redhat.com>
> > ---
> > I'd still like to echo my sentiment that this should not be a def_bool.
> > We hit this error on our internal testing and we couldn't even turn
> > off the driver until we fixed this.
>
> I assume there's some reason you can't commit a patch to your tree to
> change it to bool, or turn it off entirely? That would change the SHA
> which is perhaps reason enough.
>
> In general we have far too many options and most of them never get
> turned off (or on), so it just creates testing/bug surface for not much
> benefit. This is one that will probably be turned on in all distro
> kernels for example.
>
> But I have no real objection to making it user configurable.
>
>
> Alex I assume you'll merge this fix via the vfio tree?
>
> cheers
>
> > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > index 9662c063a6b1..08d4676a8495 100644
> > --- a/drivers/vfio/pci/Makefile
> > +++ b/drivers/vfio/pci/Makefile
> > @@ -1,3 +1,4 @@
> > +ccflags-y += -I$(src)
> >
> > vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > --
> > 2.20.1
Hi.
If I correctly understand the usage of TRACE_INCLUDE_PATH,
the correct fix should be like follows:
diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
index 228ccdb..4d13e51 100644
--- a/drivers/vfio/pci/trace.h
+++ b/drivers/vfio/pci/trace.h
@@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
#endif /* _TRACE_VFIO_PCI_H */
#undef TRACE_INCLUDE_PATH
-#define TRACE_INCLUDE_PATH .
+#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
#undef TRACE_INCLUDE_FILE
#define TRACE_INCLUDE_FILE trace
--
Best Regards
Masahiro Yamada
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-07 10:12 ` Masahiro Yamada
@ 2019-01-07 11:07 ` Cornelia Huck
2019-01-07 11:39 ` Masahiro Yamada
0 siblings, 1 reply; 16+ messages in thread
From: Cornelia Huck @ 2019-01-07 11:07 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Michael Ellerman, Laura Abbott, Alexey Kardashevskiy,
Alex Williamson, kvm, Linux Kernel Mailing List
On Mon, 7 Jan 2019 19:12:10 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> >
> > Laura Abbott <labbott@redhat.com> writes:
> > > Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> > > subdriver") introduced a trace.h file in the local directory but
> > > missed adding the local include path, resulting in compilation
> > > failures with tracepoints:
> > >
> > > In file included from drivers/vfio/pci/trace.h:102,
> > > from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> > > ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> > > #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> > >
> > > Fix this by adjusting the include path.
> > >
> > > Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> > > Signed-off-by: Laura Abbott <labbott@redhat.com>
(...)
> > Alex I assume you'll merge this fix via the vfio tree?
> >
> > cheers
> >
> > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > index 9662c063a6b1..08d4676a8495 100644
> > > --- a/drivers/vfio/pci/Makefile
> > > +++ b/drivers/vfio/pci/Makefile
> > > @@ -1,3 +1,4 @@
> > > +ccflags-y += -I$(src)
> > >
> > > vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > --
> > > 2.20.1
>
>
> Hi.
>
> If I correctly understand the usage of TRACE_INCLUDE_PATH,
> the correct fix should be like follows:
>
>
> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
> index 228ccdb..4d13e51 100644
> --- a/drivers/vfio/pci/trace.h
> +++ b/drivers/vfio/pci/trace.h
> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
> #endif /* _TRACE_VFIO_PCI_H */
>
> #undef TRACE_INCLUDE_PATH
> -#define TRACE_INCLUDE_PATH .
> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> #undef TRACE_INCLUDE_FILE
> #define TRACE_INCLUDE_FILE trace
Going from the comments in samples/trace_events/trace-events-sample.h,
I think both approaches are possible, and I see both used in various
places.
Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
a path.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-04 19:57 [PATCH] vfio_pci: Add local source directory as include Laura Abbott
2019-01-07 7:26 ` Alexey Kardashevskiy
2019-01-07 8:58 ` Michael Ellerman
@ 2019-01-07 11:12 ` Cornelia Huck
2 siblings, 0 replies; 16+ messages in thread
From: Cornelia Huck @ 2019-01-07 11:12 UTC (permalink / raw)
To: Laura Abbott
Cc: Alexey Kardashevskiy, Alex Williamson, Michael Ellerman, kvm,
Linux Kernel Mailing List
On Fri, 4 Jan 2019 11:57:14 -0800
Laura Abbott <labbott@redhat.com> wrote:
> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> subdriver") introduced a trace.h file in the local directory but
> missed adding the local include path, resulting in compilation
> failures with tracepoints:
>
> In file included from drivers/vfio/pci/trace.h:102,
> from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>
> Fix this by adjusting the include path.
>
> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> I'd still like to echo my sentiment that this should not be a def_bool.
> We hit this error on our internal testing and we couldn't even turn
> off the driver until we fixed this.
> ---
> drivers/vfio/pci/Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index 9662c063a6b1..08d4676a8495 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -1,3 +1,4 @@
> +ccflags-y += -I$(src)
>
> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-07 11:07 ` Cornelia Huck
@ 2019-01-07 11:39 ` Masahiro Yamada
2019-01-07 20:13 ` Alex Williamson
0 siblings, 1 reply; 16+ messages in thread
From: Masahiro Yamada @ 2019-01-07 11:39 UTC (permalink / raw)
To: Cornelia Huck
Cc: Michael Ellerman, Laura Abbott, Alexey Kardashevskiy,
Alex Williamson, kvm, Linux Kernel Mailing List
On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Mon, 7 Jan 2019 19:12:10 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
> > On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> > >
> > > Laura Abbott <labbott@redhat.com> writes:
> > > > Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> > > > subdriver") introduced a trace.h file in the local directory but
> > > > missed adding the local include path, resulting in compilation
> > > > failures with tracepoints:
> > > >
> > > > In file included from drivers/vfio/pci/trace.h:102,
> > > > from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> > > > ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> > > > #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> > > >
> > > > Fix this by adjusting the include path.
> > > >
> > > > Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> > > > Signed-off-by: Laura Abbott <labbott@redhat.com>
>
> (...)
>
> > > Alex I assume you'll merge this fix via the vfio tree?
> > >
> > > cheers
> > >
> > > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > > index 9662c063a6b1..08d4676a8495 100644
> > > > --- a/drivers/vfio/pci/Makefile
> > > > +++ b/drivers/vfio/pci/Makefile
> > > > @@ -1,3 +1,4 @@
> > > > +ccflags-y += -I$(src)
> > > >
> > > > vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > > > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > > --
> > > > 2.20.1
> >
> >
> > Hi.
> >
> > If I correctly understand the usage of TRACE_INCLUDE_PATH,
> > the correct fix should be like follows:
> >
> >
> > diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
> > index 228ccdb..4d13e51 100644
> > --- a/drivers/vfio/pci/trace.h
> > +++ b/drivers/vfio/pci/trace.h
> > @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
> > #endif /* _TRACE_VFIO_PCI_H */
> >
> > #undef TRACE_INCLUDE_PATH
> > -#define TRACE_INCLUDE_PATH .
> > +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> > #undef TRACE_INCLUDE_FILE
> > #define TRACE_INCLUDE_FILE trace
>
> Going from the comments in samples/trace_events/trace-events-sample.h,
> I think both approaches are possible, and I see both used in various
> places.
>
> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
> a path.
ccflags-y += -I$(src)
would add the header search path for all files in drivers/vfio/pci/
whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
CFLAGS_vfio_pci_nvlink2.o += -I$(src)
is a bit better.
However, it is not obvious why this extra header search path is needed
until you find vfio_pci_nvlink2.c including trace.h
#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
clarifies the intention because the related code is all placed in trace.h
From the comment in include/trace/define_trace.h
TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-07 8:58 ` Michael Ellerman
2019-01-07 10:12 ` Masahiro Yamada
@ 2019-01-07 20:06 ` Laura Abbott
1 sibling, 0 replies; 16+ messages in thread
From: Laura Abbott @ 2019-01-07 20:06 UTC (permalink / raw)
To: Michael Ellerman, Alexey Kardashevskiy, Alex Williamson
Cc: kvm, Linux Kernel Mailing List
On 1/7/19 12:58 AM, Michael Ellerman wrote:
> Laura Abbott <labbott@redhat.com> writes:
>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
>> subdriver") introduced a trace.h file in the local directory but
>> missed adding the local include path, resulting in compilation
>> failures with tracepoints:
>>
>> In file included from drivers/vfio/pci/trace.h:102,
>> from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
>> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>>
>> Fix this by adjusting the include path.
>>
>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> I'd still like to echo my sentiment that this should not be a def_bool.
>> We hit this error on our internal testing and we couldn't even turn
>> off the driver until we fixed this.
>
> I assume there's some reason you can't commit a patch to your tree to
> change it to bool, or turn it off entirely? That would change the SHA
> which is perhaps reason enough.
>
> In general we have far too many options and most of them never get
> turned off (or on), so it just creates testing/bug surface for not much
> benefit. This is one that will probably be turned on in all distro
> kernels for example.
>
> But I have no real objection to making it user configurable.
>
>
> Alex I assume you'll merge this fix via the vfio tree?
>
> cheers
>
Well now that everything is fixed, we will probably keep it on.
My gripe is that when things break like this it's difficult to
work around vs. the configuration option at least makes it possible
to work around in a fast way. This may also just be a strong
preference of mine for working around problems.
Thanks,
Laura
>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
>> index 9662c063a6b1..08d4676a8495 100644
>> --- a/drivers/vfio/pci/Makefile
>> +++ b/drivers/vfio/pci/Makefile
>> @@ -1,3 +1,4 @@
>> +ccflags-y += -I$(src)
>>
>> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>> --
>> 2.20.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-07 11:39 ` Masahiro Yamada
@ 2019-01-07 20:13 ` Alex Williamson
2019-01-07 23:52 ` Alexey Kardashevskiy
2019-01-08 8:02 ` Michael Ellerman
0 siblings, 2 replies; 16+ messages in thread
From: Alex Williamson @ 2019-01-07 20:13 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Cornelia Huck, Michael Ellerman, Laura Abbott,
Alexey Kardashevskiy, kvm, Linux Kernel Mailing List
On Mon, 7 Jan 2019 20:39:19 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
> >
> > On Mon, 7 Jan 2019 19:12:10 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> > > On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> > > >
> > > > Laura Abbott <labbott@redhat.com> writes:
> > > > > Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> > > > > subdriver") introduced a trace.h file in the local directory but
> > > > > missed adding the local include path, resulting in compilation
> > > > > failures with tracepoints:
> > > > >
> > > > > In file included from drivers/vfio/pci/trace.h:102,
> > > > > from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> > > > > ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> > > > > #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> > > > >
> > > > > Fix this by adjusting the include path.
> > > > >
> > > > > Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> > > > > Signed-off-by: Laura Abbott <labbott@redhat.com>
> >
> > (...)
> >
> > > > Alex I assume you'll merge this fix via the vfio tree?
> > > >
> > > > cheers
> > > >
> > > > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > > > index 9662c063a6b1..08d4676a8495 100644
> > > > > --- a/drivers/vfio/pci/Makefile
> > > > > +++ b/drivers/vfio/pci/Makefile
> > > > > @@ -1,3 +1,4 @@
> > > > > +ccflags-y += -I$(src)
> > > > >
> > > > > vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > > > > vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > > > --
> > > > > 2.20.1
> > >
> > >
> > > Hi.
> > >
> > > If I correctly understand the usage of TRACE_INCLUDE_PATH,
> > > the correct fix should be like follows:
> > >
> > >
> > > diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
> > > index 228ccdb..4d13e51 100644
> > > --- a/drivers/vfio/pci/trace.h
> > > +++ b/drivers/vfio/pci/trace.h
> > > @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
> > > #endif /* _TRACE_VFIO_PCI_H */
> > >
> > > #undef TRACE_INCLUDE_PATH
> > > -#define TRACE_INCLUDE_PATH .
> > > +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> > > #undef TRACE_INCLUDE_FILE
> > > #define TRACE_INCLUDE_FILE trace
> >
> > Going from the comments in samples/trace_events/trace-events-sample.h,
> > I think both approaches are possible, and I see both used in various
> > places.
> >
> > Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
> > a path.
Numbering options for clarity:
1)
> ccflags-y += -I$(src)
> would add the header search path for all files in drivers/vfio/pci/
> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
>
2)
> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
> is a bit better.
> However, it is not obvious why this extra header search path is needed
> until you find vfio_pci_nvlink2.c including trace.h
>
3)
> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> clarifies the intention because the related code is all placed in trace.h
>
>
>
> From the comment in include/trace/define_trace.h
> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
In my scan of the tree, the most common solution seems to be 2) as this
is essentially recommended in the sample file. 3) is well represented,
with much fewer examples of 1), though it might depend how liberally
we grep out or examine the use cases. Choice 1) also seems to be the
most shotgun approach, adding to the search path for all files. From a
maintenance perspective I agree that 2) seems more error prone,
especially when the build system only catches the error on in-tree
builds, something I rarely do. Therefore I'm leaning towards option
3). The hardcoded path here doesn't seem much of an issue relative to
the negatives of the other approaches (how often do we move these
files?) and it keeps the trace support relatively self-contained. Are
there further arguments for or against these options? Otherwise who
wants to formally post the TRACE_INCLUDE_PATH version? Thanks,
Alex
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-07 20:13 ` Alex Williamson
@ 2019-01-07 23:52 ` Alexey Kardashevskiy
2019-01-08 0:24 ` Alex Williamson
2019-01-08 8:02 ` Michael Ellerman
1 sibling, 1 reply; 16+ messages in thread
From: Alexey Kardashevskiy @ 2019-01-07 23:52 UTC (permalink / raw)
To: Alex Williamson, Masahiro Yamada
Cc: Cornelia Huck, Michael Ellerman, Laura Abbott, kvm,
Linux Kernel Mailing List
On 08/01/2019 07:13, Alex Williamson wrote:
> On Mon, 7 Jan 2019 20:39:19 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
>>>
>>> On Mon, 7 Jan 2019 19:12:10 +0900
>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>>
>>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>>>>
>>>>> Laura Abbott <labbott@redhat.com> writes:
>>>>>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
>>>>>> subdriver") introduced a trace.h file in the local directory but
>>>>>> missed adding the local include path, resulting in compilation
>>>>>> failures with tracepoints:
>>>>>>
>>>>>> In file included from drivers/vfio/pci/trace.h:102,
>>>>>> from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
>>>>>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
>>>>>> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>>>>>>
>>>>>> Fix this by adjusting the include path.
>>>>>>
>>>>>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
>>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>>
>>> (...)
>>>
>>>>> Alex I assume you'll merge this fix via the vfio tree?
>>>>>
>>>>> cheers
>>>>>
>>>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
>>>>>> index 9662c063a6b1..08d4676a8495 100644
>>>>>> --- a/drivers/vfio/pci/Makefile
>>>>>> +++ b/drivers/vfio/pci/Makefile
>>>>>> @@ -1,3 +1,4 @@
>>>>>> +ccflags-y += -I$(src)
>>>>>>
>>>>>> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>>>>>> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>>>>>> --
>>>>>> 2.20.1
>>>>
>>>>
>>>> Hi.
>>>>
>>>> If I correctly understand the usage of TRACE_INCLUDE_PATH,
>>>> the correct fix should be like follows:
>>>>
>>>>
>>>> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
>>>> index 228ccdb..4d13e51 100644
>>>> --- a/drivers/vfio/pci/trace.h
>>>> +++ b/drivers/vfio/pci/trace.h
>>>> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
>>>> #endif /* _TRACE_VFIO_PCI_H */
>>>>
>>>> #undef TRACE_INCLUDE_PATH
>>>> -#define TRACE_INCLUDE_PATH .
>>>> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>>>> #undef TRACE_INCLUDE_FILE
>>>> #define TRACE_INCLUDE_FILE trace
>>>
>>> Going from the comments in samples/trace_events/trace-events-sample.h,
>>> I think both approaches are possible, and I see both used in various
>>> places.
>>>
>>> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
>>> a path.
>
> Numbering options for clarity:
>
> 1)
>> ccflags-y += -I$(src)
>> would add the header search path for all files in drivers/vfio/pci/
>> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
>>
>
> 2)
>> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
>> is a bit better.
>> However, it is not obvious why this extra header search path is needed
>> until you find vfio_pci_nvlink2.c including trace.h
>>
>
> 3)
>> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>> clarifies the intention because the related code is all placed in trace.h
>>
>>
>>
>> From the comment in include/trace/define_trace.h
>> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
>
> In my scan of the tree, the most common solution seems to be 2) as this
> is essentially recommended in the sample file. 3) is well represented,
> with much fewer examples of 1), though it might depend how liberally
> we grep out or examine the use cases. Choice 1) also seems to be the
> most shotgun approach, adding to the search path for all files.
The shotgun approach is always used when compiling out of tree which is
what many people do anyway without realizing that there are additional
-I. Why do not we make in-tree behave the same way? Thanks,
> From a
> maintenance perspective I agree that 2) seems more error prone,
> especially when the build system only catches the error on in-tree
> builds, something I rarely do. Therefore I'm leaning towards option
> 3). The hardcoded path here doesn't seem much of an issue relative to
> the negatives of the other approaches (how often do we move these
> files?) and it keeps the trace support relatively self-contained. Are
> there further arguments for or against these options? Otherwise who
> wants to formally post the TRACE_INCLUDE_PATH version? Thanks,
--
Alexey
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-07 23:52 ` Alexey Kardashevskiy
@ 2019-01-08 0:24 ` Alex Williamson
2019-01-08 2:20 ` Alexey Kardashevskiy
0 siblings, 1 reply; 16+ messages in thread
From: Alex Williamson @ 2019-01-08 0:24 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Masahiro Yamada, Cornelia Huck, Michael Ellerman, Laura Abbott,
kvm, Linux Kernel Mailing List
On Tue, 8 Jan 2019 10:52:43 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> On 08/01/2019 07:13, Alex Williamson wrote:
> > On Mon, 7 Jan 2019 20:39:19 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
> >>>
> >>> On Mon, 7 Jan 2019 19:12:10 +0900
> >>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >>>
> >>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> >>>>>
> >>>>> Laura Abbott <labbott@redhat.com> writes:
> >>>>>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> >>>>>> subdriver") introduced a trace.h file in the local directory but
> >>>>>> missed adding the local include path, resulting in compilation
> >>>>>> failures with tracepoints:
> >>>>>>
> >>>>>> In file included from drivers/vfio/pci/trace.h:102,
> >>>>>> from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> >>>>>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> >>>>>> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> >>>>>>
> >>>>>> Fix this by adjusting the include path.
> >>>>>>
> >>>>>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> >>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
> >>>
> >>> (...)
> >>>
> >>>>> Alex I assume you'll merge this fix via the vfio tree?
> >>>>>
> >>>>> cheers
> >>>>>
> >>>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> >>>>>> index 9662c063a6b1..08d4676a8495 100644
> >>>>>> --- a/drivers/vfio/pci/Makefile
> >>>>>> +++ b/drivers/vfio/pci/Makefile
> >>>>>> @@ -1,3 +1,4 @@
> >>>>>> +ccflags-y += -I$(src)
> >>>>>>
> >>>>>> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> >>>>>> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> >>>>>> --
> >>>>>> 2.20.1
> >>>>
> >>>>
> >>>> Hi.
> >>>>
> >>>> If I correctly understand the usage of TRACE_INCLUDE_PATH,
> >>>> the correct fix should be like follows:
> >>>>
> >>>>
> >>>> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
> >>>> index 228ccdb..4d13e51 100644
> >>>> --- a/drivers/vfio/pci/trace.h
> >>>> +++ b/drivers/vfio/pci/trace.h
> >>>> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
> >>>> #endif /* _TRACE_VFIO_PCI_H */
> >>>>
> >>>> #undef TRACE_INCLUDE_PATH
> >>>> -#define TRACE_INCLUDE_PATH .
> >>>> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> >>>> #undef TRACE_INCLUDE_FILE
> >>>> #define TRACE_INCLUDE_FILE trace
> >>>
> >>> Going from the comments in samples/trace_events/trace-events-sample.h,
> >>> I think both approaches are possible, and I see both used in various
> >>> places.
> >>>
> >>> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
> >>> a path.
> >
> > Numbering options for clarity:
> >
> > 1)
> >> ccflags-y += -I$(src)
> >> would add the header search path for all files in drivers/vfio/pci/
> >> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
> >>
> >
> > 2)
> >> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
> >> is a bit better.
> >> However, it is not obvious why this extra header search path is needed
> >> until you find vfio_pci_nvlink2.c including trace.h
> >>
> >
> > 3)
> >> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> >> clarifies the intention because the related code is all placed in trace.h
> >>
> >>
> >>
> >> From the comment in include/trace/define_trace.h
> >> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
> >
> > In my scan of the tree, the most common solution seems to be 2) as this
> > is essentially recommended in the sample file. 3) is well represented,
> > with much fewer examples of 1), though it might depend how liberally
> > we grep out or examine the use cases. Choice 1) also seems to be the
> > most shotgun approach, adding to the search path for all files.
>
>
> The shotgun approach is always used when compiling out of tree which is
> what many people do anyway without realizing that there are additional
> -I. Why do not we make in-tree behave the same way? Thanks,
Are you suggesting bandaging this individual leaf directory, as in
choice 1), because it's no worse than what happens for an out-of-tree
build anyway, or are you suggesting to fix the in-tree build behavior to
be as inefficient as the out-of-tree behavior in general? It appears
to me that options 2) and 3) are the intended solutions for this issue
while 1) is more of a workaround. Thanks,
Alex
> > From a
> > maintenance perspective I agree that 2) seems more error prone,
> > especially when the build system only catches the error on in-tree
> > builds, something I rarely do. Therefore I'm leaning towards option
> > 3). The hardcoded path here doesn't seem much of an issue relative to
> > the negatives of the other approaches (how often do we move these
> > files?) and it keeps the trace support relatively self-contained. Are
> > there further arguments for or against these options? Otherwise who
> > wants to formally post the TRACE_INCLUDE_PATH version? Thanks,
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-08 0:24 ` Alex Williamson
@ 2019-01-08 2:20 ` Alexey Kardashevskiy
2019-01-08 2:38 ` Masahiro Yamada
0 siblings, 1 reply; 16+ messages in thread
From: Alexey Kardashevskiy @ 2019-01-08 2:20 UTC (permalink / raw)
To: Alex Williamson
Cc: Masahiro Yamada, Cornelia Huck, Michael Ellerman, Laura Abbott,
kvm, Linux Kernel Mailing List
On 08/01/2019 11:24, Alex Williamson wrote:
> On Tue, 8 Jan 2019 10:52:43 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
>> On 08/01/2019 07:13, Alex Williamson wrote:
>>> On Mon, 7 Jan 2019 20:39:19 +0900
>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>>
>>>> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
>>>>>
>>>>> On Mon, 7 Jan 2019 19:12:10 +0900
>>>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>>>>
>>>>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>>>>>>
>>>>>>> Laura Abbott <labbott@redhat.com> writes:
>>>>>>>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
>>>>>>>> subdriver") introduced a trace.h file in the local directory but
>>>>>>>> missed adding the local include path, resulting in compilation
>>>>>>>> failures with tracepoints:
>>>>>>>>
>>>>>>>> In file included from drivers/vfio/pci/trace.h:102,
>>>>>>>> from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
>>>>>>>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
>>>>>>>> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>>>>>>>>
>>>>>>>> Fix this by adjusting the include path.
>>>>>>>>
>>>>>>>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
>>>>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>>>>
>>>>> (...)
>>>>>
>>>>>>> Alex I assume you'll merge this fix via the vfio tree?
>>>>>>>
>>>>>>> cheers
>>>>>>>
>>>>>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
>>>>>>>> index 9662c063a6b1..08d4676a8495 100644
>>>>>>>> --- a/drivers/vfio/pci/Makefile
>>>>>>>> +++ b/drivers/vfio/pci/Makefile
>>>>>>>> @@ -1,3 +1,4 @@
>>>>>>>> +ccflags-y += -I$(src)
>>>>>>>>
>>>>>>>> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>>>>>>>> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>>>>>>>> --
>>>>>>>> 2.20.1
>>>>>>
>>>>>>
>>>>>> Hi.
>>>>>>
>>>>>> If I correctly understand the usage of TRACE_INCLUDE_PATH,
>>>>>> the correct fix should be like follows:
>>>>>>
>>>>>>
>>>>>> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
>>>>>> index 228ccdb..4d13e51 100644
>>>>>> --- a/drivers/vfio/pci/trace.h
>>>>>> +++ b/drivers/vfio/pci/trace.h
>>>>>> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
>>>>>> #endif /* _TRACE_VFIO_PCI_H */
>>>>>>
>>>>>> #undef TRACE_INCLUDE_PATH
>>>>>> -#define TRACE_INCLUDE_PATH .
>>>>>> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>>>>>> #undef TRACE_INCLUDE_FILE
>>>>>> #define TRACE_INCLUDE_FILE trace
>>>>>
>>>>> Going from the comments in samples/trace_events/trace-events-sample.h,
>>>>> I think both approaches are possible, and I see both used in various
>>>>> places.
>>>>>
>>>>> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
>>>>> a path.
>>>
>>> Numbering options for clarity:
>>>
>>> 1)
>>>> ccflags-y += -I$(src)
>>>> would add the header search path for all files in drivers/vfio/pci/
>>>> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
>>>>
>>>
>>> 2)
>>>> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
>>>> is a bit better.
>>>> However, it is not obvious why this extra header search path is needed
>>>> until you find vfio_pci_nvlink2.c including trace.h
>>>>
>>>
>>> 3)
>>>> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>>>> clarifies the intention because the related code is all placed in trace.h
>>>>
>>>>
>>>>
>>>> From the comment in include/trace/define_trace.h
>>>> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
>>>
>>> In my scan of the tree, the most common solution seems to be 2) as this
>>> is essentially recommended in the sample file. 3) is well represented,
>>> with much fewer examples of 1), though it might depend how liberally
>>> we grep out or examine the use cases. Choice 1) also seems to be the
>>> most shotgun approach, adding to the search path for all files.
>>
>>
>> The shotgun approach is always used when compiling out of tree which is
>> what many people do anyway without realizing that there are additional
>> -I. Why do not we make in-tree behave the same way? Thanks,
>
> Are you suggesting bandaging this individual leaf directory, as in
> choice 1), because it's no worse than what happens for an out-of-tree
> build anyway,
I suggest making in-tree and out-of-tree behavior equal - both either
fail or compile. Just makes sense to me.
Since out-of-tree adds extra -I, then there should have been a reason
for that at the time (before 2005 though). Unfortunately git blame does
not say why it was done this way for out-of-tree so imho the safest
thing is to add -I for in-tree as well. Or ditch extra -I and do 2) or
3) - this is fine too and can count as an impressive compile
optimization, although... look below :)
> or are you suggesting to fix the in-tree build behavior to
> be as inefficient as the out-of-tree behavior in general?
Inefficient you say. Hm.
I tried this:
--- scripts/Makefile.lib.old 2019-01-08 11:39:51.830983393 +1100
+++ scripts/Makefile.lib 2019-01-08 13:09:54.199054981 +1100
@@ -140,11 +140,6 @@
# If building the kernel in a separate objtree expand all occurrences
# of -Idir to -I$(srctree)/dir except for absolute paths (starting with
'/').
-ifeq ($(KBUILD_SRC),)
-__c_flags = $(_c_flags)
-__a_flags = $(_a_flags)
-__cpp_flags = $(_cpp_flags)
-else
# -I$(obj) locates generated .h files
# $(call addtree,-I$(obj)) locates .h files in srctree, from generated
.c files
@@ -154,7 +149,6 @@
$(call flags,_c_flags)
__a_flags = $(call flags,_a_flags)
__cpp_flags = $(call flags,_cpp_flags)
-endif
Compiled 3 times with the patch and without, "make clean ; time make
-j200 vmlinux modules".
No patch:
real 4m33.047s user 481m48.322s sys 10m15.639s
real 4m29.038s user 480m22.873s sys 10m11.394s
real 4m34.373s user 483m7.570s sys 10m10.559s
With the patch:
real 4m32.008s user 479m54.207s sys 10m13.075s
real 4m30.027s user 479m46.272s sys 10m15.886s
real 4m31.548s user 480m2.897s sys 10m10.024
which is slightly faster but I guess within accuracy.
> It appears
> to me that options 2) and 3) are the intended solutions for this issue
> while 1) is more of a workaround. Thanks,
>
> Alex
>
>>> From a
>>> maintenance perspective I agree that 2) seems more error prone,
>>> especially when the build system only catches the error on in-tree
>>> builds, something I rarely do. Therefore I'm leaning towards option
>>> 3). The hardcoded path here doesn't seem much of an issue relative to
>>> the negatives of the other approaches (how often do we move these
>>> files?) and it keeps the trace support relatively self-contained. Are
>>> there further arguments for or against these options? Otherwise who
>>> wants to formally post the TRACE_INCLUDE_PATH version? Thanks,
--
Alexey
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-08 2:20 ` Alexey Kardashevskiy
@ 2019-01-08 2:38 ` Masahiro Yamada
2019-01-08 2:57 ` Alexey Kardashevskiy
0 siblings, 1 reply; 16+ messages in thread
From: Masahiro Yamada @ 2019-01-08 2:38 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Alex Williamson, Cornelia Huck, Michael Ellerman, Laura Abbott,
kvm, Linux Kernel Mailing List
On Tue, Jan 8, 2019 at 11:22 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
>
>
> On 08/01/2019 11:24, Alex Williamson wrote:
> > On Tue, 8 Jan 2019 10:52:43 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >
> >> On 08/01/2019 07:13, Alex Williamson wrote:
> >>> On Mon, 7 Jan 2019 20:39:19 +0900
> >>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >>>
> >>>> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
> >>>>>
> >>>>> On Mon, 7 Jan 2019 19:12:10 +0900
> >>>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >>>>>
> >>>>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> >>>>>>>
> >>>>>>> Laura Abbott <labbott@redhat.com> writes:
> >>>>>>>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> >>>>>>>> subdriver") introduced a trace.h file in the local directory but
> >>>>>>>> missed adding the local include path, resulting in compilation
> >>>>>>>> failures with tracepoints:
> >>>>>>>>
> >>>>>>>> In file included from drivers/vfio/pci/trace.h:102,
> >>>>>>>> from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> >>>>>>>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> >>>>>>>> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> >>>>>>>>
> >>>>>>>> Fix this by adjusting the include path.
> >>>>>>>>
> >>>>>>>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> >>>>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
> >>>>>
> >>>>> (...)
> >>>>>
> >>>>>>> Alex I assume you'll merge this fix via the vfio tree?
> >>>>>>>
> >>>>>>> cheers
> >>>>>>>
> >>>>>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> >>>>>>>> index 9662c063a6b1..08d4676a8495 100644
> >>>>>>>> --- a/drivers/vfio/pci/Makefile
> >>>>>>>> +++ b/drivers/vfio/pci/Makefile
> >>>>>>>> @@ -1,3 +1,4 @@
> >>>>>>>> +ccflags-y += -I$(src)
> >>>>>>>>
> >>>>>>>> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> >>>>>>>> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> >>>>>>>> --
> >>>>>>>> 2.20.1
> >>>>>>
> >>>>>>
> >>>>>> Hi.
> >>>>>>
> >>>>>> If I correctly understand the usage of TRACE_INCLUDE_PATH,
> >>>>>> the correct fix should be like follows:
> >>>>>>
> >>>>>>
> >>>>>> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
> >>>>>> index 228ccdb..4d13e51 100644
> >>>>>> --- a/drivers/vfio/pci/trace.h
> >>>>>> +++ b/drivers/vfio/pci/trace.h
> >>>>>> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
> >>>>>> #endif /* _TRACE_VFIO_PCI_H */
> >>>>>>
> >>>>>> #undef TRACE_INCLUDE_PATH
> >>>>>> -#define TRACE_INCLUDE_PATH .
> >>>>>> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> >>>>>> #undef TRACE_INCLUDE_FILE
> >>>>>> #define TRACE_INCLUDE_FILE trace
> >>>>>
> >>>>> Going from the comments in samples/trace_events/trace-events-sample.h,
> >>>>> I think both approaches are possible, and I see both used in various
> >>>>> places.
> >>>>>
> >>>>> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
> >>>>> a path.
> >>>
> >>> Numbering options for clarity:
> >>>
> >>> 1)
> >>>> ccflags-y += -I$(src)
> >>>> would add the header search path for all files in drivers/vfio/pci/
> >>>> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
> >>>>
> >>>
> >>> 2)
> >>>> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
> >>>> is a bit better.
> >>>> However, it is not obvious why this extra header search path is needed
> >>>> until you find vfio_pci_nvlink2.c including trace.h
> >>>>
> >>>
> >>> 3)
> >>>> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> >>>> clarifies the intention because the related code is all placed in trace.h
> >>>>
> >>>>
> >>>>
> >>>> From the comment in include/trace/define_trace.h
> >>>> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
> >>>
> >>> In my scan of the tree, the most common solution seems to be 2) as this
> >>> is essentially recommended in the sample file. 3) is well represented,
> >>> with much fewer examples of 1), though it might depend how liberally
> >>> we grep out or examine the use cases. Choice 1) also seems to be the
> >>> most shotgun approach, adding to the search path for all files.
> >>
> >>
> >> The shotgun approach is always used when compiling out of tree which is
> >> what many people do anyway without realizing that there are additional
> >> -I. Why do not we make in-tree behave the same way? Thanks,
> >
> > Are you suggesting bandaging this individual leaf directory, as in
> > choice 1), because it's no worse than what happens for an out-of-tree
> > build anyway,
>
>
> I suggest making in-tree and out-of-tree behavior equal - both either
> fail or compile. Just makes sense to me.
>
> Since out-of-tree adds extra -I, then there should have been a reason
> for that at the time (before 2005 though). Unfortunately git blame does
> not say why it was done this way for out-of-tree so imho the safest
> thing is to add -I for in-tree as well. Or ditch extra -I and do 2) or
> 3) - this is fine too and can count as an impressive compile
> optimization, although... look below :)
For the out-of-tree building,
scripts/Makefile.lib adds -I$(srctree)/$(src) and -I$(obj).
In my understanding, -I$(srctree)/$(src) is for
including check-in headers from generated C files.
-I$(obj) is for including generated headers from check-in C files.
One example of the correct usage of this is,
crypto/rsa_helper.c (check-in file)
includes
crypto/rsapubkey.asn1.h (generated fle)
Those extra header search paths are unneeded for in-tree building
since generated files and check-in files exist in the same tree.
You just happened to find it works for out-of-tree building.
We are talking about how to make include/trace/define_trace.h
include drivers/vfio/pci/trace.h
The build system cannot (should not)
cater to such a particular case.
>
> > or are you suggesting to fix the in-tree build behavior to
> > be as inefficient as the out-of-tree behavior in general?
>
> Inefficient you say. Hm.
>
> I tried this:
>
> --- scripts/Makefile.lib.old 2019-01-08 11:39:51.830983393 +1100
> +++ scripts/Makefile.lib 2019-01-08 13:09:54.199054981 +1100
> @@ -140,11 +140,6 @@
> # If building the kernel in a separate objtree expand all occurrences
> # of -Idir to -I$(srctree)/dir except for absolute paths (starting with
> '/').
>
> -ifeq ($(KBUILD_SRC),)
> -__c_flags = $(_c_flags)
> -__a_flags = $(_a_flags)
> -__cpp_flags = $(_cpp_flags)
> -else
>
> # -I$(obj) locates generated .h files
> # $(call addtree,-I$(obj)) locates .h files in srctree, from generated
> .c files
> @@ -154,7 +149,6 @@
> $(call flags,_c_flags)
> __a_flags = $(call flags,_a_flags)
> __cpp_flags = $(call flags,_cpp_flags)
> -endif
>
>
> Compiled 3 times with the patch and without, "make clean ; time make
> -j200 vmlinux modules".
>
> No patch:
>
> real 4m33.047s user 481m48.322s sys 10m15.639s
> real 4m29.038s user 480m22.873s sys 10m11.394s
> real 4m34.373s user 483m7.570s sys 10m10.559s
>
> With the patch:
>
> real 4m32.008s user 479m54.207s sys 10m13.075s
> real 4m30.027s user 479m46.272s sys 10m15.886s
> real 4m31.548s user 480m2.897s sys 10m10.024
>
>
> which is slightly faster but I guess within accuracy.
>
>
>
> > It appears
> > to me that options 2) and 3) are the intended solutions for this issue
> > while 1) is more of a workaround. Thanks,
> >
> > Alex
> >
> >>> From a
> >>> maintenance perspective I agree that 2) seems more error prone,
> >>> especially when the build system only catches the error on in-tree
> >>> builds, something I rarely do. Therefore I'm leaning towards option
> >>> 3). The hardcoded path here doesn't seem much of an issue relative to
> >>> the negatives of the other approaches (how often do we move these
> >>> files?) and it keeps the trace support relatively self-contained. Are
> >>> there further arguments for or against these options? Otherwise who
> >>> wants to formally post the TRACE_INCLUDE_PATH version? Thanks,
>
>
>
>
> --
> Alexey
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-08 2:38 ` Masahiro Yamada
@ 2019-01-08 2:57 ` Alexey Kardashevskiy
2019-01-08 3:19 ` Masahiro Yamada
0 siblings, 1 reply; 16+ messages in thread
From: Alexey Kardashevskiy @ 2019-01-08 2:57 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Alex Williamson, Cornelia Huck, Michael Ellerman, Laura Abbott,
kvm, Linux Kernel Mailing List
On 08/01/2019 13:38, Masahiro Yamada wrote:
> On Tue, Jan 8, 2019 at 11:22 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>>
>>
>> On 08/01/2019 11:24, Alex Williamson wrote:
>>> On Tue, 8 Jan 2019 10:52:43 +1100
>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>
>>>> On 08/01/2019 07:13, Alex Williamson wrote:
>>>>> On Mon, 7 Jan 2019 20:39:19 +0900
>>>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>>>>
>>>>>> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
>>>>>>>
>>>>>>> On Mon, 7 Jan 2019 19:12:10 +0900
>>>>>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>>>>>>
>>>>>>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>>>>>>>>
>>>>>>>>> Laura Abbott <labbott@redhat.com> writes:
>>>>>>>>>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
>>>>>>>>>> subdriver") introduced a trace.h file in the local directory but
>>>>>>>>>> missed adding the local include path, resulting in compilation
>>>>>>>>>> failures with tracepoints:
>>>>>>>>>>
>>>>>>>>>> In file included from drivers/vfio/pci/trace.h:102,
>>>>>>>>>> from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
>>>>>>>>>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
>>>>>>>>>> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>>>>>>>>>>
>>>>>>>>>> Fix this by adjusting the include path.
>>>>>>>>>>
>>>>>>>>>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
>>>>>>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>>>>>>
>>>>>>> (...)
>>>>>>>
>>>>>>>>> Alex I assume you'll merge this fix via the vfio tree?
>>>>>>>>>
>>>>>>>>> cheers
>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
>>>>>>>>>> index 9662c063a6b1..08d4676a8495 100644
>>>>>>>>>> --- a/drivers/vfio/pci/Makefile
>>>>>>>>>> +++ b/drivers/vfio/pci/Makefile
>>>>>>>>>> @@ -1,3 +1,4 @@
>>>>>>>>>> +ccflags-y += -I$(src)
>>>>>>>>>>
>>>>>>>>>> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>>>>>>>>>> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>>>>>>>>>> --
>>>>>>>>>> 2.20.1
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi.
>>>>>>>>
>>>>>>>> If I correctly understand the usage of TRACE_INCLUDE_PATH,
>>>>>>>> the correct fix should be like follows:
>>>>>>>>
>>>>>>>>
>>>>>>>> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
>>>>>>>> index 228ccdb..4d13e51 100644
>>>>>>>> --- a/drivers/vfio/pci/trace.h
>>>>>>>> +++ b/drivers/vfio/pci/trace.h
>>>>>>>> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
>>>>>>>> #endif /* _TRACE_VFIO_PCI_H */
>>>>>>>>
>>>>>>>> #undef TRACE_INCLUDE_PATH
>>>>>>>> -#define TRACE_INCLUDE_PATH .
>>>>>>>> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>>>>>>>> #undef TRACE_INCLUDE_FILE
>>>>>>>> #define TRACE_INCLUDE_FILE trace
>>>>>>>
>>>>>>> Going from the comments in samples/trace_events/trace-events-sample.h,
>>>>>>> I think both approaches are possible, and I see both used in various
>>>>>>> places.
>>>>>>>
>>>>>>> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
>>>>>>> a path.
>>>>>
>>>>> Numbering options for clarity:
>>>>>
>>>>> 1)
>>>>>> ccflags-y += -I$(src)
>>>>>> would add the header search path for all files in drivers/vfio/pci/
>>>>>> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
>>>>>>
>>>>>
>>>>> 2)
>>>>>> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
>>>>>> is a bit better.
>>>>>> However, it is not obvious why this extra header search path is needed
>>>>>> until you find vfio_pci_nvlink2.c including trace.h
>>>>>>
>>>>>
>>>>> 3)
>>>>>> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>>>>>> clarifies the intention because the related code is all placed in trace.h
>>>>>>
>>>>>>
>>>>>>
>>>>>> From the comment in include/trace/define_trace.h
>>>>>> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
>>>>>
>>>>> In my scan of the tree, the most common solution seems to be 2) as this
>>>>> is essentially recommended in the sample file. 3) is well represented,
>>>>> with much fewer examples of 1), though it might depend how liberally
>>>>> we grep out or examine the use cases. Choice 1) also seems to be the
>>>>> most shotgun approach, adding to the search path for all files.
>>>>
>>>>
>>>> The shotgun approach is always used when compiling out of tree which is
>>>> what many people do anyway without realizing that there are additional
>>>> -I. Why do not we make in-tree behave the same way? Thanks,
>>>
>>> Are you suggesting bandaging this individual leaf directory, as in
>>> choice 1), because it's no worse than what happens for an out-of-tree
>>> build anyway,
>>
>>
>> I suggest making in-tree and out-of-tree behavior equal - both either
>> fail or compile. Just makes sense to me.
>>
>> Since out-of-tree adds extra -I, then there should have been a reason
>> for that at the time (before 2005 though). Unfortunately git blame does
>> not say why it was done this way for out-of-tree so imho the safest
>> thing is to add -I for in-tree as well. Or ditch extra -I and do 2) or
>> 3) - this is fine too and can count as an impressive compile
>> optimization, although... look below :)
>
>
> For the out-of-tree building,
> scripts/Makefile.lib adds -I$(srctree)/$(src) and -I$(obj).
>
>
> In my understanding, -I$(srctree)/$(src) is for
> including check-in headers from generated C files.
>
> -I$(obj) is for including generated headers from check-in C files.
>
>
> One example of the correct usage of this is,
>
> crypto/rsa_helper.c (check-in file)
>
> includes
>
> crypto/rsapubkey.asn1.h (generated fle)
>
>
>
> Those extra header search paths are unneeded for in-tree building
> since generated files and check-in files exist in the same tree.
>
>
>
> You just happened to find it works for out-of-tree building.
>
>
> We are talking about how to make include/trace/define_trace.h
> include drivers/vfio/pci/trace.h
>
> The build system cannot (should not)
> cater to such a particular case.
Oh well, thanks for the explanation.
Alex, how do we proceed now? I like 2) + comment as in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/ocxl/Makefile?h=v4.20#n8
better than 3) as relative paths make it dependable on what file
actually includes it and it is not clear what is that and what happens
if that file moves deeper in the tree (unlikely to happen though).
--
Alexey
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-08 2:57 ` Alexey Kardashevskiy
@ 2019-01-08 3:19 ` Masahiro Yamada
0 siblings, 0 replies; 16+ messages in thread
From: Masahiro Yamada @ 2019-01-08 3:19 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Alex Williamson, Cornelia Huck, Michael Ellerman, Laura Abbott,
kvm, Linux Kernel Mailing List
On Tue, Jan 8, 2019 at 11:59 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
>
>
> On 08/01/2019 13:38, Masahiro Yamada wrote:
> > On Tue, Jan 8, 2019 at 11:22 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>
> >>
> >>
> >> On 08/01/2019 11:24, Alex Williamson wrote:
> >>> On Tue, 8 Jan 2019 10:52:43 +1100
> >>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>>
> >>>> On 08/01/2019 07:13, Alex Williamson wrote:
> >>>>> On Mon, 7 Jan 2019 20:39:19 +0900
> >>>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >>>>>
> >>>>>> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
> >>>>>>>
> >>>>>>> On Mon, 7 Jan 2019 19:12:10 +0900
> >>>>>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >>>>>>>
> >>>>>>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> >>>>>>>>>
> >>>>>>>>> Laura Abbott <labbott@redhat.com> writes:
> >>>>>>>>>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> >>>>>>>>>> subdriver") introduced a trace.h file in the local directory but
> >>>>>>>>>> missed adding the local include path, resulting in compilation
> >>>>>>>>>> failures with tracepoints:
> >>>>>>>>>>
> >>>>>>>>>> In file included from drivers/vfio/pci/trace.h:102,
> >>>>>>>>>> from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> >>>>>>>>>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> >>>>>>>>>> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> >>>>>>>>>>
> >>>>>>>>>> Fix this by adjusting the include path.
> >>>>>>>>>>
> >>>>>>>>>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> >>>>>>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
> >>>>>>>
> >>>>>>> (...)
> >>>>>>>
> >>>>>>>>> Alex I assume you'll merge this fix via the vfio tree?
> >>>>>>>>>
> >>>>>>>>> cheers
> >>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> >>>>>>>>>> index 9662c063a6b1..08d4676a8495 100644
> >>>>>>>>>> --- a/drivers/vfio/pci/Makefile
> >>>>>>>>>> +++ b/drivers/vfio/pci/Makefile
> >>>>>>>>>> @@ -1,3 +1,4 @@
> >>>>>>>>>> +ccflags-y += -I$(src)
> >>>>>>>>>>
> >>>>>>>>>> vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> >>>>>>>>>> vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> >>>>>>>>>> --
> >>>>>>>>>> 2.20.1
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Hi.
> >>>>>>>>
> >>>>>>>> If I correctly understand the usage of TRACE_INCLUDE_PATH,
> >>>>>>>> the correct fix should be like follows:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
> >>>>>>>> index 228ccdb..4d13e51 100644
> >>>>>>>> --- a/drivers/vfio/pci/trace.h
> >>>>>>>> +++ b/drivers/vfio/pci/trace.h
> >>>>>>>> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
> >>>>>>>> #endif /* _TRACE_VFIO_PCI_H */
> >>>>>>>>
> >>>>>>>> #undef TRACE_INCLUDE_PATH
> >>>>>>>> -#define TRACE_INCLUDE_PATH .
> >>>>>>>> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> >>>>>>>> #undef TRACE_INCLUDE_FILE
> >>>>>>>> #define TRACE_INCLUDE_FILE trace
> >>>>>>>
> >>>>>>> Going from the comments in samples/trace_events/trace-events-sample.h,
> >>>>>>> I think both approaches are possible, and I see both used in various
> >>>>>>> places.
> >>>>>>>
> >>>>>>> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
> >>>>>>> a path.
> >>>>>
> >>>>> Numbering options for clarity:
> >>>>>
> >>>>> 1)
> >>>>>> ccflags-y += -I$(src)
> >>>>>> would add the header search path for all files in drivers/vfio/pci/
> >>>>>> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
> >>>>>>
> >>>>>
> >>>>> 2)
> >>>>>> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
> >>>>>> is a bit better.
> >>>>>> However, it is not obvious why this extra header search path is needed
> >>>>>> until you find vfio_pci_nvlink2.c including trace.h
> >>>>>>
> >>>>>
> >>>>> 3)
> >>>>>> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> >>>>>> clarifies the intention because the related code is all placed in trace.h
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> From the comment in include/trace/define_trace.h
> >>>>>> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
> >>>>>
> >>>>> In my scan of the tree, the most common solution seems to be 2) as this
> >>>>> is essentially recommended in the sample file. 3) is well represented,
> >>>>> with much fewer examples of 1), though it might depend how liberally
> >>>>> we grep out or examine the use cases. Choice 1) also seems to be the
> >>>>> most shotgun approach, adding to the search path for all files.
> >>>>
> >>>>
> >>>> The shotgun approach is always used when compiling out of tree which is
> >>>> what many people do anyway without realizing that there are additional
> >>>> -I. Why do not we make in-tree behave the same way? Thanks,
> >>>
> >>> Are you suggesting bandaging this individual leaf directory, as in
> >>> choice 1), because it's no worse than what happens for an out-of-tree
> >>> build anyway,
> >>
> >>
> >> I suggest making in-tree and out-of-tree behavior equal - both either
> >> fail or compile. Just makes sense to me.
> >>
> >> Since out-of-tree adds extra -I, then there should have been a reason
> >> for that at the time (before 2005 though). Unfortunately git blame does
> >> not say why it was done this way for out-of-tree so imho the safest
> >> thing is to add -I for in-tree as well. Or ditch extra -I and do 2) or
> >> 3) - this is fine too and can count as an impressive compile
> >> optimization, although... look below :)
> >
> >
> > For the out-of-tree building,
> > scripts/Makefile.lib adds -I$(srctree)/$(src) and -I$(obj).
> >
> >
> > In my understanding, -I$(srctree)/$(src) is for
> > including check-in headers from generated C files.
> >
> > -I$(obj) is for including generated headers from check-in C files.
> >
> >
> > One example of the correct usage of this is,
> >
> > crypto/rsa_helper.c (check-in file)
> >
> > includes
> >
> > crypto/rsapubkey.asn1.h (generated fle)
> >
> >
> >
> > Those extra header search paths are unneeded for in-tree building
> > since generated files and check-in files exist in the same tree.
> >
> >
> >
> > You just happened to find it works for out-of-tree building.
> >
> >
> > We are talking about how to make include/trace/define_trace.h
> > include drivers/vfio/pci/trace.h
> >
> > The build system cannot (should not)
> > cater to such a particular case.
>
>
> Oh well, thanks for the explanation.
>
> Alex, how do we proceed now? I like 2) + comment as in
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/ocxl/Makefile?h=v4.20#n8
It is clear, but
3) TRACE_INCLUDE_PATH solution
is self-documenting.
> better than 3) as relative paths make it dependable on what file
> actually includes it and it is not clear what is that and what happens
> if that file moves deeper in the tree (unlikely to happen though).
From the build system PoV, I'd like to discourage people
from adding crappy header search path like this.
Both 2) and 3) have pros and cons.
I just sent 3) patch, but it is the maintainer's call after all.
>
>
>
>
> --
> Alexey
--
Best Regards
Masahiro Yamada
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] vfio_pci: Add local source directory as include
2019-01-07 20:13 ` Alex Williamson
2019-01-07 23:52 ` Alexey Kardashevskiy
@ 2019-01-08 8:02 ` Michael Ellerman
1 sibling, 0 replies; 16+ messages in thread
From: Michael Ellerman @ 2019-01-08 8:02 UTC (permalink / raw)
To: Alex Williamson, Masahiro Yamada
Cc: Cornelia Huck, Laura Abbott, Alexey Kardashevskiy, kvm,
Linux Kernel Mailing List
Alex Williamson <alex.williamson@redhat.com> writes:
...
>
> Numbering options for clarity:
>
> 1)
>> ccflags-y += -I$(src)
>> would add the header search path for all files in drivers/vfio/pci/
>> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
>>
>
> 2)
>> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
>> is a bit better.
>> However, it is not obvious why this extra header search path is needed
>> until you find vfio_pci_nvlink2.c including trace.h
>>
>
> 3)
>> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>> clarifies the intention because the related code is all placed in trace.h
Good summary.
>> From the comment in include/trace/define_trace.h
>> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
>
> In my scan of the tree, the most common solution seems to be 2) as this
> is essentially recommended in the sample file. 3) is well represented,
> with much fewer examples of 1), though it might depend how liberally
> we grep out or examine the use cases.
It seems to me that 1 and 2 is overwhelmingly used:
$ git grep -F "#define TRACE_INCLUDE_PATH" | wc -l
133
That counts all definitions of TRACE_INCLUDE_PATH.
$ git grep -F "#define TRACE_INCLUDE_PATH ." | wc -l
122
That's all files using '.', so only 11 locations use the relative path
method (3).
Which is unsurprising given that the sample uses '.'.
And people often look at existing code for an example, so they're also
going to tend to use '.'.
I agree with Masahiro that adding include paths to the Makefile for
this is a bit gross, and method 3 is much more preferable.
Fixing all the existing code to use method 3 would be a good beginner
project :)
cheers
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-01-08 8:02 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-04 19:57 [PATCH] vfio_pci: Add local source directory as include Laura Abbott
2019-01-07 7:26 ` Alexey Kardashevskiy
2019-01-07 8:58 ` Michael Ellerman
2019-01-07 10:12 ` Masahiro Yamada
2019-01-07 11:07 ` Cornelia Huck
2019-01-07 11:39 ` Masahiro Yamada
2019-01-07 20:13 ` Alex Williamson
2019-01-07 23:52 ` Alexey Kardashevskiy
2019-01-08 0:24 ` Alex Williamson
2019-01-08 2:20 ` Alexey Kardashevskiy
2019-01-08 2:38 ` Masahiro Yamada
2019-01-08 2:57 ` Alexey Kardashevskiy
2019-01-08 3:19 ` Masahiro Yamada
2019-01-08 8:02 ` Michael Ellerman
2019-01-07 20:06 ` Laura Abbott
2019-01-07 11:12 ` Cornelia Huck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).