Re: VERBOSE flags

Re: VERBOSE flags

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 2552 bytes --]

On 9/14/2016 2:37 AM, Leon Romanovsky wrote:
> On Wed, Sep 14, 2016 at 9:17 AM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On 9/14/2016 1:27 AM, Leon Romanovsky wrote:
>>> Hi Doug,
>>>
>>> Can you please respond to my questions to this patch [1]?
>>>
>>> Thanks
>>>
>>> [1] https://patchwork.kernel.org/patch/9302591/
>>>
>>
>> I don't appear to still have that email.  But I know what I came up with
>> at the time.
>>
>> The debug information was in the mlx5 entry point for post_send.  That
>> means you can create a sysfs entry that allows you to enable or disable
>> the debugging, and then make two different mlx5 post_send entry points.
>> When debugging is enabled, you simply change the entry point for
>> mlx5_post_send to mlx5_post_send_debug, where mlx5_post_send_debug is
>> essentially nothing but mlx5_post_send; dump_wqe.  The unconditional
>> branch will be highly optimized by the CPU, so no real loss on it.  And
>> when not in debug mode, there is zero penalty.  In short, make use of
>> the entry pointers to make this configurable but also zero penalty on
>> the hot path in the non-debug case.
> 
> I did it in similar way to Intel's HFI driver and had in mind future
> consolidation of all
> these verbose flags for whole subsystem.
> 
> I proposed topic (debuggability) in mini-summit to talk about it.

OK, but if the hfi1 driver does this similar to what you were doing,
then I think we should change it too.

Essentially, given that all IB devices have a set of function pointers
that are registered and stored on a per device basis, there is no reason
that the core code can't be modified to present
/sys/class/infiniband/debug/<list of specific entry points we wish to
have possible debug functions for> and if the user wants to debug a
specific entry point, they set it to 1 when it's normally 0.  Then
drivers can optionally register debug versions of their entry points,
and in the core if the user tries to enable debug on an entry point, and
the driver registered a debug variant for that entry point, we start
using it.  Then we can drop the CONFIG_*_DEBUG options out of all of the
drivers.  That would be my preference.  I don't want any compile time
options because you never will be running a debug kernel when you need
it.  Per device, run time selectable so that you get 0 overhead in the
normal case is what I would like to see.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: VERBOSE flags

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3528 bytes --]

On Sun, Sep 18, 2016 at 04:35:37PM -0400, Doug Ledford wrote:
> On 9/14/2016 2:37 AM, Leon Romanovsky wrote:
> > On Wed, Sep 14, 2016 at 9:17 AM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> >> On 9/14/2016 1:27 AM, Leon Romanovsky wrote:
> >>> Hi Doug,
> >>>
> >>> Can you please respond to my questions to this patch [1]?
> >>>
> >>> Thanks
> >>>
> >>> [1] https://patchwork.kernel.org/patch/9302591/
> >>>
> >>
> >> I don't appear to still have that email.  But I know what I came up with
> >> at the time.
> >>
> >> The debug information was in the mlx5 entry point for post_send.  That
> >> means you can create a sysfs entry that allows you to enable or disable
> >> the debugging, and then make two different mlx5 post_send entry points.
> >> When debugging is enabled, you simply change the entry point for
> >> mlx5_post_send to mlx5_post_send_debug, where mlx5_post_send_debug is
> >> essentially nothing but mlx5_post_send; dump_wqe.  The unconditional
> >> branch will be highly optimized by the CPU, so no real loss on it.  And
> >> when not in debug mode, there is zero penalty.  In short, make use of
> >> the entry pointers to make this configurable but also zero penalty on
> >> the hot path in the non-debug case.
> >
> > I did it in similar way to Intel's HFI driver and had in mind future
> > consolidation of all
> > these verbose flags for whole subsystem.
> >
> > I proposed topic (debuggability) in mini-summit to talk about it.
>
> OK, but if the hfi1 driver does this similar to what you were doing,
> then I think we should change it too.
>
> Essentially, given that all IB devices have a set of function pointers
> that are registered and stored on a per device basis, there is no reason
> that the core code can't be modified to present
> /sys/class/infiniband/debug/<list of specific entry points we wish to
> have possible debug functions for> and if the user wants to debug a
> specific entry point, they set it to 1 when it's normally 0.  Then
> drivers can optionally register debug versions of their entry points,
> and in the core if the user tries to enable debug on an entry point, and
> the driver registered a debug variant for that entry point, we start
> using it.

