Re: [RFC PATCH 4/5] sched/events: Introduce sched_entity load tracking trace event

[Dietmar Eggemann]

On 03/28/2017 10:08 AM, Peter Zijlstra wrote:
> On Tue, Mar 28, 2017 at 07:35:40AM +0100, Dietmar Eggemann wrote:
> > diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
> > index 51db8a90e45f..647cfaf528fd 100644
> > --- a/include/trace/events/sched.h
> > +++ b/include/trace/events/sched.h
> > @@ -566,14 +566,15 @@ TRACE_EVENT(sched_wake_idle_without_ipi,
> >  #ifdef CONFIG_SMP
> >  #ifdef CREATE_TRACE_POINTS
> >  static inline
> > -int __trace_sched_cpu(struct cfs_rq *cfs_rq)
> > +int __trace_sched_cpu(struct cfs_rq *cfs_rq, struct sched_entity *se)
> >  {
> >  #ifdef CONFIG_FAIR_GROUP_SCHED
> > -	struct rq *rq = cfs_rq->rq;
> > +	struct rq *rq = cfs_rq ? cfs_rq->rq : NULL;
> >  #else
> > -	struct rq *rq = container_of(cfs_rq, struct rq, cfs);
> > +	struct rq *rq = cfs_rq ? container_of(cfs_rq, struct rq, cfs) : NULL;
> >  #endif
> > -	return cpu_of(rq);
> > +	return rq ? cpu_of(rq)
> > +		  : task_cpu((container_of(se, struct task_struct, se)));
> >  }
>
> So here you duplicate lots of FAIR_GROUP internals. So why do you then
> have to expose group_cfs_rq() in the previous patch?

Not having group_cfs_rq() available made the trace event code too 
confusing.

But like I mentioned in the second to last paragraph in the cover 
letter, having all necessary cfs accessor-functions (rq_of(), task_of(), 
etc.) available would definitely streamline the coding effort of these 
trace events.

Do you think that making them public in include/linux/sched.h is the way 
to go? What about the namespace issue with other sched classes? Should 
they be exported with the name they have right now (since cfs was there 
first) or should they be renamed to cfs_task_of() and rq_of_cfs_rq() etc. ?

RT and Deadline class already have the own (private) accessor-functions 
(e.g. dl_task_of() or rq_of_dl_rq()).