Received-SPF: pass (google.com: domain of linux-kernel+bounces-85311-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <86f000a3-3068-4c57-858d-c00e3eb6e974@arm.com>
Date: Wed, 28 Feb 2024 16:58:48 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/2] sched/fair: Add EAS checks before updating
 overutilized
To: Shrikanth Hegde <sshegde@linux.ibm.com>, mingo@kernel.org,
 peterz@infradead.org, vincent.guittot@linaro.org
Cc: yu.c.chen@intel.com, dietmar.eggemann@arm.com,
 linux-kernel@vger.kernel.org, nysal@linux.ibm.com, aboorvad@linux.ibm.com,
 srikar@linux.vnet.ibm.com, vschneid@redhat.com, morten.rasmussen@arm.com,
 qyousef@layalina.io
References: <20240228071621.602596-1-sshegde@linux.ibm.com>
 <20240228071621.602596-2-sshegde@linux.ibm.com>
Content-Language: en-US
From: Pierre Gondois <pierre.gondois@arm.com>
In-Reply-To: <20240228071621.602596-2-sshegde@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

It is nice to avoid calling effective_cpu_util() through the following
when EAS is not enabled:

cpu_overutilized()
\-util_fits_cpu()
   \- ...
     \-effective_cpu_util()

On 2/28/24 08:16, Shrikanth Hegde wrote:
> Overutilized field of root domain is only used for EAS(energy aware scheduler)
> to decide whether to do regular load balance or EAS aware load balance. It
> is not used if EAS not possible.
> 
> Currently enqueue_task_fair and task_tick_fair accesses, sometime updates
> this field. In update_sd_lb_stats it is updated often.
> Which causes cache contention due to load/store tearing and burns
> a lot of cycles. Hence add EAS check before updating this field.
> EAS check is optimized at compile time or it is static branch.
> Hence it shouldn't cost much.
> 
> With the patch, both enqueue_task_fair and newidle_balance don't show
> up as hot routines in perf profile.
> 
> 6.8-rc4:
> 7.18%  swapper          [kernel.vmlinux]              [k] enqueue_task_fair
> 6.78%  s                [kernel.vmlinux]              [k] newidle_balance
> +patch:
> 0.14%  swapper          [kernel.vmlinux]              [k] enqueue_task_fair
> 0.00%  swapper          [kernel.vmlinux]              [k] newidle_balance
> 
> Minor change; trace_sched_overutilized_tp expect that second argument to
> be bool. So do a int to bool conversion for that.
> 
> Fixes: 2802bf3cd936 ("sched/fair: Add over-utilization/tipping point indicator")
> Signed-off-by: Shrikanth Hegde <sshegde@linux.ibm.com>
> ---
>   kernel/sched/fair.c | 35 ++++++++++++++++++++++++-----------
>   1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8e30e2bb77a0..3105fb08b87e 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -6670,15 +6670,30 @@ static inline bool cpu_overutilized(int cpu)
>   	return !util_fits_cpu(cpu_util_cfs(cpu), rq_util_min, rq_util_max, cpu);
>   }
> 
> -static inline void update_overutilized_status(struct rq *rq)
> +static inline void update_rd_overutilized_status(struct root_domain *rd,
> +						 int status)
>   {
> -	if (!READ_ONCE(rq->rd->overutilized) && cpu_overutilized(rq->cpu)) {
> -		WRITE_ONCE(rq->rd->overutilized, SG_OVERUTILIZED);
> -		trace_sched_overutilized_tp(rq->rd, SG_OVERUTILIZED);
> +	if (sched_energy_enabled()) {
> +		WRITE_ONCE(rd->overutilized, status);
> +		trace_sched_overutilized_tp(rd, !!status);
> +	}
> +}

NIT:
When called from check_update_overutilized_status(),
sched_energy_enabled() will be checked twice.

> +
> +static inline void check_update_overutilized_status(struct rq *rq)
> +{
> +	/*
> +	 * overutilized field is used for load balancing decisions only
> +	 * if energy aware scheduler is being used
> +	 */
> +	if (sched_energy_enabled()) {
> +		if (!READ_ONCE(rq->rd->overutilized) && cpu_overutilized(rq->cpu))
> +			update_rd_overutilized_status(rq->rd, SG_OVERUTILIZED);
>   	}
>   }
>   #else
> -static inline void update_overutilized_status(struct rq *rq) { }
> +static inline void check_update_overutilized_status(struct rq *rq) { }
> +static inline void update_rd_overutilized_status(struct root_domain *rd,
> +						 bool status) { }
>   #endif
> 
>   /* Runqueue only has SCHED_IDLE tasks enqueued */
> @@ -6779,7 +6794,7 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
>   	 * and the following generally works well enough in practice.
>   	 */
>   	if (!task_new)
> -		update_overutilized_status(rq);
> +		check_update_overutilized_status(rq);
> 
>   enqueue_throttle:
>   	assert_list_leaf_cfs_rq(rq);
> @@ -10613,13 +10628,11 @@ static inline void update_sd_lb_stats(struct lb_env *env, struct sd_lb_stats *sd
>   		WRITE_ONCE(rd->overload, sg_status & SG_OVERLOAD);
> 
>   		/* Update over-utilization (tipping point, U >= 0) indicator */
> -		WRITE_ONCE(rd->overutilized, sg_status & SG_OVERUTILIZED);
> -		trace_sched_overutilized_tp(rd, sg_status & SG_OVERUTILIZED);
> +		update_rd_overutilized_status(rd, sg_status & SG_OVERUTILIZED);
>   	} else if (sg_status & SG_OVERUTILIZED) {
>   		struct root_domain *rd = env->dst_rq->rd;
> 
> -		WRITE_ONCE(rd->overutilized, SG_OVERUTILIZED);
> -		trace_sched_overutilized_tp(rd, SG_OVERUTILIZED);
> +		update_rd_overutilized_status(rd, SG_OVERUTILIZED);
>   	}
> 
>   	update_idle_cpu_scan(env, sum_util);
> @@ -12625,7 +12638,7 @@ static void task_tick_fair(struct rq *rq, struct task_struct *curr, int queued)
>   		task_tick_numa(rq, curr);
> 
>   	update_misfit_status(curr, rq);
> -	update_overutilized_status(task_rq(curr));
> +	check_update_overutilized_status(task_rq(curr));
> 
>   	task_tick_core(rq, curr);
>   }
> --
> 2.39.3
>