From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751569AbcFFKwe (ORCPT <rfc822;w@1wt.eu>);
	Mon, 6 Jun 2016 06:52:34 -0400
Received: from foss.arm.com ([217.140.101.70]:35556 "EHLO foss.arm.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750789AbcFFKwc (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Mon, 6 Jun 2016 06:52:32 -0400
Subject: Re: [RFC PATCH v2] sched: reflect sched_entity movement into
 task_group's utilization
To: Vincent Guittot <vincent.guittot@linaro.org>, peterz@infradead.org,
        mingo@kernel.org, linux-kernel@vger.kernel.org, pjt@google.com
References: <1464080252-17209-1-git-send-email-vincent.guittot@linaro.org>
 <1464083754-4424-1-git-send-email-vincent.guittot@linaro.org>
Cc: yuyang.du@intel.com, bsegall@google.com, Morten.Rasmussen@arm.com
From: Dietmar Eggemann <dietmar.eggemann@arm.com>
Message-ID: <575555ED.5080509@arm.com>
Date: Mon, 6 Jun 2016 11:52:29 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.8.0
MIME-Version: 1.0
In-Reply-To: <1464083754-4424-1-git-send-email-vincent.guittot@linaro.org>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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?

[...]