Received-SPF: pass (google.com: domain of linux-kernel+bounces-82985-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <d6699c3a-3df6-46a3-98db-e07c8722f106@arm.com>
Date: Tue, 27 Feb 2024 10:42:55 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 2/4] sched/fair: Check a task has a fitting cpu when
 updating misfit
Content-Language: en-US
To: Qais Yousef <qyousef@layalina.io>, Ingo Molnar <mingo@kernel.org>,
 Peter Zijlstra <peterz@infradead.org>,
 Vincent Guittot <vincent.guittot@linaro.org>
Cc: linux-kernel@vger.kernel.org, Pierre Gondois <Pierre.Gondois@arm.com>
References: <20240220225622.2626569-1-qyousef@layalina.io>
 <20240220225622.2626569-3-qyousef@layalina.io>
From: Dietmar Eggemann <dietmar.eggemann@arm.com>
In-Reply-To: <20240220225622.2626569-3-qyousef@layalina.io>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 20/02/2024 23:56, Qais Yousef wrote:
> If a misfit task is affined to a subset of the possible cpus, we need to
> verify that one of these cpus can fit it. Otherwise the load balancer
> code will continuously trigger needlessly leading the balance_interval
> to increase in return and eventually end up with a situation where real
> imbalances take a long time to address because of this impossible
> imbalance situation.
> 
> This can happen in Android world where it's common for background tasks
> to be restricted to little cores.
> 
> Similarly if we can't fit the biggest core, triggering misfit is
> pointless as it is the best we can ever get on this system.
> 
> To be able to detect that; we use asym_cap_list to iterate through
> capacities in the system to see if the task is able to run at a higher
> capacity level based on its p->cpus_ptr. We do that when the affinity
> change, a fair task is forked, or when a task switched to fair policy.
> We store the max_allowed_capacity in task_struct to allow for cheap
> comparison in the fast path.
> 
> Improve check_misfit_status() function by removing redundant checks.
> misfit_task_load will be 0 if the task can't move to a bigger CPU. And
> nohz_load_balance() already checks for cpu_check_capacity() before

s/nohz_load_balance()/nohz_balancer_kick() ?

> calling check_misfit_status().

Isn't there an issue with CPU hotplug.

On a tri-geared Juno:

root@juno:~# cat /sys/devices/system/cpu/cpu*/cpu_capacity
513
1024
1024
513
256
256

root@juno:~# taskset -pc 0,3-5 $$

[  108.248425] set_task_max_allowed_capacity() [bash 1636]
max_allowed_capacity=513 nr_cpus_allowed=4 cpus_mask=0,3-5

echo 0 > /sys//devices/system/cpu/cpu0/online
echo 0 > /sys//devices/system/cpu/cpu3/online

[  134.136887] set_task_max_allowed_capacity() [bash 1639]
max_allowed_capacity=513 nr_cpus_allowed=4 cpus_mask=0,3-5


Cpuset seems to be fine since it set task's cpumask.

[...]

> +/*
> + * Check the max capacity the task is allowed to run at for misfit detection.

Nitpick: It's rather a setter function so s/check/set here ?

> + */
> +static void set_task_max_allowed_capacity(struct task_struct *p)
> +{
> +	struct asym_cap_data *entry;
> +
> +	if (!sched_asym_cpucap_active())
> +		return;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(entry, &asym_cap_list, link) {
> +		cpumask_t *cpumask;
> +
> +		cpumask = cpu_capacity_span(entry);
> +		if (!cpumask_intersects(p->cpus_ptr, cpumask))
> +			continue;
> +
> +		p->max_allowed_capacity = entry->capacity;
> +		break;
> +	}
> +	rcu_read_unlock();
> +}

[...]

> @@ -9601,16 +9644,10 @@ check_cpu_capacity(struct rq *rq, struct sched_domain *sd)
>  				(arch_scale_cpu_capacity(cpu_of(rq)) * 100));
>  }
>  
> -/*
> - * Check whether a rq has a misfit task and if it looks like we can actually
> - * help that task: we can migrate the task to a CPU of higher capacity, or
> - * the task's current CPU is heavily pressured.
> - */
> -static inline int check_misfit_status(struct rq *rq, struct sched_domain *sd)
> +/* Check if the rq has a misfit task */
> +static inline bool check_misfit_status(struct rq *rq, struct sched_domain *sd)

`struct sched_domain *sd` is not needed anymore.

Since there is only 1 user of check_misfit_status() you might remove it
entirely and use `rq->rq->misfit_task_load` directly in
nohz_balancer_kick() ?

[...]