As a short writeup to anyone who lacks context of this thread, we are talking
about debug information in DATA paths flows. The control paths don't need
anything special and standard dynamic_debug and/or tracing infrastructure
will be enough for them.

Doug,
The function pointers will cause us to create two similar functions
(debug/regular) for every important function. It will leave to code
duplication and/or introducing of new code to fill additional fields
in returns to debug functions, so they will be able to print it.

I wanted to propose to use tracepoints to provide debug information in
data paths. The tracepoints infrastructure uses hooks in the code with
minimal impact on performance.


> Then we can drop the CONFIG_*_DEBUG options out of all of the
> drivers.  That would be my preference.  I don't want any compile time
> options because you never will be running a debug kernel when you need
> it.  Per device, run time selectable so that you get 0 overhead in the
> normal case is what I would like to see.

Great,
We are aligned on this point, so do I. I see no reason in special compiled
kernel or global module parameter to print debug prints for whole driver.

>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG Key ID: 0E572FDD
>




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: VERBOSE flags

[Dalessandro, Dennis]

On Mon, 2016-09-19 at 09:15 +0300, Leon Romanovsky wrote:
> The function pointers will cause us to create two similar functions
> (debug/regular) for every important function. It will leave to code
> duplication and/or introducing of new code to fill additional fields
> in returns to debug functions, so they will be able to print it.

I do sort of like Doug's idea. It means you can easily instrument the
heck out of a routine and have it always available. It provides more
flexibility in what you can do in a debug version.

You do make a valid point though about duplicating code. Perhaps that
can be alleviated to some extent just by refactoring and adding
inlines, etc.  

> I wanted to propose to use tracepoints to provide debug information
> in
> data paths. The tracepoints infrastructure uses hooks in the code
> with
> minimal impact on performance.

This is so far the path we have chosen for hfi1. We have a number of
tracepoints in various parts of the code. Again it's available on
demand without special kernels or boot/driver options. 

> > Then we can drop the CONFIG_*_DEBUG options out of all of the
> > drivers.  That would be my preference.  I don't want any compile
> > time
> > options because you never will be running a debug kernel when you
> > need
> > it.  Per device, run time selectable so that you get 0 overhead in
> > the
> > normal case is what I would like to see.
> 
> Great,
> We are aligned on this point, so do I. I see no reason in special
> compiled
> kernel or global module parameter to print debug prints for whole
> driver.

Same here.

-Denny

Re: VERBOSE flags

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]

On Mon, Sep 19, 2016 at 12:29:43PM +0000, Dalessandro, Dennis wrote:
> On Mon, 2016-09-19 at 09:15 +0300, Leon Romanovsky wrote:
> > The function pointers will cause us to create two similar functions
> > (debug/regular) for every important function. It will leave to code
> > duplication and/or introducing of new code to fill additional fields
> > in returns to debug functions, so they will be able to print it.
>
> I do sort of like Doug's idea. It means you can easily instrument the
> heck out of a routine and have it always available. It provides more
> flexibility in what you can do in a debug version.
>
> You do make a valid point though about duplicating code. Perhaps that
> can be alleviated to some extent just by refactoring and adding
> inlines, etc.  

At the end, we will invent tracepoints :)

>
> > I wanted to propose to use tracepoints to provide debug information
> > in
> > data paths. The tracepoints infrastructure uses hooks in the code
> > with
> > minimal impact on performance.
>
> This is so far the path we have chosen for hfi1. We have a number of
> tracepoints in various parts of the code. Again it's available on
> demand without special kernels or boot/driver options. 

CONFIG_SDMA_VERBOSITY ????

