Roman Gushchin <[email protected]> writes:
> tg_set_cfs_bandwidth() sets cfs_b->timer_active to 0 to
> force the period timer restart. It's not safe, because
> can lead to deadlock, described in commit 927b54fccbf0:
> "__start_cfs_bandwidth calls hrtimer_cancel while holding rq->lock,
> waiting for the hrtimer to finish. However, if sched_cfs_period_timer
> runs for another loop iteration, the hrtimer can attempt to take
> rq->lock, resulting in deadlock."
> Three CPUs must be involved:
> CPU0 CPU1 CPU2
> take rq->lock period timer fired
> ... take cfs_b lock
> ... ... tg_set_cfs_bandwidth()
> throttle_cfs_rq() release cfs_b lock take cfs_b lock
> ... distribute_cfs_runtime() timer_active = 0
> take cfs_b->lock wait for rq->lock ...
> __start_cfs_bandwidth()
> {wait for timer callback
> break if timer_active == 1}
>
> So, CPU0 and CPU1 are deadlocked.
>
> Instead of resetting cfs_b->timer_active, tg_set_cfs_bandwidth can
> wait for period timer callbacks (ignoring cfs_b->timer_active) and
> restart the timer explicitly.
>
> Signed-off-by: Roman Gushchin <[email protected]>
> Cc: Ben Segall <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> Cc: [email protected]
> Cc: Chris J Arges <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
Reviewed-by: Ben Segall <[email protected]>
> ---
> kernel/sched/core.c | 3 +--
> kernel/sched/fair.c | 8 ++++----
> kernel/sched/sched.h | 2 +-
> 3 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index b4308d7..e122774 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7826,8 +7826,7 @@ static int tg_set_cfs_bandwidth(struct task_group *tg,
> u64 period, u64 quota)
> /* restart the period timer (if active) to handle new period expiry */
> if (runtime_enabled && cfs_b->timer_active) {
> /* force a reprogram */
> - cfs_b->timer_active = 0;
> - __start_cfs_bandwidth(cfs_b);
> + __start_cfs_bandwidth(cfs_b, true);
> }
> raw_spin_unlock_irq(&cfs_b->lock);
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index c7ab8ea..54939f8 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -2117,7 +2117,7 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> */
> if (!cfs_b->timer_active) {
> __refill_cfs_bandwidth_runtime(cfs_b);
> - __start_cfs_bandwidth(cfs_b);
> + __start_cfs_bandwidth(cfs_b, false);
> }
>
> if (cfs_b->runtime > 0) {
> @@ -2298,7 +2298,7 @@ static void throttle_cfs_rq(struct cfs_rq *cfs_rq)
> raw_spin_lock(&cfs_b->lock);
> list_add_tail_rcu(&cfs_rq->throttled_list, &cfs_b->throttled_cfs_rq);
> if (!cfs_b->timer_active)
> - __start_cfs_bandwidth(cfs_b);
> + __start_cfs_bandwidth(cfs_b, false);
> raw_spin_unlock(&cfs_b->lock);
> }
>
> @@ -2681,7 +2681,7 @@ static void init_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> }
>
> /* requires cfs_b->lock, may release to reprogram timer */
> -void __start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
> +void __start_cfs_bandwidth(struct cfs_bandwidth *cfs_b, bool force)
> {
> /*
> * The timer may be active because we're trying to set a new bandwidth
> @@ -2696,7 +2696,7 @@ void __start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
> cpu_relax();
> raw_spin_lock(&cfs_b->lock);
> /* if someone else restarted the timer then we're done */
> - if (cfs_b->timer_active)
> + if (!force && cfs_b->timer_active)
> return;
> }
>
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index dfa31d5..bc150a6 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -209,7 +209,7 @@ extern void init_cfs_bandwidth(struct cfs_bandwidth
> *cfs_b);
> extern int sched_group_set_shares(struct task_group *tg, unsigned long
> shares);
>
> extern void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b);
> -extern void __start_cfs_bandwidth(struct cfs_bandwidth *cfs_b);
> +extern void __start_cfs_bandwidth(struct cfs_bandwidth *cfs_b, bool force);
> extern void unthrottle_cfs_rq(struct cfs_rq *cfs_rq);
>
> extern void free_rt_sched_group(struct task_group *tg);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
