linux-kernel - Re: [PATCH] sched/cputime: Mitigate performance regression in times()/clock

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] [day] [month] [year] [list]

Message-ID: <1470383892.2094.13.camel@suse.cz>
Date:	Fri, 05 Aug 2016 09:58:12 +0200
From:	Giovanni Gherdovich <ggherdovich@...e.cz>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Ingo Molnar <mingo@...hat.com>,
	Mike Galbraith <mgalbraith@...e.de>,
	Stanislaw Gruszka <sgruszka@...hat.com>,
	linux-kernel@...r.kernel.org, Mel Gorman <mgorman@...e.com>,
	mgorman@...hsingularity.net
Subject: Re: [PATCH] sched/cputime: Mitigate performance regression in
 times()/clock_gettime()

On Wed, 2016-08-03 at 12:02 +0200, Peter Zijlstra wrote:
> On Wed, Aug 03, 2016 at 12:04:52AM +0200, Giovanni Gherdovich wrote:
> 
> > > > +#if defined(CONFIG_FAIR_GROUP_SCHED)
> > > 
> > > This here wants a comment on why we're doing this. Because I'm
> > > sure that if someone were to read this code in a few weeks
> > > they'd go WTF!?
> > 
> > I had that config variable set in the machine I was testing on, and
> > thought that for some reason it was related to my observations. I
> > will repeat the experiment without it, and if I obtain the same
> > results I will drop the conditional. Otherwise I will motivate its
> > necessity.
> > 
> 
> No, I meant we want a comment here explaining the reason for these
> prefetches.
> 
> You'll need the #ifdef because se->cfs_rq doesn't exist otherwise.

Oh! Thanks for pointing that out.

> > 
> > I post below the snippets of generated code with and without CSE
> > that I got running 'disassemble /m task_sched_runtime' in gdb;
> > you'll see they're identical. If you prefer the explicit hint I'll
> > include it in v2, but it's probably safe to say it isn't needed.
> 
> I much prefer the manual CSE, its much more readable.

Ok.

> 
> Also, maybe pull the whole thing into a helper function with a
> descriptive name, like:
> 
> /*
>  * XXX comment on why this is needed goes here...
>  */
> static inline void prefetch_curr_exec_start(struct task_struct *p)
> {
> #ifdef CONFIG_FAIR_GROUP_SCHED
> 	struct sched_entity *curr = (&p->se)->cfs_rq->curr;
> 
> 	prefetch(curr);
> 	prefetch(&curr->exec_start);
> #endif
> }

Ok.

On Wed, 2016-08-03 at 12:34 +0200, Peter Zijlstra wrote:
> On Wed, Aug 03, 2016 at 12:02:54PM +0200, Peter Zijlstra wrote:
> > /*
> >  * XXX comment on why this is needed goes here...
> >  */
> > static inline void prefetch_curr_exec_start(struct task_struct *p)
> > {
> > #ifdef CONFIG_FAIR_GROUP_SCHED
> > 	struct sched_entity *curr = (&p->se)->cfs_rq->curr;
> #else
> 	struct sched_entity *curr = task_rq(p)->cfs_rq->curr;
> #endif
> 
> Would be the 'right' thing for !cgroup stuffs

Ok, thanks. With a bit of help I figured that you actually meant to
write

#else
	struct sched_entity *curr = (&task_rq(p)->cfs)->curr;
#endif

Will add that.

V2 is on its way; I previously said I would put "From: Mike" in the
patch header, but since then I discussed with Mike and we agreed that
the patch will be "From: Giovanni" and Suggested-by: Mike.

Regards,
Giovanni