Messages in this thread | | | Date | Fri, 15 Sep 2023 09:07:24 +0530 | Subject | Re: [RFC PATCH 2/2] sched/fair: skip the cache hot CPU in select_idle_cpu() | From | K Prateek Nayak <> |
| |
Hello Chenyu,
On 9/14/2023 4:13 PM, Chen Yu wrote: > Hi Prateek, > > On 2023-09-14 at 11:00:02 +0530, K Prateek Nayak wrote: >> Hello Chenyu, >> >> One question ... >> >> On 9/11/2023 8:20 AM, Chen Yu wrote: >>> [..snip..] >>> 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 >>> [..more snip..] >>> @@ -7052,10 +7072,14 @@ static int select_idle_core(struct task_struct *p, int core, struct cpumask *cpu >>> int cpu; >>> >>> for_each_cpu(cpu, cpu_smt_mask(core)) { >>> - if (!available_idle_cpu(cpu)) { >>> + bool cache_hot = sched_feat(SIS_CACHE) ? >>> + sched_clock_cpu(cpu) < cpu_rq(cpu)->cache_hot_timeout : false; >>> + >>> + if (!available_idle_cpu(cpu) || cache_hot) { >>> idle = false; >>> if (*idle_cpu == -1) { >>> - if (sched_idle_cpu(cpu) && cpumask_test_cpu(cpu, p->cpus_ptr)) { >>> + if (sched_idle_cpu(cpu) && cpumask_test_cpu(cpu, p->cpus_ptr) && >>> + !cache_hot) { >> >> Here, the CPU is running a SCHED_IDLE task ... >> >>> *idle_cpu = cpu; >>> break; >>> } >> >> ... but just below this, there are following lines to cache the idle_cpu: >> >> } >> if (*idle_cpu == -1 && cpumask_test_cpu(cpu, p->cpus_ptr)) >> *idle_cpu = cpu; >> >> Would it make sense to also add the same "cache_hot" check here when we >> come across an idle CPU during the search for an idle core? Something >> like: >> >> - if (*idle_cpu == -1 && cpumask_test_cpu(cpu, p->cpus_ptr)) > > When we reached above code, the following condition should be true: > (available_idle_cpu(cpu) && !cache_hot) > because the previous 'if' statement is false. So I guess we already > has !cache_hot ?
Ah! You are right. I missed the break at end of the if block. Thank you for pointing it out to me :)
> >> + if (*idle_cpu == -1 && !cache_hot && cpumask_test_cpu(cpu, p->cpus_ptr)) >> *idle_cpu = cpu; >> >> Implications with the above change: >> >> If the entire core is idle, "select_idle_core()" will return the core >> and the search will bail out in "select_idle_cpu()". Otherwise, the >> cache-hot idle CPUs encountered during the search for idle core will be >> ignored now and if "idle_cpu" is not -1, it contains an idle CPU that is >> not cache-hot. >> >> Thoughts? >> > > Yes, agree, we want to skip the cache-hot idle CPU if that entire core is not idle > in your case. > > thanks, > Chenyu
-- Thanks and Regards, Prateek
| |