>
> > > Then we can drop the CONFIG_*_DEBUG options out of all of the
> > > drivers.  That would be my preference.  I don't want any compile
> > > time
> > > options because you never will be running a debug kernel when you
> > > need
> > > it.  Per device, run time selectable so that you get 0 overhead in
> > > the
> > > normal case is what I would like to see.
> >
> > Great,
> > We are aligned on this point, so do I. I see no reason in special
> > compiled
> > kernel or global module parameter to print debug prints for whole
> > driver.
>
> Same here.
>
> -Denny

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: VERBOSE flags

[Dalessandro, Dennis]

On Mon, 2016-09-19 at 16:25 +0300, Leon Romanovsky wrote:
> On Mon, Sep 19, 2016 at 12:29:43PM +0000, Dalessandro, Dennis wrote:
> > On Mon, 2016-09-19 at 09:15 +0300, Leon Romanovsky wrote:
> > > The function pointers will cause us to create two similar
> > > functions
> > > (debug/regular) for every important function. It will leave to
> > > code
> > > duplication and/or introducing of new code to fill additional
> > > fields
> > > in returns to debug functions, so they will be able to print it.
> > 
> > I do sort of like Doug's idea. It means you can easily instrument
> > the
> > heck out of a routine and have it always available. It provides
> > more
> > flexibility in what you can do in a debug version.
> > 
> > You do make a valid point though about duplicating code. Perhaps
> > that
> > can be alleviated to some extent just by refactoring and adding
> > inlines, etc.  
> 
> At the end, we will invent tracepoints :)
> 
> > 
> > > I wanted to propose to use tracepoints to provide debug
> > > information
> > > in
> > > data paths. The tracepoints infrastructure uses hooks in the code
> > > with
> > > minimal impact on performance.
> > 
> > This is so far the path we have chosen for hfi1. We have a number
> > of
> > tracepoints in various parts of the code. Again it's available on
> > demand without special kernels or boot/driver options. 
> 
> CONFIG_SDMA_VERBOSITY ????

Yes we do have that, which illustrates that trace points can not do
everything. I had hoped to deprecate that at some point. Regardless,
sometimes you want additional code to be added or to do something
slightly different. You can't do that with a tracepoint.

You can see we use tracepoints pretty extensively and that is our main
vehicle for debugging. Here is where all the definitions are:

$ ls trace_*
trace_ctxts.h  trace_dbg.h  trace_ibhdrs.h  trace_misc.h  trace_rc.h  t
race_rx.h  trace_tx.h

-Denny

> > 
> > > > Then we can drop the CONFIG_*_DEBUG options out of all of the
> > > > drivers.  That would be my preference.  I don't want any
> > > > compile
> > > > time
> > > > options because you never will be running a debug kernel when
> > > > you
> > > > need
> > > > it.  Per device, run time selectable so that you get 0 overhead
> > > > in
> > > > the
> > > > normal case is what I would like to see.
> > > 
> > > Great,
> > > We are aligned on this point, so do I. I see no reason in special
> > > compiled
> > > kernel or global module parameter to print debug prints for whole
> > > driver.
> > 
> > Same here.
> > 
> > -Denny

Re: VERBOSE flags

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 2180 bytes --]

On 9/19/2016 2:15 AM, Leon Romanovsky wrote:
> On Sun, Sep 18, 2016 at 04:35:37PM -0400, Doug Ledford wrote:

> Doug,
> The function pointers will cause us to create two similar functions
> (debug/regular) for every important function. It will leave to code
> duplication and/or introducing of new code to fill additional fields
> in returns to debug functions, so they will be able to print it.

That's situationally specific.  The original patch you posted that I
said no to is an example of where this wouldn't have been true.
Basically it all boils down to whether or not your debug code can be a
wrapper of your existing function or if it needs to be embedded in the
middle of it.  If the latter, then you are more likely to have code
duplication as you point out.  But, then refactoring code can help with
that problem too.

> I wanted to propose to use tracepoints to provide debug information in
> data paths. The tracepoints infrastructure uses hooks in the code with
> minimal impact on performance.

