Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1414893 > unrolled thread

Re: [RFC PATCH v2] sched: reflect sched_entity movement into task_group's utilization

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH v2] sched: reflect sched_entity movement into  task_group's utilization Dietmar Eggemann <dietmar.eggemann@arm.com> - 2016-06-06 13:00 +0200
    Re: [RFC PATCH v2] sched: reflect sched_entity movement into  task_group's utilization Vincent Guittot <vincent.guittot@linaro.org> - 2016-06-06 14:50 +0200

#1414893 — Re: [RFC PATCH v2] sched: reflect sched_entity movement into task_group's utilization

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

> +
> +	/* Get sched_entity of cfs rq */

You mean /* Get cfs_rq owned by this task group */?

> +	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)?

> +
> +	/* 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?
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?

[...]

[toc] | [next] | [standalone]

#1414995

On 6 June 2016 at 12:52, Dietmar Eggemann <dietmar.eggemann@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

>
> [...]

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web