Re: [PATCH] sched/deadline: fix dl bandwidth of root domain overflow after dl task dead

[Juri Lelli]

On 06/08/15 09:39, Wanpeng Li wrote:
> Hi Juri,
>

Hi,

> 2015-05-06 16:14 GMT+08:00 Juri Lelli <juri.lelli@xxxxxxx
> <mailto:juri.lelli@xxxxxxx>>:
> 
>     Hi Wanpeng,
> 
>     I finally got to review this, sorry about the huge delay.
> 
>     On 07/04/2015 04:36, Wanpeng Li wrote:
>     > The total used dl bandwidth of each root domain will be reset to 0 after
>     > cpu hotplug when rebuild sched domains, since the call path is:
>     >
>     > _cpu_down
>     >   cpuset_cpu_inactive()
>     >     cpuset_update_active_cpus()
>     >       partition_sched_domains()
>     >         build_sched_domains()
>     >           init_rootdomain()
>     >             init_dl_bw()
>     >
>     > The bandwidth which dl task occupy will be released when dl task dead,
>     > it will be minus from total used dl bandwidth of its root domain,
>     > however, bandwidth overflow occurs since total used dl bandwidth is 0.
>     >
> 
>     Right, that's a bug.
> 
>     > This patch fix it by attaching the bandwidth which dl task occupy to
>     > the new root domain when the task is migrating since cpu hotplug, and
>     > attach all the used dl bandwidth of dl tasks to the new root domain
>     > when sched domains are rebuild.
>     >
> 
>     But, I think this fix has still a couple of problems:
> 
>      - what happens if a DL task is simply sleeping when domains are
>        reconfigured?
> 
>      - def_root_domain has now multiple accounting problems, as you do
>        this thing even when a cpu is moved there in the cpuoff path
> 
>     Also, runqueue (and throttling) information are dynamic, while we
>     are trying to fix a static problem. It's probably not a good idea
>     mixing them.
> 
>     I'm not sure how (I need more time to think it through), but can
>     we maybe fix this using cpuset information?
> 
> 
> Any ideas?
> 

Yes, actually. I might have a different fix, but I'd like to play with
it a bit more as it is a bit too intrusive. Let me see if I can come
up with something that I can share.

Thanks,

- Juri

> Regards,
> Wanpeng Li
>  
> 
> 
>     Thanks,
> 
>     - Juri
> 
>     > Signed-off-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxxxxxx
>     <mailto:wanpeng.li@xxxxxxxxxxxxxxx>>
>     > ---
>     >  kernel/sched/core.c     |  1 +
>     >  kernel/sched/deadline.c | 25 +++++++++++++++++++++++++
>     >  kernel/sched/sched.h    |  1 +
>     >  3 files changed, 27 insertions(+)
>     >
>     > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>     > index 28b0d75..c940999 100644
>     > --- a/kernel/sched/core.c
>     > +++ b/kernel/sched/core.c
>     > @@ -5586,6 +5586,7 @@ static void rq_attach_root(struct rq *rq,
>     struct root_domain *rd)
>     >       rq->rd = rd;
>     >
>     >       cpumask_set_cpu(rq->cpu, rd->span);
>     > +     attach_dl_bw(rq);
>     >       if (cpumask_test_cpu(rq->cpu, cpu_active_mask))
>     >               set_rq_online(rq);
>     >
>     > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
>     > index 5e95145..62680d7 100644
>     > --- a/kernel/sched/deadline.c
>     > +++ b/kernel/sched/deadline.c
>     > @@ -224,6 +224,7 @@ static void dl_task_offline_migration(struct
>     rq *rq, struct task_struct *p)
>     >  {
>     >       struct rq *later_rq = NULL;
>     >       bool fallback = false;
>     > +     struct dl_bw *dl_b;
>     >
>     >       later_rq = find_lock_later_rq(p, rq);
>     >
>     > @@ -258,6 +259,11 @@ static void dl_task_offline_migration(struct
>     rq *rq, struct task_struct *p)
>     >       set_task_cpu(p, later_rq->cpu);
>     >       activate_task(later_rq, p, ENQUEUE_REPLENISH);
>     >
>     > +     dl_b = dl_bw_of(later_rq->cpu);
>     > +     raw_spin_lock(&dl_b->lock);
>     > +     __dl_add(dl_b, p->dl.dl_bw);
>     > +     raw_spin_unlock(&dl_b->lock);
>     > +
>     >       if (!fallback)
>     >               resched_curr(later_rq);
>     >
>     > @@ -1776,6 +1782,25 @@ static void prio_changed_dl(struct rq *rq,
>     struct task_struct *p,
>     >               switched_to_dl(rq, p);
>     >  }
>     >
>     > +void attach_dl_bw(struct rq *rq)
>     > +{
>     > +     struct rb_node *next_node = rq->dl.rb_leftmost;
>     > +     struct sched_dl_entity *dl_se;
>     > +     struct dl_bw *dl_b;
>     > +
>     > +     dl_b = dl_bw_of(rq->cpu);
>     > +     raw_spin_lock(&dl_b->lock);
>     > +next_node:
>     > +     if (next_node) {
>     > +             dl_se = rb_entry(next_node, struct sched_dl_entity,
>     rb_node);
>     > +             __dl_add(dl_b, dl_se->dl_bw);
>     > +             next_node = rb_next(next_node);
>     > +
>     > +             goto next_node;
>     > +     }
>     > +     raw_spin_unlock(&dl_b->lock);
>     > +}
>     > +
>     >  const struct sched_class dl_sched_class = {
>     >       .next                   = &rt_sched_class,
>     >       .enqueue_task           = enqueue_task_dl,
>     > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
>     > index e0e1299..a7b1a59 100644
>     > --- a/kernel/sched/sched.h
>     > +++ b/kernel/sched/sched.h
>     > @@ -1676,6 +1676,7 @@ extern void init_dl_rq(struct dl_rq *dl_rq);
>     >
>     >  extern void cfs_bandwidth_usage_inc(void);
>     >  extern void cfs_bandwidth_usage_dec(void);
>     > +void attach_dl_bw(struct rq *rq);
>     >
>     >  #ifdef CONFIG_NO_HZ_COMMON
>     >  enum rq_nohz_flag_bits {
>     >
> 
>     --
>     To unsubscribe from this list: send the line "unsubscribe
>     linux-kernel" in
>     the body of a message to majordomo@xxxxxxxxxxxxxxx
>     <mailto:majordomo@xxxxxxxxxxxxxxx>
>     More majordomo info at  http://vger.kernel.org/majordomo-info.html
>     Please read the FAQ at  http://www.tux.org/lkml/
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/