Received-SPF: pass (google.com: domain of linux-kernel+bounces-78014-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 23 Feb 2024 10:30:31 +0100
From: Vincent Guittot <vincent.guittot@linaro.org>
To: Qais Yousef <qyousef@layalina.io>
Cc: Ingo Molnar <mingo@kernel.org>, Peter Zijlstra <peterz@infradead.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	linux-kernel@vger.kernel.org,
	Pierre Gondois <Pierre.Gondois@arm.com>
Subject: Re: [PATCH v6 2/4] sched/fair: Check a task has a fitting cpu when
 updating misfit
Message-ID: <Zdhlt9GZe502wpPy@vingu-book>
References: <20240220225622.2626569-1-qyousef@layalina.io>
 <20240220225622.2626569-3-qyousef@layalina.io>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240220225622.2626569-3-qyousef@layalina.io>

Le mardi 20 f?vr. 2024 ? 22:56:20 (+0000), Qais Yousef a ?crit :
> 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
> calling check_misfit_status().
> 
> Test:
> =====
> 
> Add
> 
> 	trace_printk("balance_interval = %lu\n", interval)
> 
> in get_sd_balance_interval().
> 
> run
> 	if [ "$MASK" != "0" ]; then
> 		adb shell "taskset -a $MASK cat /dev/zero > /dev/null"
> 	fi
> 	sleep 10
> 	// parse ftrace buffer counting the occurrence of each valaue
> 
> Where MASK is either:
> 
> 	* 0: no busy task running
> 	* 1: busy task is pinned to 1 cpu; handled today to not cause
> 	  misfit
> 	* f: busy task pinned to little cores, simulates busy background
> 	  task, demonstrates the problem to be fixed
> 
> Results:
> ========
> 
> Note how occurrence of balance_interval = 128 overshoots for MASK = f.
> 
> BEFORE
> ------
> 
> 	MASK=0
> 
> 		   1 balance_interval = 175
> 		 120 balance_interval = 128
> 		 846 balance_interval = 64
> 		  55 balance_interval = 63
> 		 215 balance_interval = 32
> 		   2 balance_interval = 31
> 		   2 balance_interval = 16
> 		   4 balance_interval = 8
> 		1870 balance_interval = 4
> 		  65 balance_interval = 2
> 
> 	MASK=1
> 
> 		  27 balance_interval = 175
> 		  37 balance_interval = 127
> 		 840 balance_interval = 64
> 		 167 balance_interval = 63
> 		 449 balance_interval = 32
> 		  84 balance_interval = 31
> 		 304 balance_interval = 16
> 		1156 balance_interval = 8
> 		2781 balance_interval = 4
> 		 428 balance_interval = 2
> 
> 	MASK=f
> 
> 		   1 balance_interval = 175
> 		1328 balance_interval = 128
> 		  44 balance_interval = 64
> 		 101 balance_interval = 63
> 		  25 balance_interval = 32
> 		   5 balance_interval = 31
> 		  23 balance_interval = 16
> 		  23 balance_interval = 8
> 		4306 balance_interval = 4
> 		 177 balance_interval = 2
> 
> AFTER
> -----
> 
> Note how the high values almost disappear for all MASK values. The
> system has background tasks that could trigger the problem without
> simulate it even with MASK=0.
> 
> 	MASK=0
> 
> 		 103 balance_interval = 63
> 		  19 balance_interval = 31
> 		 194 balance_interval = 8
> 		4827 balance_interval = 4
> 		 179 balance_interval = 2
> 
> 	MASK=1
> 
> 		 131 balance_interval = 63
> 		   1 balance_interval = 31
> 		  87 balance_interval = 8
> 		3600 balance_interval = 4
> 		   7 balance_interval = 2
> 
> 	MASK=f
> 
> 		   8 balance_interval = 127
> 		 182 balance_interval = 63
> 		   3 balance_interval = 31
> 		   9 balance_interval = 16
> 		 415 balance_interval = 8
> 		3415 balance_interval = 4
> 		  21 balance_interval = 2
> 
> Signed-off-by: Qais Yousef <qyousef@layalina.io>

I have a comment below but anyway

Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>

> ---
>  include/linux/sched.h |  1 +
>  init/init_task.c      |  1 +
>  kernel/sched/fair.c   | 77 +++++++++++++++++++++++++++++++++----------
>  3 files changed, 61 insertions(+), 18 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index ffe8f618ab86..774cddbeab09 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -835,6 +835,7 @@ struct task_struct {
>  #endif
>  
>  	unsigned int			policy;
> +	unsigned long			max_allowed_capacity;
>  	int				nr_cpus_allowed;
>  	const cpumask_t			*cpus_ptr;
>  	cpumask_t			*user_cpus_ptr;
> diff --git a/init/init_task.c b/init/init_task.c
> index 7ecb458eb3da..b3dbab4c959e 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -77,6 +77,7 @@ struct task_struct init_task __aligned(L1_CACHE_BYTES) = {
>  	.cpus_ptr	= &init_task.cpus_mask,
>  	.user_cpus_ptr	= NULL,
>  	.cpus_mask	= CPU_MASK_ALL,
> +	.max_allowed_capacity	= SCHED_CAPACITY_SCALE,
>  	.nr_cpus_allowed= NR_CPUS,
>  	.mm		= NULL,
>  	.active_mm	= &init_mm,
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 8e30e2bb77a0..20006fcf7df2 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5092,24 +5092,35 @@ static inline int task_fits_cpu(struct task_struct *p, int cpu)
>  
>  static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
>  {
> +	unsigned long cpu_cap;
> +	int cpu = cpu_of(rq);
> +
>  	if (!sched_asym_cpucap_active())
>  		return;
>  
> -	if (!p || p->nr_cpus_allowed == 1) {
> -		rq->misfit_task_load = 0;
> -		return;
> -	}
> +	if (!p || p->nr_cpus_allowed == 1)
> +		goto out;
>  
> -	if (task_fits_cpu(p, cpu_of(rq))) {
> -		rq->misfit_task_load = 0;
> -		return;
> -	}
> +	cpu_cap = arch_scale_cpu_capacity(cpu);
> +
> +	/*
> +	 * Affinity allows us to go somewhere higher?  Or are we on biggest
> +	 * available CPU already?
> +	 */
> +	if (cpu_cap == p->max_allowed_capacity)
> +		goto out;
> +
> +	if (task_fits_cpu(p, cpu))
> +		goto out;

I think the below is easier to read:

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 209f5da17d79..0929e6704b44 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5095,35 +5095,25 @@ static inline int task_fits_cpu(struct task_struct *p, int cpu)

 static inline void update_misfit_status(struct task_struct *p, struct rq *rq)
 {
-       unsigned long cpu_cap;
        int cpu = cpu_of(rq);

        if (!sched_asym_cpucap_active())
                return;

-       if (!p || p->nr_cpus_allowed == 1)
-               goto out;
-
-       cpu_cap = arch_scale_cpu_capacity(cpu);
-
        /*
         * Affinity allows us to go somewhere higher?  Or are we on biggest
-        * available CPU already?
+        * available CPU already? Or do we fit into this CPU ?
         */
-       if (cpu_cap == p->max_allowed_capacity)
-               goto out;
-
-       if (task_fits_cpu(p, cpu))
-               goto out;
-
-       /*
-        * Make sure that misfit_task_load will not be null even if
-        * task_h_load() returns 0.
-        */
-       rq->misfit_task_load = max_t(unsigned long, task_h_load(p), 1);
-       return;
-out:
-       rq->misfit_task_load = 0;
+       if (!p || (p->nr_cpus_allowed == 1) ||
+          (arch_scale_cpu_capacity(cpu) == p->max_allowed_capacity) ||
+          task_fits_cpu(p, cpu))
+               rq->misfit_task_load = 0;
+       else
+               /*
+                * Make sure that misfit_task_load will not be null even if
+                * task_h_load() returns 0.
+                */
+               rq->misfit_task_load = max_t(unsigned long, task_h_load(p), 1);
 }

 #else /* CONFIG_SMP */


>  
>  	/*
>  	 * Make sure that misfit_task_load will not be null even if
>  	 * task_h_load() returns 0.
>  	 */
>  	rq->misfit_task_load = max_t(unsigned long, task_h_load(p), 1);
> +	return;
> +out:
> +	rq->misfit_task_load = 0;
>  }
>  
>  #else /* CONFIG_SMP */
> @@ -8241,6 +8252,36 @@ static void task_dead_fair(struct task_struct *p)
>  	remove_entity_load_avg(&p->se);
>  }
>  
> +/*
> + * Check the max capacity the task is allowed to run at for misfit detection.
> + */
> +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();
> +}
> +
> +static void set_cpus_allowed_fair(struct task_struct *p, struct affinity_context *ctx)
> +{
> +	set_cpus_allowed_common(p, ctx);
> +	set_task_max_allowed_capacity(p);
> +}
> +
>  static int
>  balance_fair(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
>  {
> @@ -8249,6 +8290,8 @@ balance_fair(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
>  
>  	return newidle_balance(rq, rf) != 0;
>  }
> +#else
> +static inline void set_task_max_allowed_capacity(struct task_struct *p) {}
>  #endif /* CONFIG_SMP */
>  
>  static void set_next_buddy(struct sched_entity *se)
> @@ -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)
>  {
> -	return rq->misfit_task_load &&
> -		(arch_scale_cpu_capacity(rq->cpu) < rq->rd->max_cpu_capacity ||
> -		 check_cpu_capacity(rq, sd));
> +	return rq->misfit_task_load;
>  }
>  
>  /*
> @@ -12645,6 +12682,8 @@ static void task_fork_fair(struct task_struct *p)
>  	rq_lock(rq, &rf);
>  	update_rq_clock(rq);
>  
> +	set_task_max_allowed_capacity(p);
> +
>  	cfs_rq = task_cfs_rq(current);
>  	curr = cfs_rq->curr;
>  	if (curr)
> @@ -12768,6 +12807,8 @@ static void switched_to_fair(struct rq *rq, struct task_struct *p)
>  {
>  	attach_task_cfs_rq(p);
>  
> +	set_task_max_allowed_capacity(p);
> +
>  	if (task_on_rq_queued(p)) {
>  		/*
>  		 * We were most likely switched from sched_rt, so
> @@ -13139,7 +13180,7 @@ DEFINE_SCHED_CLASS(fair) = {
>  	.rq_offline		= rq_offline_fair,
>  
>  	.task_dead		= task_dead_fair,
> -	.set_cpus_allowed	= set_cpus_allowed_common,
> +	.set_cpus_allowed	= set_cpus_allowed_fair,
>  #endif
>  
>  	.task_tick		= task_tick_fair,
> -- 
> 2.34.1
>