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 ?
> + 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
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