Yes, but one of the things you said before was "zero impact on normal
operations", but with tracepoints you just said "minimal impact".  So it
sounds like tracepoints don't meet your own initial requirements.  I'm
not against them, but utilizing the entry points allows for achievment
of the "zero impact" requirement.  And as Denny pointed out, some things
can't be done with tracepoints.  So, the final solution might be a mix
of different things including different entry points, tracepoints, and
some core infrastructure.

> 
>> Then we can drop the CONFIG_*_DEBUG options out of all of the
>> drivers.  That would be my preference.  I don't want any compile time
>> options because you never will be running a debug kernel when you need
>> it.  Per device, run time selectable so that you get 0 overhead in the
>> normal case is what I would like to see.
> 
> Great,
> We are aligned on this point, so do I. I see no reason in special compiled
> kernel or global module parameter to print debug prints for whole driver.



-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: VERBOSE flags

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2572 bytes --]

On Mon, Sep 19, 2016 at 12:03:34PM -0400, Doug Ledford wrote:
> On 9/19/2016 2:15 AM, Leon Romanovsky wrote:
> > On Sun, Sep 18, 2016 at 04:35:37PM -0400, Doug Ledford wrote:
>
> > Doug,
> > The function pointers will cause us to create two similar functions
> > (debug/regular) for every important function. It will leave to code
> > duplication and/or introducing of new code to fill additional fields
> > in returns to debug functions, so they will be able to print it.
>
> That's situationally specific.  The original patch you posted that I
> said no to is an example of where this wouldn't have been true.
> Basically it all boils down to whether or not your debug code can be a
> wrapper of your existing function or if it needs to be embedded in the
> middle of it.  If the latter, then you are more likely to have code
> duplication as you point out.  But, then refactoring code can help with
> that problem too.

That specific patch was sent to ensure that if(0) {...} code isn't
removed by mistake.

>
> > I wanted to propose to use tracepoints to provide debug information in
> > data paths. The tracepoints infrastructure uses hooks in the code with
> > minimal impact on performance.
>
> Yes, but one of the things you said before was "zero impact on normal
> operations", but with tracepoints you just said "minimal impact".  So it
> sounds like tracepoints don't meet your own initial requirements.  I'm
> not against them, but utilizing the entry points allows for achievment
> of the "zero impact" requirement.  And as Denny pointed out, some things
> can't be done with tracepoints.  So, the final solution might be a mix
> of different things including different entry points, tracepoints, and
> some core infrastructure.

You are right and my initial requirement which is fine in theory (zero
impact) not so good in real life (precompile e.t.c).

Let's start from the tracepoints and see what elase we need to add to core.

>
> >
> >> Then we can drop the CONFIG_*_DEBUG options out of all of the
> >> drivers.  That would be my preference.  I don't want any compile time
> >> options because you never will be running a debug kernel when you need
> >> it.  Per device, run time selectable so that you get 0 overhead in the
> >> normal case is what I would like to see.
> >
> > Great,
> > We are aligned on this point, so do I. I see no reason in special compiled
> > kernel or global module parameter to print debug prints for whole driver.
>
>
>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG Key ID: 0E572FDD
>




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: VERBOSE flags

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2440 bytes --]

