Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1631075 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] sched/fair: Use task_groups instead of leaf_cfs_rq_list  to walk all cfs_rqs Tejun Heo <tj@kernel.org> - 2017-04-26 02:50 +0200
    Re: [PATCH 1/2] sched/fair: Use task_groups instead of  leaf_cfs_rq_list to walk all cfs_rqs Peter Zijlstra <peterz@infradead.org> - 2017-05-01 18:30 +0200
    Re: [PATCH 1/2] sched/fair: Use task_groups instead of  leaf_cfs_rq_list to walk all cfs_rqs Peter Zijlstra <peterz@infradead.org> - 2017-05-01 19:10 +0200
      Re: [PATCH 1/2] sched/fair: Use task_groups instead of  leaf_cfs_rq_list to walk all cfs_rqs Peter Zijlstra <peterz@infradead.org> - 2017-05-01 19:10 +0200
        Re: [PATCH 1/2] sched/fair: Use task_groups instead of  leaf_cfs_rq_list to walk all cfs_rqs Tejun Heo <tj@kernel.org> - 2017-05-01 21:10 +0200
    Re: [PATCH 1/2] sched/fair: Use task_groups instead of  leaf_cfs_rq_list to walk all cfs_rqs Peter Zijlstra <peterz@infradead.org> - 2017-05-04 15:40 +0200
      Re: [PATCH 1/2] sched/fair: Use task_groups instead of  leaf_cfs_rq_list to walk all cfs_rqs Tejun Heo <tj@kernel.org> - 2017-05-04 19:40 +0200

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

Currently, rq->leaf_cfs_rq_list is a traversal ordered list of all
live cfs_rqs which have ever been active on the CPU; unfortunately,
this makes update_blocked_averages() O(total number of CPU cgroups)
which isn't scalable at all.

The next patch will make rq->leaf_cfs_rq_list only contain the cfs_rqs
which are currently active.  In preparation, this patch converts users
which need to traverse all cfs_rqs to use task_groups list instead.

task_groups list is protected by its own lock.  While it allows RCU
protected traversal and the order of operations guarantees that all
online cfs_rqs will be visited, holding rq->lock won't protect against
iterating an already unregistered cfs_rq.  To avoid operating on an
already dead cfs_rq, a new state variable cfs_rq->online is added
which is protected by rq->lock and tracks whether the cfs_rq is
registered.

As clearing of cfs_rq->online should be protected by rq->lock, the
locking avoidance optimization in unregister_fair_sched_group() is
removed.  Given that the optimization is meanginful only when removing
cgroups which are created but never used and the general heaviness of
cgroup removal, the impact is unlikely to be noticeable.

Another change is that print_cfs_stats() would now print cfs_rqs in a
different order.  If this matters, we can improve it later so that it
first prints cfs_rqs on leaf_cfs_rq_list and then follow up with the
rest.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Paul Turner <pjt@google.com>
Cc: Chris Mason <clm@fb.com>
---
Hello,

This is another set of CPU controller performance fix patches and
based on top of -next as of today.

Thanks.

 kernel/sched/fair.c  |   45 +++++++++++++++++++++++++--------------------
 kernel/sched/sched.h |    1 +
 2 files changed, 26 insertions(+), 20 deletions(-)

--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4644,23 +4644,32 @@ static void destroy_cfs_bandwidth(struct
 
 static void __maybe_unused update_runtime_enabled(struct rq *rq)
 {
-	struct cfs_rq *cfs_rq;
+	struct task_group *tg;
 
-	for_each_leaf_cfs_rq(rq, cfs_rq) {
-		struct cfs_bandwidth *cfs_b = &cfs_rq->tg->cfs_bandwidth;
+	rcu_read_lock();
+	list_for_each_entry_rcu(tg, &task_groups, list) {
+		struct cfs_bandwidth *cfs_b = &tg->cfs_bandwidth;
+		struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)];
+
+		if (!cfs_rq->online)
+			continue;
 
 		raw_spin_lock(&cfs_b->lock);
 		cfs_rq->runtime_enabled = cfs_b->quota != RUNTIME_INF;
 		raw_spin_unlock(&cfs_b->lock);
 	}
+	rcu_read_unlock();
 }
 
 static void __maybe_unused unthrottle_offline_cfs_rqs(struct rq *rq)
 {
-	struct cfs_rq *cfs_rq;
+	struct task_group *tg;
 
-	for_each_leaf_cfs_rq(rq, cfs_rq) {
-		if (!cfs_rq->runtime_enabled)
+	rcu_read_lock();
+	list_for_each_entry_rcu(tg, &task_groups, list) {
+		struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)];
+
+		if (!cfs_rq->online || !cfs_rq->runtime_enabled)
 			continue;
 
 		/*
@@ -4677,6 +4686,7 @@ static void __maybe_unused unthrottle_of
 		if (cfs_rq_throttled(cfs_rq))
 			unthrottle_cfs_rq(cfs_rq);
 	}
+	rcu_read_unlock();
 }
 
 #else /* CONFIG_CFS_BANDWIDTH */
@@ -9345,6 +9355,7 @@ void online_fair_sched_group(struct task
 		se = tg->se[i];
 
 		raw_spin_lock_irq(&rq->lock);
+		se->my_q->online = 1;
 		update_rq_clock(rq);
 		attach_entity_cfs_rq(se);
 		sync_throttle(tg, i);
@@ -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);
 	}
 }
