linux-kernel - Re: [External] Re: [PATCH v3 2/2] sched/fair: optimize and simplify rq leaf_cfs_rq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKfTPtAL_N=grk7kaqB6bsZHh5JCvckTk9AtzFCJpanD=g_LOg@mail.gmail.com>
Date:   Tue, 31 May 2022 18:32:25 +0200
From:   Vincent Guittot <vincent.guittot@...aro.org>
To:     Chengming Zhou <zhouchengming@...edance.com>
Cc:     mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com,
        dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
        mgorman@...e.de, bristot@...hat.com, vschneid@...hat.com,
        odin@...d.al, linux-kernel@...r.kernel.org,
        duanxiongchun@...edance.com, songmuchun@...edance.com
Subject: Re: [External] Re: [PATCH v3 2/2] sched/fair: optimize and simplify
 rq leaf_cfs_rq_list

On Tue, 31 May 2022 at 15:55, Chengming Zhou
<zhouchengming@...edance.com> wrote:
>
> Hi,
>
> On 2022/5/30 15:52, Vincent Guittot wrote:
> > On Thu, 26 May 2022 at 12:40, Chengming Zhou
> > <zhouchengming@...edance.com> 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
> >>