On 6 June 2016 at 12:52, Dietmar Eggemann <dietmar.eggem...@arm.com> wrote:
> On 24/05/16 10:55, Vincent Guittot wrote:
>
> [...]
>
>> +/* Take into account the change of the utilization of a child task group */
>> +static void update_tg_cfs_util(struct sched_entity *se, int blocked)
>> +{
>> + int delta;
>> + struct cfs_rq *cfs_rq;
>> + long update_util_avg;
>> + long last_update_time;
>> + long old_util_avg;
>> +
>> +
>> + /*
>> + * update_blocked_average will call this function for root cfs_rq
>> + * whose se is null. In this case just return
>> + */
>> + if (!se)
>> + return;
>> +
>> + if (entity_is_task(se))
>> + return 0;
>
> void function
yes, i have seen that and correct it in the next version that i'm preparing
>
>> +
>> + /* Get sched_entity of cfs rq */
>
> You mean /* Get cfs_rq owned by this task group */?
yes
>
>> + cfs_rq = group_cfs_rq(se);
>> +
>> + update_util_avg = cfs_rq->diff_util_avg;
>> +
>> + if (!update_util_avg)
>> + return 0;
>
> Couldn't you not just get rid of long update_util_avg and only use
> cfs_rq->diff_util_avg here (clearing pending changes after you set
> se->avg.util_avg)?
yes probably
At the origin, i have done that to prevent simultaneous use of the
value but it's not need AFAICT now
>
>> +
>> + /* Clear pending changes */
>> + cfs_rq->diff_util_avg = 0;
>> +
>> + /* Add changes in sched_entity utilizaton */
>> + old_util_avg = se->avg.util_avg;
>> + se->avg.util_avg = max_t(long, se->avg.util_avg + update_util_avg, 0);
>> + se->avg.util_sum = se->avg.util_avg * LOAD_AVG_MAX;
>> +
>> + /* Get parent cfs_rq */
>> + cfs_rq = cfs_rq_of(se);
>> +
>> + if (blocked) {
>> + /*
>> + * blocked utilization has to be synchronized with its parent
>> + * cfs_rq's timestamp
>> + */
>
> We don't have stand-alone blocked utilization any more although the
> function is still called update_blocked_averages(). Do you need this for
> cpu's going through idle periods?
yes blocked load/utilization has been merged with runnable ones but
there are still present and they have to be updated for idle cpus
> It's also necessary because there could be other se's which could have
> been [en|de]queued at/from this cfs_rq so its last_update_time value is
> more recent?
yes, the parent can have been updated because of other sched_entities
so we have to aligned time stamp before reflecting the change
>
> [...]
