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
