Re: [PATCH v6 2/4] sched/fair: Check a task has a fitting cpu when updating misfit

[Qais Yousef]

On 02/23/24 10:30, Vincent Guittot wrote:
> 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@xxxxxxxxxxx>
> 
> I have a comment below but anyway
> 
> Reviewed-by: Vincent Guittot <vincent.guittot@xxxxxxxxxx>

Thanks for all the reviews and the help!

I sent v7 in reply to this taking your comment into account with minor tweaks
to avoid the else leg and white space to keep rq->misfit_task_load = 0 easily
spottable below the now long if condition.


Thanks!

--
Qais Yousef