Re: [RFC 02/11] Add RoCE driver framework

From: Leon Romanovsky <leon@kernel.org>
To: "Mintz, Yuval" <Yuval.Mintz@cavium.com>
Cc: Yuval Mintz <Yuval.Mintz@qlogic.com>,
	Mark Bloch <markb@mellanox.com>,
	Ram Amrani <Ram.Amrani@qlogic.com>,
	"dledford@redhat.com" <dledford@redhat.com>,
	David Miller <davem@davemloft.net>,
	Ariel Elior <Ariel.Elior@qlogic.com>,
	Michal Kalderon <Michal.Kalderon@qlogic.com>,
	Rajesh Borundia <rajesh.borundia@qlogic.com>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>,
	netdev <netdev@vger.kernel.org>
Subject: Re: [RFC 02/11] Add RoCE driver framework
Date: Wed, 14 Sep 2016 16:00:32 +0300	[thread overview]
Message-ID: <20160914130032.GB26069@leon.nu> (raw)
In-Reply-To: <BL2PR07MB2306533A710B9EC40F56E8678DF10@BL2PR07MB2306.namprd07.prod.outlook.com>

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

On Wed, Sep 14, 2016 at 08:15:23AM +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?

>

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