On Mon, Sep 19, 2016 at 01:34:39PM +0000, Dalessandro, Dennis wrote:
> On Mon, 2016-09-19 at 16:25 +0300, Leon Romanovsky wrote:
> > On Mon, Sep 19, 2016 at 12:29:43PM +0000, Dalessandro, Dennis wrote:
> > > On Mon, 2016-09-19 at 09:15 +0300, Leon Romanovsky wrote:
> > > > The function pointers will cause us to create two similar
> > > > functions
> > > > (debug/regular) for every important function. It will leave to
> > > > code
> > > > duplication and/or introducing of new code to fill additional
> > > > fields
> > > > in returns to debug functions, so they will be able to print it.
> > >
> > > I do sort of like Doug's idea. It means you can easily instrument
> > > the
> > > heck out of a routine and have it always available. It provides
> > > more
> > > flexibility in what you can do in a debug version.
> > >
> > > You do make a valid point though about duplicating code. Perhaps
> > > that
> > > can be alleviated to some extent just by refactoring and adding
> > > inlines, etc.  
> >
> > At the end, we will invent tracepoints :)
> >
> > >
> > > > I wanted to propose to use tracepoints to provide debug
> > > > information
> > > > in
> > > > data paths. The tracepoints infrastructure uses hooks in the code
> > > > with
> > > > minimal impact on performance.
> > >
> > > This is so far the path we have chosen for hfi1. We have a number
> > > of
> > > tracepoints in various parts of the code. Again it's available on
> > > demand without special kernels or boot/driver options. 
> >
> > CONFIG_SDMA_VERBOSITY ????
>
> Yes we do have that, which illustrates that trace points can not do
> everything. I had hoped to deprecate that at some point. Regardless,
> sometimes you want additional code to be added or to do something
> slightly different. You can't do that with a tracepoint.

Can you elaborate more on this topic? Which limitations did you see
in tracepoints?

In regards of other discussion in KS 2016, "tracepoints and ABI
stability warranties" [1]. Are you strict with this tracepoints ABI?

>
> You can see we use tracepoints pretty extensively and that is our main
> vehicle for debugging. Here is where all the definitions are:
>
> $ ls trace_*
> trace_ctxts.h  trace_dbg.h  trace_ibhdrs.h  trace_misc.h  trace_rc.h  t
> race_rx.h  trace_tx.h

+1 :)

[1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2016-September/003850.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: VERBOSE flags

[Dalessandro, Dennis]

On Tue, 2016-09-20 at 13:56 +0300, Leon Romanovsky wrote:
> > Yes we do have that, which illustrates that trace points can not do
> > everything. I had hoped to deprecate that at some point.
> > Regardless,
> > sometimes you want additional code to be added or to do something
> > slightly different. You can't do that with a tracepoint.
> 
> Can you elaborate more on this topic? Which limitations did you see
> in tracepoints?

Just making up an example. Say you have a hash table. You want to dump
the items. If you iterate over it and do a trace point, yeah the actual
trace point has little to no perf impact, but you still walk the
table. 

There are situations where you need to do more for debugging than just
printing a message. 

> In regards of other discussion in KS 2016, "tracepoints and ABI
> stability warranties" [1]. Are you strict with this tracepoints ABI?

Honestly we are not super strict about it, but do take care not to
break/remove something that could be useful. It will be interesting to
see what comes out of the discussion. Perhaps we do need to be more
strict about them going forward.

-Denny

Re: VERBOSE flags

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

On Tue, Sep 20, 2016 at 12:11:40PM +0000, Dalessandro, Dennis wrote:
> On Tue, 2016-09-20 at 13:56 +0300, Leon Romanovsky wrote:
> > > Yes we do have that, which illustrates that trace points can not do
> > > everything. I had hoped to deprecate that at some point.
> > > Regardless,
> > > sometimes you want additional code to be added or to do something
> > > slightly different. You can't do that with a tracepoint.
> >
> > Can you elaborate more on this topic? Which limitations did you see
> > in tracepoints?
>
> Just making up an example. Say you have a hash table. You want to dump
> the items. If you iterate over it and do a trace point, yeah the actual
> trace point has little to no perf impact, but you still walk the
> table. 
>
> There are situations where you need to do more for debugging than just
> printing a message. 

Thanks

>
> > In regards of other discussion in KS 2016, "tracepoints and ABI
> > stability warranties" [1]. Are you strict with this tracepoints ABI?
>
> Honestly we are not super strict about it, but do take care not to
> break/remove something that could be useful. It will be interesting to
> see what comes out of the discussion. Perhaps we do need to be more
> strict about them going forward.

I followed the discussion and it looks like that majority are against to
declare tracepoints as ABI. Otherwise, you will find yourself unable to
change the kernel code.

>
> -Denny

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]