@@ -9523,11 +9528,11 @@ const struct sched_class fair_sched_clas
 #ifdef CONFIG_SCHED_DEBUG
 void print_cfs_stats(struct seq_file *m, int cpu)
 {
-	struct cfs_rq *cfs_rq;
+	struct task_group *tg;
 
 	rcu_read_lock();
-	for_each_leaf_cfs_rq(cpu_rq(cpu), cfs_rq)
-		print_cfs_rq(m, cpu, cfs_rq);
+	list_for_each_entry_rcu(tg, &task_groups, list)
+		print_cfs_rq(m, cpu, tg->cfs_rq[cpu]);
 	rcu_read_unlock();
 }
 
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -448,6 +448,7 @@ struct cfs_rq {
 
 #ifdef CONFIG_FAIR_GROUP_SCHED
 	struct rq *rq;	/* cpu runqueue to which this cfs_rq is attached */
+	int online;	/* online state, protected by rq->lock */
 
 	/*
 	 * leaf cfs_rqs are those that hold tasks (lowest schedulable entity in

[toc] | [next] | [standalone]

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

On Tue, Apr 25, 2017 at 05:40:39PM -0700, Tejun Heo wrote:

> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4644,23 +4644,32 @@ static void destroy_cfs_bandwidth(struct
>  
>  static void __maybe_unused update_runtime_enabled(struct rq *rq)
>  {
> -	struct cfs_rq *cfs_rq;
> +	struct task_group *tg;
>  
> -	for_each_leaf_cfs_rq(rq, cfs_rq) {
> -		struct cfs_bandwidth *cfs_b = &cfs_rq->tg->cfs_bandwidth;
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(tg, &task_groups, list) {
> +		struct cfs_bandwidth *cfs_b = &tg->cfs_bandwidth;
> +		struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)];
> +
> +		if (!cfs_rq->online)
> +			continue;
>  
>  		raw_spin_lock(&cfs_b->lock);
>  		cfs_rq->runtime_enabled = cfs_b->quota != RUNTIME_INF;
>  		raw_spin_unlock(&cfs_b->lock);
>  	}
> +	rcu_read_unlock();
>  }
>  
>  static void __maybe_unused unthrottle_offline_cfs_rqs(struct rq *rq)
>  {
> -	struct cfs_rq *cfs_rq;
> +	struct task_group *tg;
>  
> -	for_each_leaf_cfs_rq(rq, cfs_rq) {
> -		if (!cfs_rq->runtime_enabled)
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(tg, &task_groups, list) {
> +		struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)];
> +
> +		if (!cfs_rq->online || !cfs_rq->runtime_enabled)
>  			continue;
>  
>  		/*
> @@ -4677,6 +4686,7 @@ static void __maybe_unused unthrottle_of
>  		if (cfs_rq_throttled(cfs_rq))
>  			unthrottle_cfs_rq(cfs_rq);
>  	}
> +	rcu_read_unlock();
>  }

Note that both these are called with rq->lock held. I don't think we
need to actually add rcu_read_lock() here, as it will fully serialize
against your ->online = 0 that holds rq->lock.

Also, arguably you can keep using for_each_leaf_cfs_rq() for unthrottle,
because I don't think we can (should?) remove a throttled group from the
leaf list -- its not empty after all.

Then again, this is CPU hotplug code and nobody cares about its
performance much, and its better to be safe than sorry, so yes, use
task_groups list to make sure to reach all groups.

[toc] | [prev] | [next] | [standalone]

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

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.


> @@ -9523,11 +9528,11 @@ const struct sched_class fair_sched_clas
>  #ifdef CONFIG_SCHED_DEBUG
>  void print_cfs_stats(struct seq_file *m, int cpu)
>  {
> -	struct cfs_rq *cfs_rq;
> +	struct task_group *tg;
>  
>  	rcu_read_lock();
> -	for_each_leaf_cfs_rq(cpu_rq(cpu), cfs_rq)
> -		print_cfs_rq(m, cpu, cfs_rq);
> +	list_for_each_entry_rcu(tg, &task_groups, list)
> +		print_cfs_rq(m, cpu, tg->cfs_rq[cpu]);
>  	rcu_read_unlock();
>  }

If you have a gazillion empty groups, do we really want to print them?



diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h
index 74d9c3a1feee..8bad2c371b18 100644
--- a/include/linux/rcutiny.h
+++ b/include/linux/rcutiny.h
@@ -58,6 +58,11 @@ static inline void cond_synchronize_sched(unsigned long oldstate)
 	might_sleep();
 }
 
+static bool void should_synchornize_sched(unsigned long oldstate)
+{
+	return true;
+}
+
 extern void rcu_barrier_bh(void);
 extern void rcu_barrier_sched(void);
 
diff --git a/include/linux/rcutree.h b/include/linux/rcutree.h
index 0bacb6b2af69..9918b632db27 100644
--- a/include/linux/rcutree.h
+++ b/include/linux/rcutree.h
@@ -78,6 +78,7 @@ unsigned long get_state_synchronize_rcu(void);
 void cond_synchronize_rcu(unsigned long oldstate);
 unsigned long get_state_synchronize_sched(void);
 void cond_synchronize_sched(unsigned long oldstate);
+bool should_synchronize_sched(unsigned long oldstate);
 
 extern unsigned long rcutorture_testseq;
 extern unsigned long rcutorture_vernum;
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 91fff49d5869..59eb0d115217 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -3423,6 +3423,18 @@ void cond_synchronize_sched(unsigned long oldstate)
 }
 EXPORT_SYMBOL_GPL(cond_synchronize_sched);
 
+bool should_synchronize_sched(unsigned long oldstate)
+{
+	unsigned long newstate;
+
+	/*
+	 * Ensure that this load happens before any RCU-destructive
+	 * actions the caller might carry out after we return.
+	 */
+	newstate = smp_load_acquire(&rcu_sched_state.completed);
+	return ULONG_CMP_GE(oldstate, newstate);
+}
+
 /*
  * Check to see if there is any immediate RCU-related work to be done
  * by the current CPU, for the specified type of RCU, returning 1 if so.
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index ecad8e2d2a29..d7630b34d197 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6362,7 +6362,7 @@ void ia64_set_curr_task(int cpu, struct task_struct *p)
 /* task_group_lock serializes the addition/removal of task groups */
 static DEFINE_SPINLOCK(task_group_lock);
 
