Messages in this thread | | | From | bsegall@google ... | Subject | Re: [PATCH] sched: tg_set_cfs_bandwidth() causes rq->lock deadlock | Date | Mon, 19 May 2014 10:40:22 -0700 |
| |
Roman Gushchin <klamm@yandex-team.ru> 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 <klamm@yandex-team.ru> > Cc: Ben Segall <bsegall@google.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: pjt@google.com > Cc: Chris J Arges <chris.j.arges@canonical.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Reviewed-by: Ben Segall <bsegall@google.com>
> --- > 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);
| |