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

[Chengming Zhou <zhouchengming@bytedance.com>]

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

                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
>>