Re: [PATCH] vfio_pci: Add local source directory as include

[Masahiro Yamada]

On Tue, Jan 8, 2019 at 11:59 AM Alexey Kardashevskiy <aik@xxxxxxxxx> wrote:
>
>
>
> On 08/01/2019 13:38, Masahiro Yamada wrote:
> > 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.
>
>
> 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