Re: [PATCH] sched/cputime: Mitigate performance regression in times()/clock_gettime()

[Giovanni Gherdovich]

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