Message-ID: <34647ed9-6aa3-6e38-f3a5-87da87e9341a@ozlabs.ru>
Date:   Tue, 8 Jan 2019 13:57:53 +1100
From:   Alexey Kardashevskiy <aik@...abs.ru>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     Alex Williamson <alex.williamson@...hat.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 08/01/2019 13:38, Masahiro Yamada wrote:
> On Tue, Jan 8, 2019 at 11:22 AM Alexey Kardashevskiy <aik@...abs.ru> wrote:
>>
>>
>>
>> On 08/01/2019 11:24, Alex Williamson wrote:
>>> 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,
>>
>>
>> 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

Powered by blists - more mailing lists