Re: [PATCH RFC] cpumask: Randomly distribute the tasks within affinity mask

[Ankit Jain]

> On 11-Oct-2023, at 5:16 PM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> 
> !! External Email
> 
> On Wed, Oct 11, 2023 at 12:53:29PM +0200, Peter Zijlstra wrote:
>> On Wed, Oct 11, 2023 at 12:49:25PM +0530, Ankit Jain wrote:
>>> commit 46a87b3851f0 ("sched/core: Distribute tasks within affinity masks")
>>> and commit 14e292f8d453 ("sched,rt: Use cpumask_any*_distribute()")
>>> introduced the logic to distribute the tasks at initial wakeup on cpus
>>> where load balancing works poorly or disabled at all (isolated cpus).
>>> 
>>> There are cases in which the distribution of tasks
>>> that are spawned on isolcpus does not happen properly.
>>> In production deployment, initial wakeup of tasks spawn from
>>> housekeeping cpus to isolcpus[nohz_full cpu] happens on first cpu
>>> within isolcpus range instead of distributed across isolcpus.
>>> 
>>> Usage of distribute_cpu_mask_prev from one processes group,
>>> will clobber previous value of another or other groups and vice-versa.
>>> 
>>> When housekeeping cpus spawn multiple child tasks to wakeup on
>>> isolcpus[nohz_full cpu], using cpusets.cpus/sched_setaffinity(),
>>> distribution is currently performed based on per-cpu
>>> distribute_cpu_mask_prev counter.
>>> At the same time, on housekeeping cpus there are percpu
>>> bounded timers interrupt/rcu threads and other system/user tasks
>>> would be running with affinity as housekeeping cpus. In a real-life
>>> environment, housekeeping cpus are much fewer and are too much loaded.
>>> So, distribute_cpu_mask_prev value from these tasks impacts
>>> the offset value for the tasks spawning to wakeup on isolcpus and
>>> thus most of the tasks end up waking up on first cpu within the
>>> isolcpus set.
>>> 
>>> Steps to reproduce:
>>> Kernel cmdline parameters:
>>> isolcpus=2-5 skew_tick=1 nohz=on nohz_full=2-5
>>> rcu_nocbs=2-5 rcu_nocb_poll idle=poll irqaffinity=0-1
>>> 
>>> * pid=$(echo $$)
>>> * taskset -pc 0 $pid
>>> * cat loop-normal.c
>>> int main(void)
>>> {
>>>        while (1)
>>>                ;
>>>        return 0;
>>> }
>>> * gcc -o loop-normal loop-normal.c
>>> * for i in {1..50}; do ./loop-normal & done
>>> * pids=$(ps -a | grep loop-normal | cut -d' ' -f5)
>>> * for i in $pids; do taskset -pc 2-5 $i ; done
>>> 
>>> Expected output:
>>> * All 50 “loop-normal” tasks should wake up on cpu2-5
>>> equally distributed.
>>> * ps -eLo cpuid,pid,tid,ppid,cls,psr,cls,cmd | grep "^    [2345]"
>>> 
>>> Actual output:
>>> * All 50 “loop-normal” tasks got woken up on cpu2 only
>> 
>> Your expectation is wrong. Things work as advertised.
> 
> That is, isolcpus results in single CPU balance domains and as such we
> must not distribute -- there is no load balancing.
> 
> Ideally we'd reject setting cpumasks with multi bits set on domains like
> that, but alas, that would break historical behaviour :/

Thank you Peter for investing your time in reviewing this change.
I completely agree and understand that cpumask with multi bits 
should not be set for domains like isolcpus and should probably
be addressed in user space.

> 
> Now, looking at the code, I don't think the current code actually
> behaves correct in this case :-(, somewhere along the line we should
> truncate cpu_valid_mask to a single bit. Let me see where the sane place
> is to do that.
> 
> 
> 
> !! External Email: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.