Date:   Thu, 15 Sep 2016 07:37:16 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     "Mintz, Yuval" <Yuval.Mintz@...ium.com>
Cc:     Yuval Mintz <Yuval.Mintz@...gic.com>,
        Mark Bloch <markb@...lanox.com>,
        Ram Amrani <Ram.Amrani@...gic.com>,
        "dledford@...hat.com" <dledford@...hat.com>,
        David Miller <davem@...emloft.net>,
        Ariel Elior <Ariel.Elior@...gic.com>,
        Michal Kalderon <Michal.Kalderon@...gic.com>,
        Rajesh Borundia <rajesh.borundia@...gic.com>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [RFC 02/11] Add RoCE driver framework

On Wed, Sep 14, 2016 at 06:25:38PM +0000, Mintz, Yuval wrote:
> > > > > >> >> +uint debug;
> > > > > >> >> +module_param(debug, uint, 0);
> > > > > > >>> +MODULE_PARM_DESC(debug, "Default debug msglevel");
> > > > > >>
> > > > > >> >Why are you adding this as a module parameter?
> > > > > >>
> > > > > >>  I believe this is mostly to follow same line as qede which also defines
> > > > > > > 'debug' module parameter for allowing easy user control of debug
> > > > > > > prints [& specifically for probe prints, which can't be controlled
> > > > > > > otherwise].
> > > > >
> > > > > > Can you give us an example where dynamic debug and tracing infrastructures
> > > > > > are not enough?
> > > > >
> > > > > > AFAIK, most of these debug module parameters are legacy copy/paste
> > > > > > code which is useless in real life scenarios.
> > > > >
> > > > > Define 'enough'; Using dynamic debug you can provide all the necessary
> > > > > information and at an even better granularity that's achieved by suggested
> > >  > > infrastructure,  but is harder for an end-user to use. Same goes for tracing.
> > > > >
> > > > > The 'debug' option provides an easy grouping for prints related to a specific
> > > > > area in the driver.
> > > >
> > > > It is hard to agree with you that user which knows how-to load modules
> > > > with parameters won't success to enable debug prints.
> > >
> > > I think you're giving too much credit to the end-user. :-D
> > >
> > > > In addition, global increase in debug level for whole driver will create
> > > > printk storm in dmesg and give nothing to debuggability.
> > >
> > > So basically, what you're claiming is that ethtool 'msglvl' setting for devices
> > > is completely obselete. While this *might* be true, we use it extensively
> > > in our qede and qed drivers; The debug module parameter merely provides
> > > a manner of setting the debug value prior to initial probe for all interfaces.
> > > qedr follows the same practice.
>
> > Thanks for this excellent example. Ethtool 'msglvl' adds this
> > dynamically, while your DEBUG argument works for loading module
> > only.
>
> > If you want dynamic prints, you have two options:
> > 1. Add support of ethtool to whole RDMA stack.
> > 2. Use dynamic tracing infrastructure.
>
> > Which option do you prefer?
> Option 3 - continuing this discussion. :-)

Sorry,
I was under impression that you want this driver to be merged, but it
looks like It was incorrect assumption. Let's continue discussion.

>
> Perhaps I misread your intentions - I thought that by dynamic debug
> you meant that all debug in RDMA should be pr_debug() based, and
> therefore my objection regarding the ease with which users can
> configure it.

It is not for all RDMA, but in your proposed driver. You are adding this
"debug" module argument to your module.

> If all you meant was 'dynamically set' as opposed to 'statically set'
> then I agree that having that sort of configurability is preferable
> [Even though end-user would still probably prefer a module
> parameter for reproductions; As the name implies, 'debug' isn't
> meant to be used in other situations].

We are not adding code just for fun, but for a real reason, and
especially interfaces which will be visible to user.

The overall expectation from the driver's authors that they are
submitting driver which doesn't have bringup issues. For real
life scenarios, where the bugs will be reveled after some time of
usage, this global debug is useless.

>
> The other thing to consider are the probe-time prints.
> Problem is, you wouldn't have a control node between probe
> and until after the probing would be over, so it would be a bit
> hard to configure that.
> You can always think of some generic method of fixing that as well
> [sysfs node for the entire system for probe-time prints, perhaps?]

/sys/kernel/debug/tracing
/sys/kernel/debug/dynamic_debug

>
> Do notice you would be harming user-experience of reproductions
> though - as it would have to follow different mechanisms to open
> debug prints of various qed* components.

I don't understand this point at all. Do you think that it is normal to
ask user to debug your driver? Is this called "user-experience"?

And regarding the second point, the old code is not an excuse to
copy/paste bad practices.

As a summary, I didn't see in your responses any real life example where
you will need global debug level for your driver.

Thanks

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists