lkml.org 
[lkml]   [2017]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/3] sched/fair: Introduce scaled capacity awareness in select_idle_sibling code path
From
Date
Hi Joel,


On 10/02/2017 09:52 PM, Joel Fernandes wrote:
> Hi Rohit,
>
> On Thu, Sep 28, 2017 at 8:09 AM, Rohit Jain <rohit.k.jain@oracle.com> wrote:
> [..]
>>>> With this case, because we know from the past avg, one of the strands is
>>>> running low on capacity, I am trying to return a better strand for the
>>>> thread to start on.
>>>>
>>> I know what you're trying to do but they way you've retrofitted it into
>>> the
>>> core looks weird (to me) and makes the code unreadable and ugly IMO.
>>>
>>> Why not do something simpler like skip the core if any SMT thread has been
>>> running at lesser capacity? I'm not sure if this works great or if the
>>> maintainers
>>> will prefer your or my below approach, but I find the below diff much
>>> cleaner
>>> for the select_idle_core bit. It also makes more sense since resources are
>>> shared at SMT level so makes sense to me to skip the core altogether for
>>> this:
>>>
>>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>>> index 6ee7242dbe0a..f324a84e29f1 100644
>>> --- a/kernel/sched/fair.c
>>> +++ b/kernel/sched/fair.c
>>> @@ -5738,14 +5738,17 @@ static int select_idle_core(struct task_struct *p,
>>> struct sched_domain *sd, int
>>> for_each_cpu_wrap(core, cpus, target) {
>>> bool idle = true;
>>> + bool full_cap = true;
>>> for_each_cpu(cpu, cpu_smt_mask(core)) {
>>> cpumask_clear_cpu(cpu, cpus);
>>> if (!idle_cpu(cpu))
>>> idle = false;
>>> + if (!full_capacity(cpu))
>>> + full_cap = false;
>>> }
>>> - if (idle)
>>> + if (idle && full_cap)
>>> return core;
>>> }
>>>
>>
>>
>> Well, with your changes you will skip over fully idle cores which is not
>> an ideal thing either. I see that you were advocating for select
>> idle+lowest capacity core, whereas I was stopping at the first idlecore.
>>
>> Since the whole philosophy till now in this patch is "Don't spare an
>> idle CPU", I think the following diff might look better to you. Please
>> note this is only for discussion sakes, I haven't fully tested it yet.
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index ec15e5f..c2933eb 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -6040,7 +6040,9 @@ void __update_idle_core(struct rq *rq)
>> static int select_idle_core(struct task_struct *p, struct sched_domain *sd,
>> int target)
>> {
>> struct cpumask *cpus = this_cpu_cpumask_var_ptr(select_idle_mask);
>> - int core, cpu;
>> + int core, cpu, rcpu, backup_core;
>> +
>> + rcpu = backup_core = -1;
>>
>> if (!static_branch_likely(&sched_smt_present))
>> return -1;
>> @@ -6052,15 +6054,34 @@ static int select_idle_core(struct task_struct *p,
>> struct sched_domain *sd, int
>>
>> for_each_cpu_wrap(core, cpus, target) {
>> bool idle = true;
>> + bool full_cap = true;
>>
>> for_each_cpu(cpu, cpu_smt_mask(core)) {
>> cpumask_clear_cpu(cpu, cpus);
>> if (!idle_cpu(cpu))
>> idle = false;
>> +
>> + if (!full_capacity(cpu)) {
>> + full_cap = false;
>> + }
>> }
>>
>> - if (idle)
>> + if (idle && full_cap)
>> return core;
>> + else if (idle && backup_core == -1)
>> + backup_core = core;
>> + }
>> +
>> + if (backup_core != -1) {
>> + for_each_cpu(cpu, cpu_smt_mask(backup_core)) {
>> + if (full_capacity(cpu))
>> + return cpu;
>> + else if ((rcpu == -1) ||
>> + (capacity_of(cpu) > capacity_of(rcpu)))
>> + rcpu = cpu;
>> + }
>> +
>> + return rcpu;
>> }
>>
>>
>> Do let me know what you think.
> I think that if there isn't a benefit in your tests in doing the above
> vs the simpler approach, then I prefer the simpler approach especially
> since there's no point/benefit in complicating the code for
> select_idle_core.

Fair enough!

If there are no more concerns in this version, then I will go ahead and
try out all that is discussed in this version and send an updated
version. Please let me know if there are any other concerns/feedback.

Thanks,
Rohit

>
> thanks,
>
> - Joel

\
 
 \ /
  Last update: 2017-10-04 02:19    [W:0.130 / U:1.024 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site