| Message ID | 20190104195714.30045-1-labbott@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio_pci: Add local source directory as include | expand |
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 >
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
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
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.
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>
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
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
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
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,
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, >
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,
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
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).
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
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
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
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(+)