Date:   Wed, 14 Sep 2016 18:25:38 +0000
From:   "Mintz, Yuval" <Yuval.Mintz@...ium.com>
To:     Leon Romanovsky <leon@...nel.org>
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

> > > > >> >> +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. :-)

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

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

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.

Powered by blists - more mailing lists