On Tue, Jan 8, 2019 at 11:22 AM Alexey Kardashevskiy <aik@xxxxxxxxx> wrote: > > > > On 08/01/2019 11:24, Alex Williamson wrote: > > On Tue, 8 Jan 2019 10:52:43 +1100 > > Alexey Kardashevskiy <aik@xxxxxxxxx> wrote: > > > >> On 08/01/2019 07:13, Alex Williamson wrote: > >>> On Mon, 7 Jan 2019 20:39:19 +0900 > >>> Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote: > >>> > >>>> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@xxxxxxxxxx> wrote: > >>>>> > >>>>> On Mon, 7 Jan 2019 19:12:10 +0900 > >>>>> Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx> wrote: > >>>>> > >>>>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@xxxxxxxxxxxxxx> wrote: > >>>>>>> > >>>>>>> Laura Abbott <labbott@xxxxxxxxxx> 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@xxxxxxxxxx> > >>>>> > >>>>> (...) > >>>>> > >>>>>>> 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