-static void sched_free_group(struct task_group *tg)
+static void __sched_free_group(struct task_group *tg)
 {
 	free_fair_sched_group(tg);
 	free_rt_sched_group(tg);
@@ -6370,6 +6370,22 @@ static void sched_free_group(struct task_group *tg)
 	kmem_cache_free(task_group_cache, tg);
 }
 
+static void sched_free_group_rcu_sched(struct rcu_head *rhp)
+{
+	/* Now it should be safe to free those cfs_rqs: */
+	__sched_free_group(container_of(rhp, struct task_group, rcu));
+}
+
+static void sched_free_group(struct task_group *tg)
+{
+	if (should_synchronize_sched(tg->rcu_stamp)) {
+		call_rcu_sched(&tg->rcu, sched_free_group_rcu_sched);
+		return;
+	}
+
+	__sched_free_group(tg);
+}
+
 /* allocate runqueue etc for a new task group */
 struct task_group *sched_create_group(struct task_group *parent)
 {
@@ -6434,6 +6450,8 @@ void sched_offline_group(struct task_group *tg)
 	list_del_rcu(&tg->list);
 	list_del_rcu(&tg->siblings);
 	spin_unlock_irqrestore(&task_group_lock, flags);
+
+	tg->rcu_stamp = get_state_synchronize_sched();
 }
 
 static void sched_change_group(struct task_struct *tsk, int type)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b2e5e9e64c86..1dbab563e798 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -302,6 +302,7 @@ struct task_group {
 #endif
 
 	struct rcu_head rcu;
+	long rcu_stamp;
 	struct list_head list;
 
 	struct task_group *parent;

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

Hello,

On Mon, May 01, 2017 at 07:02:41PM +0200, Peter Zijlstra wrote:
> 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

So, the rcu_read_lock() thing protects against sched_free_group() and
thanks to the order of operations, all online cfs_rq's are guaranteed
to be visbile in the two callbacks; however, nothing prevents the code
paths from seeing already dead cfs_rqs, which *may* be okay if the
code paths are safe to run on dead and unlinked cfs_rqs, but it's
still nasty and fragile.

The new ->online condition which is synchronized by rq->lock
guarantees that both functions only process live ones.  We need
something synchronized by rq->lock to guarantee this whether that's a
new list entry or a flag like ->online.  It'd be better to encapsulate
and document this iteration.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

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

On Tue, Apr 25, 2017 at 05:40:39PM -0700, Tejun Heo wrote:

> task_groups list is protected by its own lock.  While it allows RCU
> protected traversal and the order of operations guarantees that all
> online cfs_rqs will be visited, holding rq->lock won't protect against
> iterating an already unregistered cfs_rq.

So I think the below is sufficient.

Yes we can hit an (almost) dead cfs_rq, but poking the bandwidth
variables thereof is harmless.

unthrottle_cfs_rq() also stops doing anything much when it finds the
cfs_rq is empty, which must be the case if we're removing it.

I don't know Paul's opinion on RCU GPs happening while stop_machine(),
but just in case he feels that's fair game, I did add the
rcu_read_lock() thingies.

The lockdep assert is mostly documentation, to more easily see
it is indeed held when we get there.

I left print_cfs_stats using the leaf list, no point in printing stuff
that's empty.

This way we can avoid taking all RQ locks on cgroup destruction.

---
 kernel/sched/fair.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index cd6c3f957d44..c897ad6d714c 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4644,22 +4644,32 @@ static void destroy_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
 
 static void __maybe_unused update_runtime_enabled(struct rq *rq)
 {
-	struct cfs_rq *cfs_rq;
+	struct task_group *tg;
 
-	for_each_leaf_cfs_rq(rq, cfs_rq) {
-		struct cfs_bandwidth *cfs_b = &cfs_rq->tg->cfs_bandwidth;
+	lockdep_assert_held(&rq->lock);
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(tg, &task_groups, list) {
+		struct cfs_bandwidth *cfs_b = &tg->cfs_bandwidth;
+		struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)];
 
 		raw_spin_lock(&cfs_b->lock);
 		cfs_rq->runtime_enabled = cfs_b->quota != RUNTIME_INF;
 		raw_spin_unlock(&cfs_b->lock);
 	}
+	rcu_read_unlock();
 }
 
 static void __maybe_unused unthrottle_offline_cfs_rqs(struct rq *rq)
 {
-	struct cfs_rq *cfs_rq;
+	struct task_group *tg;
+
+	lockdep_assert_held(&rq->lock);
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(tg, &task_groups, list) {
+		struct cfs_rq *cfs_rq = tg->cfs_rq[cpu_of(rq)];
 
-	for_each_leaf_cfs_rq(rq, cfs_rq) {
 		if (!cfs_rq->runtime_enabled)
 			continue;
 
@@ -4677,6 +4687,7 @@ static void __maybe_unused unthrottle_offline_cfs_rqs(struct rq *rq)
 		if (cfs_rq_throttled(cfs_rq))
 			unthrottle_cfs_rq(cfs_rq);
 	}
+	rcu_read_unlock();
 }
 
 #else /* CONFIG_CFS_BANDWIDTH */

[toc] | [prev] | [next] | [standalone]

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

Hello, Peter.

On Thu, May 04, 2017 at 03:31:22PM +0200, Peter Zijlstra wrote:
> Yes we can hit an (almost) dead cfs_rq, but poking the bandwidth
> variables thereof is harmless.
> 
> unthrottle_cfs_rq() also stops doing anything much when it finds the
> cfs_rq is empty, which must be the case if we're removing it.

Yeah, if you're okay with calling the functions on dead cfs_rq's, just
wrapping with rcu_read_lock should be enough.

> I don't know Paul's opinion on RCU GPs happening while stop_machine(),
> but just in case he feels that's fair game, I did add the
> rcu_read_lock() thingies.
> 
> The lockdep assert is mostly documentation, to more easily see
> it is indeed held when we get there.
> 
> I left print_cfs_stats using the leaf list, no point in printing stuff
> that's empty.
> 
> This way we can avoid taking all RQ locks on cgroup destruction.

Looks good to me.

Thanks.

-- 
tejun

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web