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

2019-01-08 Thread Michael Ellerman 
Alex Williamson  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

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

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

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

2019-01-07 Thread Alexey Kardashevskiy 



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

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

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

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

2019-01-07 Thread Alexey Kardashevskiy 



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

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

2019-01-07 Thread Alex Williamson 
On Tue, 8 Jan 2019 10:52:43 +1100
Alexey Kardashevskiy  wrote:

> On 08/01/2019 07:13, Alex Williamson wrote:
> > On Mon, 7 Jan 2019 20:39:19 +0900
> > Masahiro Yamada  wrote:
> >   
> >> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck  wrote:  
> >>>
> >>> On Mon, 7 Jan 2019 19:12:10 +0900
> >>> Masahiro Yamada  wrote:
> >>>
>  On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman  
>  wrote:
> >
> > Laura Abbott  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 
> >>>
> >>> (...)
> >>>
> > 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

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

2019-01-07 Thread Alexey Kardashevskiy 



On 08/01/2019 07:13, Alex Williamson wrote:
> On Mon, 7 Jan 2019 20:39:19 +0900
> Masahiro Yamada  wrote:
> 
>> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck  wrote:
>>>
>>> On Mon, 7 Jan 2019 19:12:10 +0900
>>> Masahiro Yamada  wrote:
>>>  
 On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman  
 wrote:  
>
> Laura Abbott  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   
>>>
>>> (...)
>>>  
> 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,

-- 
Alexey

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

2019-01-07 Thread Alex Williamson 
On Mon, 7 Jan 2019 20:39:19 +0900
Masahiro Yamada  wrote:

> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck  wrote:
> >
> > On Mon, 7 Jan 2019 19:12:10 +0900
> > Masahiro Yamada  wrote:
> >  
> > > On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman  
> > > wrote:  
> > > >
> > > > Laura Abbott  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   
> >
> > (...)
> >  
> > > > 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

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

2019-01-07 Thread Laura Abbott 

On 1/7/19 12:58 AM, Michael Ellerman wrote:

Laura Abbott  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 
---
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

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

2019-01-07 Thread Masahiro Yamada 
On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck  wrote:
>
> On Mon, 7 Jan 2019 19:12:10 +0900
> Masahiro Yamada  wrote:
>
> > On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman  wrote:
> > >
> > > Laura Abbott  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 
>
> (...)
>
> > > 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




-- 
Best Regards
Masahiro Yamada

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

2019-01-07 Thread Cornelia Huck 
On Fri,  4 Jan 2019 11:57:14 -0800
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 
> ---
> 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

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

2019-01-07 Thread Cornelia Huck 
On Mon, 7 Jan 2019 19:12:10 +0900
Masahiro Yamada  wrote:

> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman  wrote:
> >
> > Laura Abbott  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 

(...)

> > 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.

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

2019-01-07 Thread Masahiro Yamada 
On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman  wrote:
>
> Laura Abbott  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 
> > ---
> > 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









-- 
Best Regards
Masahiro Yamada

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

2019-01-07 Thread Michael Ellerman 
Laura Abbott  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 
> ---
> 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

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

2019-01-06 Thread Alexey Kardashevskiy 



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 



Reviewed-by: Alexey Kardashevskiy 



> ---
> 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
> 

-- 
Alexey

[PATCH] vfio_pci: Add local source directory as include

2019-01-04 Thread Laura Abbott 
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 
---
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
-- 
2.20.1