Re: [External] Re: [PATCH v3 2/2] sched/fair: optimize and simplify rq leaf_cfs_rq_list

[Vincent Guittot]

On Tue, 31 May 2022 at 15:55, Chengming Zhou
<zhouchengming@xxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 2022/5/30 15:52, Vincent Guittot wrote:
> > On Thu, 26 May 2022 at 12:40, Chengming Zhou
> > <zhouchengming@xxxxxxxxxxxxx> wrote:
> >>
> [...]
> >> @@ -11257,9 +11218,10 @@ static inline bool vruntime_normalized(struct task_struct *p)
> >>   */
> >>  static void propagate_entity_cfs_rq(struct sched_entity *se)
> >>  {
> >> -       struct cfs_rq *cfs_rq;
> >> +       struct cfs_rq *cfs_rq = cfs_rq_of(se);
> >>
> >> -       list_add_leaf_cfs_rq(cfs_rq_of(se));
> >> +       if (!throttled_hierarchy(cfs_rq))
> >> +               list_add_leaf_cfs_rq(cfs_rq);
> >>
> >>         /* Start to propagate at parent */
> >>         se = se->parent;
> >> @@ -11268,7 +11230,8 @@ static void propagate_entity_cfs_rq(struct sched_entity *se)
> >>                 cfs_rq = cfs_rq_of(se);
> >>
> >
> >  you can break  if the cfs is throttled because it's sched_entity has
> > been dequeued. In this case we check if the cfs is throttled not if
> > the hierarchy is throttled
> >
> > +               if (cfs_rq_throttled(cfs_rq))
> > +                       break;
> >
>
> This propagate part still make me confused. :-)
>
> I wonder if you think we should change like this:
>
> static void propagate_entity_cfs_rq(struct sched_entity *se)
> {
>         struct cfs_rq *cfs_rq = cfs_rq_of(se);
>
>         if (cfs_rq_throttled(cfs_rq))                   --> break if cfs is throttled
>                 return;
>
>         if (!throttled_hierarchy(cfs_rq))
>                 list_add_leaf_cfs_rq(cfs_rq);
>
>         /* Start to propagate at parent */
>         se = se->parent;
>
>         for_each_sched_entity(se) {
>                 cfs_rq = cfs_rq_of(se);
>
>                 if (cfs_rq_throttled(cfs_rq))           --> break if cfs is throttled
>                         break;
>
>                 update_load_avg(cfs_rq, se, UPDATE_TG); --> throttled cfs_rq->prop not updated
>                 if (!throttled_hierarchy(cfs_rq))
>                         list_add_leaf_cfs_rq(cfs_rq);
>         }
> }
>
>
>
> If I understand right, we should update_load_avg() until cfs_rq_throttled(),
> including that throttled cfs_rq? So we can go on propagating when unthrottle.
>
> Maybe like this?
>
> static void propagate_entity_cfs_rq(struct sched_entity *se)
> {
>         struct cfs_rq *cfs_rq = cfs_rq_of(se);
>
>         if (cfs_rq_throttled(cfs_rq))                   --> break if cfs is throttled
>                 return;
>
>         if (!throttled_hierarchy(cfs_rq))
>                 list_add_leaf_cfs_rq(cfs_rq);
>
>         /* Start to propagate at parent */
>         se = se->parent;
>
>         for_each_sched_entity(se) {
>                 cfs_rq = cfs_rq_of(se);
>
>                 update_load_avg(cfs_rq, se, UPDATE_TG); --> update throttled cfs_rq->prop

Yes, that looks correct.
So we will ensure that the attach/detach is propagated down up to
thethrottle cfs

>
>                 if (cfs_rq_throttled(cfs_rq))           --> break if cfs is throttled
>                         break;
>
>                 if (!throttled_hierarchy(cfs_rq))
>                         list_add_leaf_cfs_rq(cfs_rq);
>         }
> }
>
>
> Thanks!
>
>
> >>                 update_load_avg(cfs_rq, se, UPDATE_TG);
> >> -               list_add_leaf_cfs_rq(cfs_rq);
> >> +               if (!throttled_hierarchy(cfs_rq))
> >> +                       list_add_leaf_cfs_rq(cfs_rq);
> >>         }
> >>  }
> >>  #else
> >> --
> >> 2.36.1
> >>