CFS Bandwidth Control V5

In this patch we introduce the notion of CFS bandwidth, partitioned into globally unassigned bandwidth, and locally claimed bandwidth.

- The global bandwidth is per task_group, it represents a pool of unclaimed bandwidth that cfs_rqs can allocate from.
- The local bandwidth is tracked per-cfs_rq, this represents allotments from the global pool bandwidth assigned to a specific cpu.

Bandwidth is managed via cgroupfs, adding two new interfaces to the cpu subsystem:
- cpu.cfs_period_us : the bandwidth period in usecs
- cpu.cfs_quota_us : the cpu bandwidth (in usecs) that this tg will be allowed to consume over period above.

A per-cfs_bandwidth timer is also introduced to handle future refresh at period expiration. There's some minor refactoring here so that start_bandwidth_timer() functionality can be shared

Signed-off-by Paul Turner <[email protected]>

Signed-off-by Nikhil Rao <[email protected]>

Signed-off-by Bharata B Rao <[email protected]>

---

+const u64 max_cfs_quota_period = 5 * NSEC_PER_SEC; /* 5s */

Add constraints validation for CFS bandwidth hierachies.

It is checked that:
sum(child bandwidth) <= parent_bandwidth

In a quota limited hierarchy, an unconstrainted entity (e.g. bandwidth==RUNTIME_INF) inherits the bandwidth of its parent.

Since bandwidth periods may be non-uniform we normalize to the maximum allowed period, 5 seconds.

This behavior may be disabled (allowing child bandwidth to exceed parent) via kernel.sched_cfs_bandwidth_consistent=0

Signed-off-by Paul Turner <[email protected]>

---
 include/linux/sched.h |    8 +++
 kernel/sched.c        |  127 +++++++++++++++++++++++++++++++++++++++++++++++---
 kernel/sched_fair.c   |    8 +++
 kernel/sysctl.c       |   11 ++++
 4 files changed, 147 insertions(+), 7 deletions(-)

Index: tip/kernel/sched.c
===================================================================

kernel/sched.c +19 -0

