Re: [PATCH 1/2] sched/fair: Use task_groups instead of leaf_cfs_rq_list to walk all cfs_rqs

[Peter Zijlstra]

On Mon, May 01, 2017 at 06:59:39PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 25, 2017 at 05:40:39PM -0700, Tejun Heo wrote:
> > @@ -9355,24 +9366,18 @@ void online_fair_sched_group(struct task
> >  void unregister_fair_sched_group(struct task_group *tg)
> >  {
> >  	unsigned long flags;
> > -	struct rq *rq;
> >  	int cpu;
> >  
> >  	for_each_possible_cpu(cpu) {
> > +		struct rq *rq = cpu_rq(cpu);
> > +		struct cfs_rq *cfs_rq = tg->cfs_rq[cpu];
> > +
> >  		if (tg->se[cpu])
> >  			remove_entity_load_avg(tg->se[cpu]);
> >  
> > -		/*
> > -		 * Only empty task groups can be destroyed; so we can speculatively
> > -		 * check on_list without danger of it being re-added.
> > -		 */
> > -		if (!tg->cfs_rq[cpu]->on_list)
> > -			continue;
> > -
> > -		rq = cpu_rq(cpu);
> > -
> >  		raw_spin_lock_irqsave(&rq->lock, flags);
> > -		list_del_leaf_cfs_rq(tg->cfs_rq[cpu]);
> > +		list_del_leaf_cfs_rq(cfs_rq);
> > +		cfs_rq->online = 0;
> >  		raw_spin_unlock_irqrestore(&rq->lock, flags);
> >  	}
> >  }
> 
> It would be nice to be able to retain that on_list check and avoid
> taking all those rq->lock's.
> 
> Since per the previous mail, those online/offline loops are protected by
> rq->lock, all we need is to ensure an rcu_sched GP passes between here
> and sched_free_group().
> 
> Which I think we can do differently, see below. Then we don't need the
> ->online thing and can keep using the ->on_list check.

n/m, I need to stop staring at a screen. Wrapping those two sites in
rcu_read_lock() achieves the very same.

So we want the rcu_read_lock() to serialize against sched_free_group,
but don't need the new ->online thing and can retain the ->on_list
stuff. Or I've completely lost the plot (which is entirely possible...)

I'll stare at this again tomorrow