Date:   Mon, 7 Jan 2019 17:24:37 -0700
From:   Alex Williamson <alex.williamson@...hat.com>
To:     Alexey Kardashevskiy <aik@...abs.ru>
Cc:     Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Cornelia Huck <cohuck@...hat.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Laura Abbott <labbott@...hat.com>, kvm@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] vfio_pci: Add local source directory as include

On Tue, 8 Jan 2019 10:52:43 +1100
Alexey Kardashevskiy <aik@...abs.ru> wrote:

> On 08/01/2019 07:13, Alex Williamson wrote:
> > On Mon, 7 Jan 2019 20:39:19 +0900
> > Masahiro Yamada <yamada.masahiro@...ionext.com> wrote:
> >   
> >> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@...hat.com> wrote:  
> >>>
> >>> On Mon, 7 Jan 2019 19:12:10 +0900
> >>> Masahiro Yamada <yamada.masahiro@...ionext.com> wrote:
> >>>    
> >>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@...erman.id.au> wrote:    
> >>>>>
> >>>>> Laura Abbott <labbott@...hat.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@...hat.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,  
> 

Powered by blists - more mailing lists