+static int tg_cfs_schedulable_down(struct task_group *tg, void *data)
+{
+	struct cfs_schedulable_data *d = data;
+	struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(tg);
+	s64 quota = 0, parent_quota = -1;
+
+	quota = normalize_cfs_quota(tg, d);
+	if (!tg->parent) {
+		quota = RUNTIME_INF;
+	} else {
+		struct cfs_bandwidth *parent_b = tg_cfs_bandwidth(tg->parent);
+
+		parent_quota = parent_b->hierarchal_quota;
+		if (parent_quota != RUNTIME_INF) {
+			parent_quota -= quota;
+			/* invalid hierarchy, child bandwidth exceeds parent */
+			if (parent_quota < 0)
+				return -EINVAL;
+		}

kernel/sched.c +7 -0

+
+static int __cfs_schedulable(struct task_group *tg, u64 period, u64 quota)
+{
+	int ret;
+	struct cfs_schedulable_data data = {
+		.tg = tg,
+		.period = period / NSEC_PER_USEC,

kernel/sched.c +3 -0

+
+	return ret;
+}

Quoted 52 lines · Paul Turner

Add constraints validation for CFS bandwidth hierachies.

It is checked that:
   sum(child bandwidth) <= parent_bandwidth

In a quota limited hierarchy, an unconstrainted entity (e.g. bandwidth==RUNTIME_INF) inherits the bandwidth of its parent.

Since bandwidth periods may be non-uniform we normalize to the maximum allowed period, 5 seconds.

This behavior may be disabled (allowing child bandwidth to exceed parent) via kernel.sched_cfs_bandwidth_consistent=0

Signed-off-by Paul Turner <[email protected]>

---
 include/linux/sched.h |    8 +++
 kernel/sched.c        |  127 +++++++++++++++++++++++++++++++++++++++++++++++--- kernel/sched_fair.c   |    8 +++
 kernel/sysctl.c       |   11 ++++

 4 files changed, 147 insertions(+), 7 deletions(-)

Index: tip/kernel/sched.c
===================================================================

kernel/sched.c +25 -0

+static int tg_cfs_schedulable_down(struct task_group *tg, void *data)
+{
+     struct cfs_schedulable_data *d = data;
+     struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(tg);
+     s64 quota = 0, parent_quota = -1;
+
+     quota = normalize_cfs_quota(tg, d);
+     if (!tg->parent) {
+             quota = RUNTIME_INF;
+     } else {
+             struct cfs_bandwidth *parent_b = tg_cfs_bandwidth(tg->parent);
+
+             parent_quota = parent_b->hierarchal_quota;
+             if (parent_quota != RUNTIME_INF) {
+                     parent_quota -= quota;
+                     /* invalid hierarchy, child bandwidth exceeds parent */
+                     if (parent_quota < 0)
+                             return -EINVAL;
+             }
+
+             /* if no inherent limit then inherit parent quota */
+             if (quota == RUNTIME_INF)
+                     quota = parent_quota;
+             parent_b->hierarchal_quota = parent_quota;
+     }

kernel/sched.c static int cpu_cfs_period_write_u64(stru · line 9414

Quoted 4 lines

+     cfs_b->hierarchal_quota = quota;

kernel/sched.c +3 -0

+
+     return 0;
+}

I find this logic pretty weird.
As long as quota == INF i can overcommit, but as soon as there is some quota, i can not ?

Its clear, that one needs to be able to overcommit runtime, or the default runtime for a new cgroup would need to be 0.

The root problem imo is that runtime and shares should not be in the same cgroup subsystem. The semantics are too different.

Quoted 13 lines

kernel/sched.c +13 -0

+
+static int __cfs_schedulable(struct task_group *tg, u64 period, u64 quota)
+{
+     int ret;
+     struct cfs_schedulable_data data = {
+             .tg = tg,
+             .period = period / NSEC_PER_USEC,
+             .quota = quota / NSEC_PER_USEC,
+     };
+
+     if (!sysctl_sched_cfs_bandwidth_consistent)
+             return 0;
+

Quoted 4 lines

+     rcu_read_lock();
+     ret = walk_tg_tree(tg_cfs_schedulable_down, tg_nop,
+                         &data);
+     rcu_read_unlock();

walk_tg_tree does the rcu_read_lock itself.
not necessary to do that here.
look at __rt_schedulable there is no rcu.

kernel/sched.c static int cpu_cfs_period_write_u64(stru · line 9434

Quoted 4 lines

kernel/sched.c +3 -0

+
+     return ret;
+}

--
torben Hohn

Add constraints validation for CFS bandwidth hierachies.

kernel/sched.c +11 -0

+static u64 normalize_cfs_quota(struct task_group *tg,
+		               struct cfs_schedulable_data *d)
+{
+	u64 quota, period;
+	struct load_weight lw;
+
+	if (tg == d->tg) {
+		period = d->period;
+		quota = d->quota;
+	} else {
+		period = tg_get_cfs_period(tg);

Quoted 17 lines · Paul Turner

Add constraints validation for CFS bandwidth hierachies.

kernel/sched.c +15 -0

+static u64 normalize_cfs_quota(struct task_group *tg,
+		               struct cfs_schedulable_data *d)
+{
+	u64 quota, period;
+	struct load_weight lw;
+
+	if (tg == d->tg) {
+		period = d->period;
+		quota = d->quota;
+	} else {
+		period = tg_get_cfs_period(tg);
+		quota = tg_get_cfs_quota(tg);
+	}
+
+	if (quota == RUNTIME_INF)

kernel/sched.c static int cpu_cfs_period_write_u64(stru · line 9383

Quoted 6 lines

kernel/sched.c +6 -0

+		return RUNTIME_INF;
+
+	lw.weight = period;
+	lw.inv_weight = 0;
+
+	return calc_delta_mine(quota, max_cfs_quota_period, &lw) - 1;

Time to rename calc_delta_mine to something more meaningful ?

(quoted diff) +13 -0

@@ -9339,6 +9350,108 @@ static int cpu_cfs_period_write_u64(stru
 	return tg_set_cfs_period(cgroup_tg(cgrp), cfs_period_us);
 }
 
+
+struct cfs_schedulable_data {
+	struct task_group *tg;
+	u64 period, quota;
+};
+
+/*
+ * normalize group quota/period to be quota/max_period
+ * note: units are usecs
+ */
+static u64 normalize_cfs_quota(struct task_group *tg,
+		               struct cfs_schedulable_data *d)
+{

Review static int tg_set_cfs_bandwidth(struct t · line 9260

Quoted 8 lines · Paul Turner

(quoted diff) +3 -1

@@ -9251,7 +9255,13 @@ static int tg_set_cfs_bandwidth(struct t
       if (period > max_cfs_quota_period)
               return -EINVAL;
 
-     mutex_lock(&mutex);
+     mutex_lock(&cfs_constraints_mutex);
+     if (sysctl_sched_cfs_bandwidth_consistent) {
+             ret = __cfs_schedulable(tg, period, quota);

At this point:
 period => scale in ns unit
 quota  => scale in ns unit, or RUNTIME_INF

And both are unsigned. But...

Quoted 18 lines

(quoted diff) +13 -0

@@ -9339,6 +9350,108 @@ static int cpu_cfs_period_write_u64(stru
       return tg_set_cfs_period(cgroup_tg(cgrp), cfs_period_us);
  }
 
+
+struct cfs_schedulable_data {
+     struct task_group *tg;
+     u64 period, quota;
+};
+
+/*
+ * normalize group quota/period to be quota/max_period
+ * note: units are usecs
+ */
+static u64 normalize_cfs_quota(struct task_group *tg,
+                            struct cfs_schedulable_data *d)
+{

Review static int cpu_cfs_period_write_u64(stru · line 9375

Quoted 10 lines

(quoted diff) +10 -0

+     u64 quota, period;
+     struct load_weight lw;
+
+     if (tg == d->tg) {
+             period = d->period;
+             quota = d->quota;
+     } else {
+             period = tg_get_cfs_period(tg);
+             quota = tg_get_cfs_quota(tg);
+     }

... at this point:
 period => scale in us unit
 quota  => scale in us unit, or -1
Moreover:
 d->period => (scale in ns unit) / NSEC_PER_USEC d->quota  => (scale in ns unit, or RUNTIME_INF) / NSEC_PER_USEC

Therefore, ...

Review static int cpu_cfs_period_write_u64(stru · line 9378

Quoted 3 lines

(quoted diff) +3 -0

+
+     if (quota == RUNTIME_INF)
+             return RUNTIME_INF;

This check doesn't work properly.

I found this problem because I could not get child group back to be unconstrained:

[root@localhost group0]# cat cpu.cfs_*
500000
500000
[root@localhost group0]# cat sub0/cpu.cfs_* 500000
100000
[root@localhost group0]# cat sub1/cpu.cfs_* 500000
100000
[root@localhost group0]# echo -1 > sub1/cpu.cfs_quota_us
bash: echo: write error: Invalid argument

I confirmed that this write error is removed by the following change.  I'm looking forward to seeing your V6 soon.

Reviewed-by Hidetoshi Seto <[email protected]>

Thanks,
H.Seto

---
 kernel/sched.c |    8 ++++----

 1 files changed, 4 insertions(+), 4 deletions(-)

kernel/sched.c +4 -5

index 6d764b5..c8f9820 100644
@@ -9467,8 +9467,8 @@ static u64 normalize_cfs_quota(struct task_group *tg,
                period = d->period;
                quota = d->quota;
        } else {
-               period = tg_get_cfs_period(tg);
-               quota = tg_get_cfs_quota(tg);
+               period = ktime_to_ns(tg_cfs_bandwidth(tg)->period);
+               quota = tg_cfs_bandwidth(tg)->quota;
        }
 
        if (quota == RUNTIME_INF)

@@ -9515,8 +9515,8 @@ static int __cfs_schedulable(struct task_group *tg, u64 period, u64 quota)
        int ret;
        struct cfs_schedulable_data data = {
                .tg = tg,
-               .period = period / NSEC_PER_USEC,
-               .quota = quota / NSEC_PER_USEC,
+               .period = period,
+               .quota = quota,
        };
 
        if (!sysctl_sched_cfs_bandwidth_consistent)
--

1.7.4

kernel/sched_fair.c +6 -0

+static void request_cfs_rq_quota(struct cfs_rq *cfs_rq)
+{
+       struct task_group *tg = cfs_rq->tg;
+       struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(tg);
+       u64 amount = 0, min_amount;
+

kernel/sched_fair.c +19 -0

+               raw_spin_unlock(&cfs_b->lock);
+       }
+
+       cfs_rq->quota_remaining += amount;
+}
+
+static void account_cfs_rq_quota(struct cfs_rq *cfs_rq,
+               unsigned long delta_exec)
+{
+       if (!cfs_rq->quota_enabled)
+               return;
+
+       cfs_rq->quota_remaining -= delta_exec;
+
+       if (cfs_rq->quota_remaining > 0)
+               return;
+
+       request_cfs_rq_quota(cfs_rq);
+} 

Quoted 13 lines · Paul Turner

kernel/sched_fair.c +13 -0

+static void request_cfs_rq_quota(struct cfs_rq *cfs_rq)
+{
+       struct task_group *tg = cfs_rq->tg;
+       struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(tg);
+       u64 amount = 0, min_amount;
+
+       min_amount = sched_cfs_bandwidth_slice() + (-cfs_rq->quota_remaining);
+
+       if (cfs_b->runtime > 0 || cfs_b->quota == RUNTIME_INF) {
+               raw_spin_lock(&cfs_b->lock);
+               if (cfs_b->quota != RUNTIME_INF) {
+                       amount = min(cfs_b->runtime, min_amount);
+                       cfs_b->runtime -= amount;

Quoted 3 lines

+               } else {
+                       amount = min_amount;
+               }

So why would quota be RUNTIME_INF and quota_enabled be true? If its due to a race when fiddling with the cgroup filesystem setting things up that else branch wants a comment, if its for another reason all together there's also a comment missing somewhere ;-)

Quoted 19 lines

+               raw_spin_unlock(&cfs_b->lock);
+       }

kernel/sched_fair.c +17 -0

+
+       cfs_rq->quota_remaining += amount;
+}
+
+static void account_cfs_rq_quota(struct cfs_rq *cfs_rq,
+               unsigned long delta_exec)
+{
+       if (!cfs_rq->quota_enabled)
+               return;
+
+       cfs_rq->quota_remaining -= delta_exec;
+
+       if (cfs_rq->quota_remaining > 0)
+               return;
+
+       request_cfs_rq_quota(cfs_rq);
+}

+ min_amount = sched_cfs_bandwidth_slice() + (-cfs_rq->quota_remaining);

Quoted 1 line · Paul Turner

+       min_amount = sched_cfs_bandwidth_slice() + (-cfs_rq->quota_remaining);

funny way of writing that ;-)

kernel/sched_fair.c static inline u64 sched_cfs_bandwidth_sl · line 1344

Quoted 6 lines · Paul Turner

kernel/sched_fair.c +6 -0

+static void check_cfs_rq_quota(struct cfs_rq *cfs_rq)
+{
+     if (within_bandwidth(cfs_rq))
+             return;
+
+

Nit:  It'd be nice if classes agreed on naming convention to ease rummaging.  In rt, it's bandwidth for bean counting parameters, but the beans are runtime.  within_bandwidth() vs sched_rt_runtime_exceeded() kinda pokes me in the eye when I look at the total.  Seems to me it should be uniformly either quota or bandwidth, and uniformly runtime.

       -Mike

In account_cfs_rq_quota() (via update_curr()) we track consumption versus a cfs_rqs locally assigned quota and whether there is global quota available to provide a refill when it runs out.

In the case that there is no quota remaining it's necessary to throttle so that execution ceases until the susbequent period. While it is at this boundary that we detect (and signal for, via reshed_task) that a throttle is required, the actual operation is deferred until put_prev_entity().

At this point the cfs_rq is marked as throttled and not re-enqueued, this avoids potential interactions with throttled runqueues in the event that we are not immediately able to evict the running task.

Signed-off-by Paul Turner <[email protected]>

Signed-off-by Nikhil Rao <[email protected]>

Signed-off-by Bharata B Rao <[email protected]>

---
 kernel/sched.c      |    2 
 kernel/sched_fair.c |  117 +++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 113 insertions(+), 6 deletions(-)

Index: tip/kernel/sched.c
===================================================================

kernel/sched.c +1 -1

@@ -386,7 +386,7 @@ struct cfs_rq {
 	unsigned long load_contribution;
 #endif
 #ifdef CONFIG_CFS_BANDWIDTH
-	int quota_enabled;
+	int quota_enabled, throttled;
 	s64 quota_remaining;
 #endif
 #endif

Index: tip/kernel/sched_fair.c
===================================================================

kernel/sched_fair.c +66 -3

@@ -321,9 +321,6 @@ find_matching_se(struct sched_entity **s
 
 #endif	/* CONFIG_FAIR_GROUP_SCHED */
 
-static void account_cfs_rq_quota(struct cfs_rq *cfs_rq,
-		                 unsigned long delta_exec);
-
 /**************************************************************
  * Scheduling class tree data structure manipulation methods:
  */

@@ -588,6 +585,9 @@ __update_curr(struct cfs_rq *cfs_rq, str
 #endif
 }
 
+static void account_cfs_rq_quota(struct cfs_rq *cfs_rq,
+		unsigned long delta_exec);
+
 static void update_curr(struct cfs_rq *cfs_rq)
 {
 	struct sched_entity *curr = cfs_rq->curr;

@@ -1221,6 +1221,9 @@ static struct sched_entity *pick_next_en
 	return se;
 }
 
+static void throttle_cfs_rq(struct cfs_rq *cfs_rq);
+static inline int within_bandwidth(struct cfs_rq *cfs_rq);
+
 static void put_prev_entity(struct cfs_rq *cfs_rq, struct sched_entity *prev)
 {
 	/*

@@ -1230,6 +1233,9 @@ static void put_prev_entity(struct cfs_r
 	if (prev->on_rq)
 		update_curr(cfs_rq);
 
+	if (!within_bandwidth(cfs_rq))
+		throttle_cfs_rq(cfs_rq);
+
 	check_spread(cfs_rq, prev);
 	if (prev->on_rq) {
 		update_stats_wait_start(cfs_rq, prev);

@@ -1241,6 +1247,8 @@ static void put_prev_entity(struct cfs_r
 	cfs_rq->curr = NULL;
 }
 
+static void check_cfs_rq_quota(struct cfs_rq *cfs_rq);
+
 static void
 entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
 {

@@ -1249,6 +1257,9 @@ entity_tick(struct cfs_rq *cfs_rq, struc
 	 */
 	update_curr(cfs_rq);
 
+	/* check that entity's usage is still within quota (if enabled) */
+	check_cfs_rq_quota(cfs_rq);
+
 	/*
 	 * Update share accounting for long-running entities.
 	 */

@@ -1294,6 +1305,46 @@ static inline u64 sched_cfs_bandwidth_sl
        return (u64)sysctl_sched_cfs_bandwidth_slice * NSEC_PER_USEC;
 }
 
+static inline int cfs_rq_throttled(struct cfs_rq *cfs_rq)
+{
+	return cfs_rq->throttled;
+}
+
+static inline int throttled_hierarchy(struct cfs_rq *cfs_rq)
+{
+	struct task_group *tg;
+	struct sched_entity *se;
+
+	if (cfs_rq_throttled(cfs_rq))
+		return 1;
+
+	tg = cfs_rq->tg;
+	se = tg->se[cpu_of(rq_of(cfs_rq))];
+	if (!se)
+		return 0;
+
+	for_each_sched_entity(se) {
+		if (cfs_rq_throttled(cfs_rq_of(se)))
+			return 1;
+	}
+
+	return 0;
+}
+
+static inline int within_bandwidth(struct cfs_rq *cfs_rq)
+{
+	return !cfs_rq->quota_enabled || cfs_rq->quota_remaining > 0;
+}
+
+static void check_cfs_rq_quota(struct cfs_rq *cfs_rq)
+{
+	if (within_bandwidth(cfs_rq))
+		return;
+
+
+	resched_task(rq_of(cfs_rq)->curr);
+}
+
 static void request_cfs_rq_quota(struct cfs_rq *cfs_rq)
 {
 	struct task_group *tg = cfs_rq->tg;

@@ -1330,6 +1381,29 @@ static void account_cfs_rq_quota(struct 
 	request_cfs_rq_quota(cfs_rq);
 }
 
+static void throttle_cfs_rq(struct cfs_rq *cfs_rq)
+{
+	struct sched_entity *se;
+
+	se = cfs_rq->tg->se[cpu_of(rq_of(cfs_rq))];
+
+	/* account load preceding throttle */
+	update_cfs_load(cfs_rq, 0);
+
+	for_each_sched_entity(se) {
+		struct cfs_rq *qcfs_rq = cfs_rq_of(se);
+		/* throttled entity or throttle-on-deactivate */

Quoted 167 lines · Paul Turner

In account_cfs_rq_quota() (via update_curr()) we track consumption versus a cfs_rqs locally assigned quota and whether there is global quota available to provide a refill when it runs out.

In the case that there is no quota remaining it's necessary to throttle so that execution ceases until the susbequent period.  While it is at this boundary that we detect (and signal for, via reshed_task) that a throttle is required, the actual operation is deferred until put_prev_entity().

At this point the cfs_rq is marked as throttled and not re-enqueued, this avoids potential interactions with throttled runqueues in the event that we are not immediately able to evict the running task.

Signed-off-by Paul Turner <[email protected]>

Signed-off-by Nikhil Rao <[email protected]>

Signed-off-by Bharata B Rao <[email protected]>

---
 kernel/sched.c      |    2
 kernel/sched_fair.c |  117 +++++++++++++++++++++++++++++++++++++++++++++++++---

 2 files changed, 113 insertions(+), 6 deletions(-)

Index: tip/kernel/sched.c
===================================================================

kernel/sched.c +1 -1

@@ -386,7 +386,7 @@ struct cfs_rq {
       unsigned long load_contribution;
  #endif
  #ifdef CONFIG_CFS_BANDWIDTH
-     int quota_enabled;
+     int quota_enabled, throttled;
       s64 quota_remaining;
  #endif
  #endif

Index: tip/kernel/sched_fair.c
===================================================================

kernel/sched_fair.c +11 -3

@@ -321,9 +321,6 @@ find_matching_se(struct sched_entity **s
 
  #endif       /* CONFIG_FAIR_GROUP_SCHED */
 
-static void account_cfs_rq_quota(struct cfs_rq *cfs_rq,
-                              unsigned long delta_exec);
-
  /**************************************************************
   * Scheduling class tree data structure manipulation methods:
   */

@@ -588,6 +585,9 @@ __update_curr(struct cfs_rq *cfs_rq, str
  #endif
  }
 
+static void account_cfs_rq_quota(struct cfs_rq *cfs_rq,
+             unsigned long delta_exec);
+
  static void update_curr(struct cfs_rq *cfs_rq)
  {
       struct sched_entity *curr = cfs_rq->curr;

@@ -1221,6 +1221,9 @@ static struct sched_entity *pick_next_en
       return se;
  }
 
+static void throttle_cfs_rq(struct cfs_rq *cfs_rq);
+static inline int within_bandwidth(struct cfs_rq *cfs_rq);
+
  static void put_prev_entity(struct cfs_rq *cfs_rq, struct sched_entity *prev)
  {
       /*

@@ -1230,6 +1233,9 @@ static void put_prev_entity(struct cfs_r
       if (prev->on_rq)
               update_curr(cfs_rq);
 
+     if (!within_bandwidth(cfs_rq))
+             throttle_cfs_rq(cfs_rq);
+
       check_spread(cfs_rq, prev);
       if (prev->on_rq) {
               update_stats_wait_start(cfs_rq, prev);

@@ -1241,6 +1247,8 @@ static void put_prev_entity(struct cfs_r
       cfs_rq->curr = NULL;
  }
 
+static void check_cfs_rq_quota(struct cfs_rq *cfs_rq);
+

 static void
 entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued) {

kernel/sched_fair.c +62 -0

@@ -1249,6 +1257,9 @@ entity_tick(struct cfs_rq *cfs_rq, struc
        */
       update_curr(cfs_rq);
 
+     /* check that entity's usage is still within quota (if enabled) */
+     check_cfs_rq_quota(cfs_rq);
+
       /*
        * Update share accounting for long-running entities.
        */

@@ -1294,6 +1305,46 @@ static inline u64 sched_cfs_bandwidth_sl
         return (u64)sysctl_sched_cfs_bandwidth_slice * NSEC_PER_USEC;
  }
 
+static inline int cfs_rq_throttled(struct cfs_rq *cfs_rq)
+{
+     return cfs_rq->throttled;
+}
+
+static inline int throttled_hierarchy(struct cfs_rq *cfs_rq)
+{
+     struct task_group *tg;
+     struct sched_entity *se;
+
+     if (cfs_rq_throttled(cfs_rq))
+             return 1;
+
+     tg = cfs_rq->tg;
+     se = tg->se[cpu_of(rq_of(cfs_rq))];
+     if (!se)
+             return 0;
+
+     for_each_sched_entity(se) {
+             if (cfs_rq_throttled(cfs_rq_of(se)))
+                     return 1;
+     }
+
+     return 0;
+}
+
+static inline int within_bandwidth(struct cfs_rq *cfs_rq)
+{
+     return !cfs_rq->quota_enabled || cfs_rq->quota_remaining > 0;
+}
+
+static void check_cfs_rq_quota(struct cfs_rq *cfs_rq)
+{
+     if (within_bandwidth(cfs_rq))
+             return;
+
+
+     resched_task(rq_of(cfs_rq)->curr);
+}
+
  static void request_cfs_rq_quota(struct cfs_rq *cfs_rq)
  {
       struct task_group *tg = cfs_rq->tg;

@@ -1330,6 +1381,29 @@ static void account_cfs_rq_quota(struct
       request_cfs_rq_quota(cfs_rq);
  }
 
+static void throttle_cfs_rq(struct cfs_rq *cfs_rq)
+{
+     struct sched_entity *se;
+
+     se = cfs_rq->tg->se[cpu_of(rq_of(cfs_rq))];
+
+     /* account load preceding throttle */
+     update_cfs_load(cfs_rq, 0);
+
+     for_each_sched_entity(se) {
+             struct cfs_rq *qcfs_rq = cfs_rq_of(se);
+             /* throttled entity or throttle-on-deactivate */
+             if (!se->on_rq)
+                     break;
+
+             dequeue_entity(qcfs_rq, se, DEQUEUE_SLEEP);
+             if (qcfs_rq->load.weight)
+                     break;
+     }

kernel/sched_fair.c static void account_cfs_rq_quota(struct · line 1405

Quoted 3 lines

kernel/sched_fair.c +3 -0

+
+     cfs_rq->throttled = 1;
+}

Since throttling is done from put_prev_entity(), iiuc, you will be doing 'put' for current entities which are not on the tree. Can you avoid the dequeue_entity() call here which I think will anyway bail out from actual dequeueing (se != cfs_rq->curr check in dequeue_entity).

Regards,
Bharata.

kernel/sched_fair.c +1 -1

@@ -1447,10 +1544,15 @@ static void dequeue_task_fair(struct rq 
        for_each_sched_entity(se) {
                cfs_rq = cfs_rq_of(se);
                dequeue_entity(cfs_rq, se, flags);
-
+               /* end evaluation on throttled cfs_rq */

kernel/sched_fair.c +10 -0

+static inline int throttled_hierarchy(struct cfs_rq *cfs_rq)
+{
+       struct task_group *tg;
+       struct sched_entity *se;
+
+       if (cfs_rq_throttled(cfs_rq))
+               return 1;
+
+       tg = cfs_rq->tg;
+       se = tg->se[cpu_of(rq_of(cfs_rq))];

kernel/sched_fair.c entity_tick(struct cfs_rq *cfs_rq, struc · line 1265

Quoted 10 lines · Paul Turner

kernel/sched_fair.c +3 -0

@@ -1249,6 +1257,9 @@ entity_tick(struct cfs_rq *cfs_rq, struc
        */
       update_curr(cfs_rq);
 
+     /* check that entity's usage is still within quota (if enabled) */
+     check_cfs_rq_quota(cfs_rq);
+
       /*
        * Update share accounting for long-running entities.
        */

You already have a hook in update_curr() to account quota, why not also use that to trigger the reschedule? request_cfs_rq_quota() already has the information we failed to replenish the local quota.

Then when you've gotten rid of check_cfs_rq_quota() there isn't a second user of within_bandwidth() and you can fold:

kernel/sched_fair.c static void put_prev_entity(struct cfs_r · line 1241

Quoted 11 lines

kernel/sched_fair.c +3 -0

@@ -1230,6 +1233,9 @@ static void put_prev_entity(struct cfs_r
       if (prev->on_rq)
               update_curr(cfs_rq);
 
+     if (!within_bandwidth(cfs_rq))
+             throttle_cfs_rq(cfs_rq);
+
       check_spread(cfs_rq, prev);
       if (prev->on_rq) {
               update_stats_wait_start(cfs_rq, prev);

Into a single hook.

Quoted 11 lines

kernel/sched_fair.c +5 -1

@@ -1447,10 +1544,15 @@ static void dequeue_task_fair(struct rq
         for_each_sched_entity(se) {
                 cfs_rq = cfs_rq_of(se);
                 dequeue_entity(cfs_rq, se, flags);
-
+               /* end evaluation on throttled cfs_rq */
+               if (cfs_rq_throttled(cfs_rq)) {
+                       se = NULL;
+                       break;
+               }
                 /* Don't dequeue parent if it has other entities besides us */

kernel/sched_fair.c static void dequeue_task_fair(struct rq · line 1557

Quoted 5 lines

kernel/sched_fair.c +1 -0

                 if (cfs_rq->load.weight)
                         break;
+               check_cfs_rq_quota(cfs_rq);
                 flags |= DEQUEUE_SLEEP;
         }

dequeue_entity() calls update_curr(), so again, by folding check_cfs_rq_quota() into your update_curr() hook this becomes simpler.

Quoted 14 lines

kernel/sched_fair.c +14 -0

+static inline int throttled_hierarchy(struct cfs_rq *cfs_rq)
+{
+       struct task_group *tg;
+       struct sched_entity *se;
+
+       if (cfs_rq_throttled(cfs_rq))
+               return 1;
+
+       tg = cfs_rq->tg;
+       se = tg->se[cpu_of(rq_of(cfs_rq))];
+       if (!se)
+               return 0;
+
+       for_each_sched_entity(se) {

kernel/sched_fair.c static inline u64 sched_cfs_bandwidth_sl · line 1329

Quoted 6 lines

+               if (cfs_rq_throttled(cfs_rq_of(se)))
+                       return 1;
+       }

kernel/sched_fair.c +3 -0

+
+       return 0;
+}

You can actually call for_each_sched_entity() with se==NULL, saves a few lines.

kernel/sched_fair.c +49 -0

+static u64 distribute_cfs_bandwidth(struct cfs_bandwidth *cfs_b, u64 runtime)
+{
+       int i;
+       u64 quota, remaining = runtime;
+       const struct cpumask *span;
+
+       rcu_read_lock();
+       span = sched_bw_period_mask();
+       for_each_cpu(i, span) {
+               struct rq *rq = cpu_rq(i);
+               struct cfs_rq *cfs_rq = cfs_bandwidth_tg(cfs_b)->cfs_rq[i];
+
+               raw_spin_lock(&rq->lock);
+               if (within_bandwidth(cfs_rq))
+                       goto next;
+
+               quota = -cfs_rq->quota_remaining;
+               quota += sched_cfs_bandwidth_slice();
+               quota = min(quota, remaining);
+               remaining -= quota;
+
+               cfs_rq->quota_remaining += quota;
+               if (cfs_rq_throttled(cfs_rq) && cfs_rq->quota_remaining > 0)
+                       unthrottle_cfs_rq(cfs_rq);
+
+next:
+               raw_spin_unlock(&rq->lock);
+
+               if (!remaining)
+                       break;
+       }
+       rcu_read_unlock();
+
+       return remaining;
+}
+
 static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun)
 {
+       u64 runtime, runtime_assigned;
+       int idle;
+
+       raw_spin_lock(&cfs_b->lock);
+       runtime = cfs_b->quota;
+       idle = cfs_b->runtime == cfs_b->runtime_assigned;
+       raw_spin_unlock(&cfs_b->lock);
+
+       if (runtime == RUNTIME_INF)
+               return 1;
+
+       runtime *= overrun;
+       runtime_assigned = runtime;

->quota in case of overrun > 1, that shouldn't be possible, the

There's something fishy there, it looks like ->runtime can end up being ->quota in case of overrun > 1, that shouldn't be possible, the refresh timer should never over-fill the bucket.

The !CONFIG_RT_GROUP_SCHED && CONFIG_SMP case has been collapsed to use rd->span instead of cpu_online_mask since I think that was incorrect before [don't actually want to hit cpu's outside of your root_domain for RT bandwidth.]

From the perspective of load-balance and shares distribution, throttled

O(n**2)

Throttled tasks are invisisble to cpu-offline since they are not eligible for selection by pick_next_task(). The regular 'escape' path for a thread that is blocked at offline is via ttwu->select_task_rq, however this will not handle a throttled group since there are no individual thread wakeups on an unthrottle.

Resolve this by unthrottling offline cpus so that threads can be migrated.

Quoted 6 lines · Paul Turner

Throttled tasks are invisisble to cpu-offline since they are not eligible for selection by pick_next_task().  The regular 'escape' path for a thread that is blocked at offline is via ttwu->select_task_rq, however this will not handle a throttled group since there are no individual thread wakeups on an unthrottle.

Resolve this by unthrottling offline cpus so that threads can be migrated.

Fair enough, the flip side is that they all can again increase their debt by a whole tick, right?

kernel/sched_fair.c +5 -0

@@ -1431,10 +1432,15 @@ static void unthrottle_cfs_rq(struct cfs
        struct rq *rq = rq_of(cfs_rq);
        struct sched_entity *se;
        struct tg_unthrottle_down_data udd;
+       struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(cfs_rq->tg);
 
        se = cfs_rq->tg->se[cpu_of(rq_of(cfs_rq))];
 
        update_rq_clock(rq);
+       raw_spin_lock(&cfs_b->lock);
+       cfs_b->throttled_time += (rq->clock - cfs_rq->throttled_timestamp);
+       raw_spin_unlock(&cfs_b->lock);
+       cfs_rq->throttled_timestamp = 0;
 
        /* don't include throttled window for load statistics */
        udd.cpu = rq->cpu;

kernel/sched_fair.c +0 -0

        raw_spin_unlock(&cfs_b->lock);
 
        return idle;

In dequeue_task_fair() we bail on dequeue when we encounter a parenting entity with additional weight. However, we perform a double shares update on this entity since we continue the shares update traversal from that point, despite dequeue_entity() having already updated its queuing cfs_rq.

Avoid this by starting from the parent when we resume.

With task entities participating in throttled sub-trees it is possible for task activation/de-activation to not lead to root visible changes to rq->nr_running. This in turn leads to incorrect idle and weight-per-task load balance decisions.

To allow correct accounting we move responsibility for updating rq->nr_running to the respective sched::classes. In the fair-group case this update is hierarchical, tracking the number of active tasks rooted at each group entity.

This also allows us to fix a small buglet in pick_next_task() when group scheduling is enabled.

Note: technically this issue also exists with the existing sched_rt throttling; however due to the nearly complete provisioning of system resources for rt scheduling this is much less common by default.

Hi Paul,

kernel/sched_fair.c static void throttle_cfs_rq(struct cfs_r · line 1445

Quoted 12 lines · Paul Turner

kernel/sched_fair.c +4 -0

@@ -1434,7 +1435,11 @@ static void throttle_cfs_rq(struct cfs_r
       if (!se)
               rq->nr_running += task_delta;
 
+     raw_spin_lock(&cfs_b->lock);
       cfs_rq->throttled = 1;
+     list_add_tail_rcu(&cfs_rq->throttled_list, &cfs_b->throttled_cfs_rq);
+     raw_spin_unlock(&cfs_b->lock);
+
       cfs_rq->throttled_timestamp = rq_of(cfs_rq)->clock;
  }
 

Though I'm looking forward to see your updates soon, just FYI, I found that somehow throttle_cfs_rq() was called with already throttled cfs_rq.

So it breaks list throttled_cfs_rq by repeating list_add_tail_rcu() and results in lockups like that Bharata and Xiao already reported.

There should be better fix, but following first aid to put_prev_entity() worked for me:

-       if (!within_bandwidth(cfs_rq))
+       if (!within_bandwidth(cfs_rq) && !cfs_rq->throttled) throttle_cfs_rq(cfs_rq);
       else
               return_cfs_rq_quota(cfs_rq);

I believe you can do better next time.

Thanks,
H.Seto

kernel/sched.c +2 -0

@@ -9333,6 +9337,8 @@ static int tg_set_cfs_bandwidth(struct t
 	raw_spin_lock_irq(&cfs_b->lock);
 	cfs_b->period = ns_to_ktime(period);
 	cfs_b->runtime = cfs_b->quota = quota;
+
+	cfs_bump_quota_generation(cfs_b);
 	raw_spin_unlock_irq(&cfs_b->lock);
 
 	for_each_possible_cpu(i) {

Index: tip/kernel/sched_fair.c
===================================================================

kernel/sched_fair.c +4 -0

+
+static inline int cfs_rq_quota_current(struct cfs_rq *cfs_rq)
+{
+	struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(cfs_rq->tg);

On Tue, 2011-03-22 at 20:03 -0700, Paul Turner wrote:

Argh, this patch is terrible for the reason that it changes the whole accounting just introduced and me having to re-open all the previous patches to look up hth stuff worked before.

Quoted 8 lines

kernel/sched.c +1 -0

@@ -436,8 +438,10 @@ void init_cfs_bandwidth(struct cfs_bandw
       raw_spin_lock_init(&cfs_b->lock);
       cfs_b->quota = cfs_b->runtime = quota;
       cfs_b->period = ns_to_ktime(period);
+     cfs_b->quota_generation = 0;
       INIT_LIST_HEAD(&cfs_b->throttled_cfs_rq);
 

kernel/sched.c void init_cfs_bandwidth(struct cfs_bandw · line 446

Quoted 3 lines

kernel/sched.c +1 -0

+
       hrtimer_init(&cfs_b->period_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
       cfs_b->period_timer.function = sched_cfs_period_timer;

We're in desperate need of more whitespace there? :-)

Quoted 18 lines

kernel/sched.c +2 -0

@@ -9333,6 +9337,8 @@ static int tg_set_cfs_bandwidth(struct t
       raw_spin_lock_irq(&cfs_b->lock);
       cfs_b->period = ns_to_ktime(period);
       cfs_b->runtime = cfs_b->quota = quota;
+
+     cfs_bump_quota_generation(cfs_b);
       raw_spin_unlock_irq(&cfs_b->lock);
 
       for_each_possible_cpu(i) {

Index: tip/kernel/sched_fair.c
===================================================================

kernel/sched_fair.c +0 -0

@@ -1331,11 +1331,25 @@ static void check_cfs_rq_quota(struct cf
       resched_task(rq_of(cfs_rq)->curr);
  }
 

kernel/sched_fair.c static void check_cfs_rq_quota(struct cf · line 1338

Quoted 5 lines

kernel/sched_fair.c +5 -0

+static void cfs_bump_quota_generation(struct cfs_bandwidth *cfs_b)
+{
+     cfs_b->quota_generation++;
+     smp_mb();
+}

Memory barriers come in pairs and with a comment, you fail on both counts.

Quoted 4 lines

kernel/sched_fair.c +4 -0

+
+static inline int cfs_rq_quota_current(struct cfs_rq *cfs_rq)
+{
+     struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(cfs_rq->tg);

kernel/sched_fair.c static void check_cfs_rq_quota(struct cf · line 1352

Quoted 10 lines

kernel/sched_fair.c +5 -0

+
+     return cfs_rq->quota_generation == cfs_b->quota_generation;
+}
+
  static void request_cfs_rq_quota(struct cfs_rq *cfs_rq)
  {
       struct task_group *tg = cfs_rq->tg;
       struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(tg);
       u64 amount = 0, min_amount;
+     int generation;

Not initialized,

kernel/sched_fair.c static void request_cfs_rq_quota(struct · line 1366

Quoted 9 lines

kernel/sched_fair.c +1 -0

       min_amount = sched_cfs_bandwidth_slice() + (-cfs_rq->quota_remaining);
 

@@ -1347,10 +1361,18 @@ static void request_cfs_rq_quota(struct
               } else {
                       amount = min_amount;
               }
+             generation = cfs_b->quota_generation;
               raw_spin_unlock(&cfs_b->lock);
       }

and since there's an if there, one can fail it, leaving generation uninitialized,

Quoted 1 line

Quoted 9 lines

+     /* a deficit should be carried forwards, surplus should be dropped */

kernel/sched_fair.c +5 -0

+
+     if (generation != cfs_rq->quota_generation &&
+         cfs_rq->quota_remaining > 0)
+             cfs_rq->quota_remaining = 0;
+

      cfs_rq->quota_remaining += amount;
+     cfs_rq->quota_generation = generation;
 }

Resulting in uninitialized usage right there.

Quoted 3 lines

Argh, this patch is terrible for the reason that it changes the whole accounting just introduced and me having to re-open all the previous patches to look up hth stuff worked before.

I didn't think it was too drastic -- the introduction of the generation is more incremental. However I agree it does cause unnecessary churn within the accounting functions across the series. Given that expiring quota is a requirement, this can be streamlined by introducing some of these notions earlier in the series as opposed to bootstrapping them at the end here -- will clean it up.

scratches head... erm right, I meant to pair this with a read barrier on querying the generation but balked when I realized that still yields an lfence on x86 since I didn't want to introduce that to the update_curr() path.

While we can probably do away with the barrier completely (it's not critical that we line them up perfectly with the new generation), I've been thinking about this one and I think I have something a little nicer that also reduces the shared cache hits.

We can take advantage of the fact that sched_clocks are already synchronized within 2 jiffies and store the quota's expiration, instead of a generation, when we refresh.

This effectively yields a fairly simple control flow (we can use rq->clock since we're always paired with update_rq_clock operations):
a) our rq->clock < expiration always implies quota is valid

Obviously if our cpu clock is ahead of the one that issued the quota, our quota is still valid since the real deadline is even further behind
Even if our cpu's clock is behind the max 1.99 jiffies the amount of time that the stale quota can remain valid is basically already within our potential margin of error since for a long running process we check on each tick edge anyway.

b) our rq->clock > expiration

Again there's two cases, if our cpu clock is behind (or equal) then the deadline has indeed passed and the quota is expired. This can be confirmed by comparing the global deadline with our local one (the global expiration will have advanced with quota refresh for this to be true).

We can also catch that our cpu is potentially ahead -- by the fact that our rq->clock > expiration but that the global expiration has not yet advanced. In this case we recognize that our quota is still valid and extend our local expiration time by either the maximum margin of error or some fraction there of (say 1 jiffy) which is guaranteed to push us back in case a) above. Again this is within our existing margin of error due to entity_tick() alignment.

This ends up looking a lot simpler and avoids much of the pressure on the global variable since we need to compare against it in the case where our clock passes expiration, once a quota period (as the extension will put us in case a where we know we don't need to consider it).

This ends up simpler than the generation muck and can be introduced cleanly earlier in the series, avoiding the churn mentioned above.

Make sense?

Signed-off-by Paul Turner <[email protected]>

Signed-off-by Bharata B Rao <[email protected]>

Quoted 3 lines · Paul Turner

Signed-off-by Paul Turner <[email protected]>

Signed-off-by Bharata B Rao <[email protected]>

Shouldn't that be Suggested-by or somesuch? You wrote the patch, you send the patch, afaict it never passed through Bharata so where does that SOB come from?

Hi all,

Please find attached the latest version of bandwidth control for the normal scheduling class. This revision has undergone fairly extensive changes since the previous version based largely on the observation that many of the edge conditions requiring special casing around update_curr() were a result of introducing side-effects into that operation. By introducing an interstitial state, where we recognize that the runqueue is over bandwidth, but not marking it throttled until we can actually remove it from the CPU we avoid the previous possible interactions with throttled entities which eliminates some head-scratching corner cases.

Hi all,

Please find attached the latest version of bandwidth control for the normal scheduling class. This revision has undergone fairly extensive changes since the previous version based largely on the observation that many of the edge conditions requiring special casing around update_curr() were a result of introducing side-effects into that operation. By introducing an interstitial state, where we recognize that the runqueue is over bandwidth, but not marking it throttled until we can actually remove it from the CPU we avoid the previous possible interactions with throttled entities which eliminates some head-scratching corner cases.

Quoted 12 lines · Paul Turner

Hi all,

Please find attached the latest version of bandwidth control for the normal scheduling class.  This revision has undergone fairly extensive changes since the previous version based largely on the observation that many of the edge conditions requiring special casing around update_curr() were a result of introducing side-effects into that operation.  By introducing an interstitial state, where we recognize that the runqueue is over bandwidth, but not marking it throttled until we can actually remove it from the CPU we avoid the previous possible interactions with throttled entities which eliminates some head-scratching corner cases.

Hi Paul,

I have wrote some codes to test this patchset. While run attached test case, the test program is blocked, it seams the children tasks can not be killed, after one or two hours, the watchdog is expired and triggers box crashed.

In particular I'd like to thank Peter Zijlstra who provided extensive comments and review for the last series.

Hi Paul,

I'm so sorry for sending this mail in the new thread, since i didn't receive your V6 patchset from LKML.

It seams the patchset can not be applied, since it's conflict between patch 3 and patch 5:

========Quote========

========End quote========

I downloaded the patchset from Internet, i missed the newer version?

I have done some test after fixed the conflict by handle, below test can cause box crash:

========Quote　cpu_hotlpug.sh ========

======== End quote　cpu_hotlpug.sh ========

Sorry to disturb you if the bug is know.

Thanks!

I confirmed that running your test cause hung-up on my box. And after some investigation, I found that this is an infinite loop in migrate_task() due to miscalculation of rq->nr_running; when a task is queued to throttled entity the nr_running is incremented at the queuing and also the unthrottling.

I made a fix for this bug and it seems works well for me. Could you try this patch and give us your feedback, Xiao?

Hi Paul and Xiao,

Please check/test a fix at the foot of this mail.

Quoted 9 lines · Xiao Guangrong

Hi Paul,

I'm so sorry for sending this mail in the new thread, since i didn't receive your V6 patchset from LKML.

It seams the patchset can not be applied, since it's conflict between patch 3 and patch 5:

========Quote========

(snip)

Quoted 1 line

========End quote========

Maybe I've fixed it by hand, or git-am is so wonderful.

I believe Paul will do it right for next time.

Quoted 7 lines

I downloaded the patchset from Internet, i missed the newer version?

I have done some test after fixed the conflict by handle, below test can cause box crash:

========Quote　cpu_hotlpug.sh ========

(snip)

Quoted 5 lines

======== End quote　cpu_hotlpug.sh ========

Sorry to disturb you if the bug is know.

Thanks!

Thank you for reporting it, Xiao!

I confirmed that running your test cause hung-up on my box. And after some investigation, I found that this is an infinite loop in migrate_task() due to miscalculation of rq->nr_running; when a task is queued to throttled entity the nr_running is incremented at the queuing and also the unthrottling.

I made a fix for this bug and it seems works well for me. Could you try this patch and give us your feedback, Xiao?

Thanks,
H.Seto

---
 kernel/sched_fair.c |   28 +++++++++++++---------------

 1 files changed, 13 insertions(+), 15 deletions(-)

kernel/sched_fair.c +0 -0

index 3936393..544072f 100644

kernel/sched_fair.c +12 -13

-                       break;
-               }
+               if (cfs_rq_throttled(cfs_rq))
+                       goto done;
+
                flags = ENQUEUE_WAKEUP;
        }
 

@@ -1855,14 +1854,14 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
                cfs_rq->h_nr_running++;
 
                if (cfs_rq_throttled(cfs_rq))
-                       break;
+                       goto done;
 
                update_cfs_load(cfs_rq, 0);
                update_cfs_shares(cfs_rq);
        }
 
-       if (!se)
-               inc_nr_running(rq);
+       inc_nr_running(rq);
+done:
        hrtick_update(rq);
  }
 

@@ -1885,10 +1884,9 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
                cfs_rq->h_nr_running--;
 
                /* end evaluation on throttled cfs_rq */
-               if (cfs_rq_throttled(cfs_rq)) {
-                       se = NULL;
-                       break;
-               }
+               if (cfs_rq_throttled(cfs_rq))
+                       goto done;
+
                /* Don't dequeue parent if it has other entities besides us */
                if (cfs_rq->load.weight) {
                         /* Avoid double update below. */

@@ -1910,14 +1908,14 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
                cfs_rq->h_nr_running--;
 
                if (cfs_rq_throttled(cfs_rq))
-                       break;
+                       goto done;
 
                update_cfs_load(cfs_rq, 0);
                update_cfs_shares(cfs_rq);
        }
 
-       if (!se)
-               dec_nr_running(rq);
+       dec_nr_running(rq);
+done:
        hrtick_update(rq);
  }
 
--