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
>