Re: [RFC PATCH 2/2] sched/fair: skip the cache hot CPU in select_idle_cpu()

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

On 9/11/23 11:26, Mathieu Desnoyers wrote:
> On 9/10/23 22:50, Chen Yu wrote:
[...]
>> ---
>>   kernel/sched/fair.c     | 30 +++++++++++++++++++++++++++---
>>   kernel/sched/features.h |  1 +
>>   kernel/sched/sched.h    |  1 +
>>   3 files changed, 29 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index e20f50726ab8..fe3b760c9654 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -6629,6 +6629,21 @@ static void dequeue_task_fair(struct rq *rq, 
>> struct task_struct *p, int flags)
>>       hrtick_update(rq);
>>       now = sched_clock_cpu(cpu_of(rq));
>>       p->se.prev_sleep_time = task_sleep ? now : 0;
>> +#ifdef CONFIG_SMP
>> +    /*
>> +     * If this rq will become idle, and dequeued task is
>> +     * a short sleeping one, check if we can reserve
>> +     * this idle CPU for that task for a short while.
>> +     * During this reservation period, other wakees will
>> +     * skip this 'idle' CPU in select_idle_cpu(), and this
>> +     * short sleeping task can pick its previous CPU in
>> +     * select_idle_sibling(), which brings better cache
>> +     * locality.
>> +     */
>> +    if (sched_feat(SIS_CACHE) && task_sleep && !rq->nr_running &&
>> +        p->se.sleep_avg && p->se.sleep_avg < 
>> sysctl_sched_migration_cost)
>> +        rq->cache_hot_timeout = now + p->se.sleep_avg;
> 
> This is really cool!
> 
> There is one scenario that worries me with this approach: workloads
> that sleep for a long time and then have short blocked periods.
> Those bursts will likely bring the average to values too high
> to stay below sysctl_sched_migration_cost.
> 
> I wonder if changing the code above for the following would help ?
> 
> if (sched_feat(SIS_CACHE) && task_sleep && !rq->nr_running && 
> p->se.sleep_avg)
>      rq->cache_hot_timeout = now + min(sysctl_sched_migration_cost, 
> p->se.sleep_avg);
> 
> For tasks that have a large sleep_avg, it would activate this rq
> "appear as not idle for rq selection" scheme for a window of
> sysctl_sched_migration_cost. If the sleep ends up being a long one,
> preventing other tasks from being migrated to this rq for a tiny
> window should not matter performance-wise. I would expect that it
> could help workloads that come in bursts.

There is perhaps a better way to handle bursts:

When calculating the sleep_avg, we actually only really care about
the sleep time for short bursts, so we could use the sysctl_sched_migration_cost
to select which of the sleeps should be taken into account in the avg.

We could rename the field "sleep_avg" to "burst_sleep_avg", and have:

u64 now = sched_clock_cpu(task_cpu(p));

if ((flags & ENQUEUE_WAKEUP) && last_sleep && cpu_online(task_cpu(p)) &&
     now > last_sleep && now - last_sleep < sysctl_sched_migration_cost)
	update_avg(&p->se.burst_sleep_avg, now - last_sleep);

Then we can have this code is dequeue_task_fair:

if (sched_feat(SIS_CACHE) && task_sleep && !rq->nr_running && p->se.busrt_sleep_avg)
	rq->cache_hot_timeout = now + p->se.burst_sleep_avg;

Thoughts ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com