Re: [PATCH v4 2/8] init.h: add __read_mostly to early_param_on_off()

From: "Luis R. Rodriguez" <mcgrof@suse.com>
To: Borislav Petkov <bp@suse.de>, Kyle McMartin <kyle@kernel.org>,
	Christoph Lameter <cl@linux.com>,
	Alok N Kataria <alokk@calsoftinc.com>,
	Shobhit Dayal <shobhit@calsoftinc.com>,
	Shai Fultheim <shai@scalex86.org>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: "Luis R. Rodriguez" <mcgrof@do-not-panic.com>,
	mingo@elte.hu, tglx@linutronix.de, hpa@zytor.com,
	plagnioj@jcrosoft.com, tomi.valkeinen@ti.com,
	daniel.vetter@intel.com, airlied@linux.ie, dledford@redhat.com,
	awalls@md.metrocast.net, syrjala@sci.fi, luto@amacapital.net,
	mst@redhat.com, cocci@systeme.lip6.fr,
	linux-kernel@vger.kernel.org, Juergen Gross <jgross@suse.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Dave Airlie <airlied@redhat.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	x86@kernel.org
Subject: Re: [PATCH v4 2/8] init.h: add __read_mostly to early_param_on_off()
Date: Thu, 30 Apr 2015 20:32:00 +0200	[thread overview]
Message-ID: <20150430183200.GW5622@wotan.suse.de> (raw)
In-Reply-To: <20150430160044.GF5117@pd.tnic>

On Thu, Apr 30, 2015 at 06:00:44PM +0200, Borislav Petkov wrote:
> On Wed, Apr 29, 2015 at 02:44:21PM -0700, Luis R. Rodriguez wrote:
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> So this one is missing a commit message too but we need to talk about
> this. Why are we adding __read_mostly in the macro? This would put every
> param declared this way into __section__(".data..read_mostly") and I'm
> not really convinced that is needed.
> 
> Those setup params get accessed a couple of times only during boot, in
> non-critical paths so we don't really care.
> 
> If you care about pat_enabled being read a lot, you could do:
> 
> int __read_mostly pat_enabled;
> 
> only for it.
> 
> But for every param declared this way? I'm not sure we want that...

I'm glad you bring this up, Christoph can you or anyone else can you provide
advise since you added __read_mostly ?

I added this to early_param_on_off() given I saw a prevalence of __read_mostly
in similar taste elsewhere in the kernel on kernel parameters, and since this
patch tried to convert one kernel parameter which used __read_mostly over to
early_param_on_off() I decided to add it and now we need to really decide if
its ideal for kernel parameters or not.

In this particular case early_param_on_off() will be used for boot time kernel
parameters which we do not intend on dynamically change a lot at run time, we
at run time in early boot have to disable a feature but this will typically
happen only once.

  Luis

> > diff --git a/include/linux/init.h b/include/linux/init.h
> > index 21b6d76..a0385cc 100644
> > --- a/include/linux/init.h
> > +++ b/include/linux/init.h
> > @@ -272,7 +272,7 @@ struct obs_kernel_param {
> >  
> >  #define early_param_on_off(str_on, str_off, var, config)		\
> >  									\
> > -	int var = IS_ENABLED(config);					\
> > +	int __read_mostly var = IS_ENABLED(config);			\
> >  									\
> >  	static int __init parse_##var##_on(char *arg)			\
> >  	{								\