Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1740163 > unrolled thread
| Started by | Rohit Jain <rohit.k.jain@oracle.com> |
|---|---|
| First post | 2017-09-26 21:50 +0200 |
| Last post | 2017-09-28 17:20 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 2/3] sched/fair: Introduce scaled capacity awareness in select_idle_sibling code path Rohit Jain <rohit.k.jain@oracle.com> - 2017-09-26 21:50 +0200
Re: [PATCH 2/3] sched/fair: Introduce scaled capacity awareness in select_idle_sibling code path joelaf <joelaf@google.com> - 2017-09-28 13:00 +0200
Re: [PATCH 2/3] sched/fair: Introduce scaled capacity awareness in select_idle_sibling code path Rohit Jain <rohit.k.jain@oracle.com> - 2017-09-28 17:20 +0200
| From | Rohit Jain <rohit.k.jain@oracle.com> |
|---|---|
| Date | 2017-09-26 21:50 +0200 |
| Subject | Re: [PATCH 2/3] sched/fair: Introduce scaled capacity awareness in select_idle_sibling code path |
| Message-ID | <uu41J-zi-27@gated-at.bofh.it> |
On 09/25/2017 11:53 PM, Joel Fernandes wrote:
> Hi Rohit,
>
> Just some comments:
Hi Joel,
Thanks for the comments.
> On Mon, Sep 25, 2017 at 5:02 PM, Rohit Jain <rohit.k.jain@oracle.com> wrote:
>> While looking for CPUs to place running tasks on, the scheduler
>> completely ignores the capacity stolen away by RT/IRQ tasks.
>>
>> This patch fixes that.
>>
>> Signed-off-by: Rohit Jain <rohit.k.jain@oracle.com>
>> ---
>> kernel/sched/fair.c | 54 ++++++++++++++++++++++++++++++++++++++++++-----------
>> 1 file changed, 43 insertions(+), 11 deletions(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index afb701f..19ff2c3 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -6040,7 +6040,10 @@ 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, rcpu_backup;
> I would call rcpu_backup as backup_cpu.
OK
>
>> + unsigned int backup_cap = 0;
>> +
>> + rcpu = rcpu_backup = -1;
>>
>> if (!static_branch_likely(&sched_smt_present))
>> return -1;
>> @@ -6057,10 +6060,20 @@ static int select_idle_core(struct task_struct *p, struct sched_domain *sd, int
>> cpumask_clear_cpu(cpu, cpus);
>> if (!idle_cpu(cpu))
>> idle = false;
>> +
>> + if (full_capacity(cpu)) {
>> + rcpu = cpu;
>> + } else if ((rcpu == -1) && (capacity_of(cpu) > backup_cap)) {
>> + backup_cap = capacity_of(cpu);
>> + rcpu_backup = cpu;
>> + }
> Here you comparing capacity of different SMT threads.
>
>> }
>>
>> - if (idle)
>> - return core;
>> + if (idle) {
>> + if (rcpu == -1)
>> + return (rcpu_backup != -1 ? rcpu_backup : core);
>> + return rcpu;
>> + }
>
> This didn't make much sense to me, here you are returning either an
> SMT thread or a core. That doesn't make much of a difference because
> SMT threads share the same capacity (SD_SHARE_CPUCAPACITY). I think
> what you want to do is find out the capacity of a 'core', not an SMT
> thread, and compare the capacity of different cores and consider the
> one which has least RT/IRQ interference.
IIUC the capacities of each strand is scaled by IRQ and 'rt_avg' for that
'rq'. Now if the strand is idle now and gets an interrupt in the future,
the 'core' would look like:
+----+----+
| I | |
| T | |
+----+----+
(I -> Interrupt, T-> Thread we are trying to schedule).
whereas if the other strand on the core was taking interrupt the core
would look like:
+----+----+
| I | T |
| | |
+----+----+
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.
>
>> }
>>
>> /*
>> @@ -6076,7 +6089,8 @@ static int select_idle_core(struct task_struct *p, struct sched_domain *sd, int
>> */
>> static int select_idle_smt(struct task_struct *p, struct sched_domain *sd, int target)
>> {
>> - int cpu;
>> + int cpu, backup_cpu = -1;
>> + unsigned int backup_cap = 0;
>>
>> if (!static_branch_likely(&sched_smt_present))
>> return -1;
>> @@ -6084,11 +6098,17 @@ static int select_idle_smt(struct task_struct *p, struct sched_domain *sd, int t
>> for_each_cpu(cpu, cpu_smt_mask(target)) {
>> if (!cpumask_test_cpu(cpu, &p->cpus_allowed))
>> continue;
>> - if (idle_cpu(cpu))
>> - return cpu;
>> + if (idle_cpu(cpu)) {
>> + if (full_capacity(cpu))
>> + return cpu;
>> + if (capacity_of(cpu) > backup_cap) {
>> + backup_cap = capacity_of(cpu);
>> + backup_cpu = cpu;
>> + }
>> + }
> Same thing here, since SMT threads share the same underlying capacity,
> is there any point in comparing the capacities of each SMT thread?
See above
Thanks,
Rohit
>
> thanks,
>
> - Joel
>
> [...]
[toc] | [next] | [standalone]
| From | joelaf <joelaf@google.com> |
|---|---|
| Date | 2017-09-28 13:00 +0200 |
| Message-ID | <uuEHV-7Wb-11@gated-at.bofh.it> |
| In reply to | #1740163 |
Hi Rohit,
On Tue, Sep 26, 2017 at 12:48 PM, Rohit Jain <rohit.k.jain@oracle.com> wrote:
[...]
>>> + unsigned int backup_cap = 0;
>>> +
>>> + rcpu = rcpu_backup = -1;
>>>
>>> if (!static_branch_likely(&sched_smt_present))
>>> return -1;
>>> @@ -6057,10 +6060,20 @@ static int select_idle_core(struct task_struct
>>> *p, struct sched_domain *sd, int
>>> cpumask_clear_cpu(cpu, cpus);
>>> if (!idle_cpu(cpu))
>>> idle = false;
>>> +
>>> + if (full_capacity(cpu)) {
>>> + rcpu = cpu;
>>> + } else if ((rcpu == -1) && (capacity_of(cpu) >
>>> backup_cap)) {
>>> + backup_cap = capacity_of(cpu);
>>> + rcpu_backup = cpu;
>>> + }
>>
>> Here you comparing capacity of different SMT threads.
>>
>>> }
>>>
>>> - if (idle)
>>> - return core;
>>> + if (idle) {
>>> + if (rcpu == -1)
>>> + return (rcpu_backup != -1 ? rcpu_backup :
>>> core);
>>> + return rcpu;
>>> + }
>>
>>
>> This didn't make much sense to me, here you are returning either an
>> SMT thread or a core. That doesn't make much of a difference because
>> SMT threads share the same capacity (SD_SHARE_CPUCAPACITY). I think
>> what you want to do is find out the capacity of a 'core', not an SMT
>> thread, and compare the capacity of different cores and consider the
>> one which has least RT/IRQ interference.
>
>
> IIUC the capacities of each strand is scaled by IRQ and 'rt_avg' for that
> 'rq'. Now if the strand is idle now and gets an interrupt in the future,
> the 'core' would look like:
>
> +----+----+
> | I | |
> | T | |
> +----+----+
>
> (I -> Interrupt, T-> Thread we are trying to schedule).
>
> whereas if the other strand on the core was taking interrupt the core
> would look like:
>
> +----+----+
> | I | T |
> | | |
> +----+----+
>
> 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;
}
thanks,
- Joel
[toc] | [prev] | [next] | [standalone]
| From | Rohit Jain <rohit.k.jain@oracle.com> |
|---|---|
| Date | 2017-09-28 17:20 +0200 |
| Message-ID | <uuILw-2bi-21@gated-at.bofh.it> |
| In reply to | #1741425 |
Hi Joel,
On 09/28/2017 05:53 AM, joelaf wrote:
> Hi Rohit,
>
> On Tue, Sep 26, 2017 at 12:48 PM, Rohit Jain <rohit.k.jain@oracle.com> wrote:
> [...]
>
<snip>
>>>> }
>>>>
>>>> - if (idle)
>>>> - return core;
>>>> + if (idle) {
>>>> + if (rcpu == -1)
>>>> + return (rcpu_backup != -1 ? rcpu_backup :
>>>> core);
>>>> + return rcpu;
>>>> + }
>>>
>>> This didn't make much sense to me, here you are returning either an
>>> SMT thread or a core. That doesn't make much of a difference because
>>> SMT threads share the same capacity (SD_SHARE_CPUCAPACITY). I think
>>> what you want to do is find out the capacity of a 'core', not an SMT
>>> thread, and compare the capacity of different cores and consider the
>>> one which has least RT/IRQ interference.
>>
>> IIUC the capacities of each strand is scaled by IRQ and 'rt_avg' for that
>> 'rq'. Now if the strand is idle now and gets an interrupt in the future,
>> the 'core' would look like:
>>
>> +----+----+
>> | I | |
>> | T | |
>> +----+----+
>>
>> (I -> Interrupt, T-> Thread we are trying to schedule).
>>
>> whereas if the other strand on the core was taking interrupt the core
>> would look like:
>>
>> +----+----+
>> | I | T |
>> | | |
>> +----+----+
>>
>> 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.
Thanks,
Rohit
>
> thanks,
>
> - Joel
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web