linux-kernel - Re: [PATCH 1/2] sched/uclamp: Fix initialization of strut uclamp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 19 Jun 2020 19:42:25 +0100
From:   Qais Yousef <qais.yousef@....com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Ingo Molnar <mingo@...hat.com>, Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Patrick Bellasi <patrick.bellasi@...bug.net>,
        Chris Redpath <chrid.redpath@....com>,
        Lukasz Luba <lukasz.luba@....com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] sched/uclamp: Fix initialization of strut uclamp_rq

On 06/19/20 20:13, Peter Zijlstra wrote:
> On Fri, Jun 19, 2020 at 06:39:44PM +0100, Qais Yousef wrote:
> > On 06/19/20 19:30, Peter Zijlstra wrote:
> > > On Thu, Jun 18, 2020 at 08:55:24PM +0100, Qais Yousef wrote:
> > > 
> > > > +	for_each_clamp_id(clamp_id) {
> > > > +		memset(uc_rq[clamp_id].bucket,
> > > > +		       0,
> > > > +		       sizeof(struct uclamp_bucket)*UCLAMP_BUCKETS);
> > > > +
> > > > +		uc_rq[clamp_id].value = uclamp_none(clamp_id);
> > > 
> > > I think you can replace all that with:
> > > 
> > > 		*uc_rq = (struct uclamp_rq){
> > > 			.value = uclamp_none(clamp_id),
> > > 		};
> > > 
> > > it's shorter and is free or weird line-breaks :-)
> > 
> > Sure. I just sent v2 so that people will be encouraged to run tests hopefully.
> > But will fix in v3.
> > 
> > Do we actually need to 0 out anything here? Shouldn't the runqueues all be in
> > BSS which gets initialized to 0 by default at boot?
> > 
> > Maybe better stay explicit..
> 
> C99 named initializer (as used here) explicitly zero initializes all
> unnamed members. Is that explicit enough? ;-)

Hehe yes, but what I meant is that unless

	DEFINE_PER_CPU_SHARED_ALIGNED(struct rq, runqueues);

has some special rules, it should be in BSS and already zeroed out when we do
sched_init(). So do we really need to explicitly zero out anything? It seems
redundant, but again maybe being explicit is more readable,
so maybe better keep it the way it is (named initializer of struct).

Thanks

--
Qais Yousef