lkml.org 
[lkml]   [2022]   [May]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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@bytedance.com> wrote:
>
> Hi,
>
> On 2022/5/30 15:52, Vincent Guittot wrote:
> > On Thu, 26 May 2022 at 12:40, Chengming Zhou
> > <zhouchengming@bytedance.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
> >>

\
 
 \ /
  Last update: 2022-05-31 18:33    [W:0.187 / U:0.756 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site