[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <33d8d0c1-da40-278b-5b84-ecb983ee9d34@huawei.com>
Date: Mon, 16 Oct 2023 20:54:22 +0800
From: Yicong Yang <yangyicong@...wei.com>
To: Vincent Guittot <vincent.guittot@...aro.org>
CC: <yangyicong@...ilicon.com>, <peterz@...radead.org>,
<mingo@...hat.com>, <juri.lelli@...hat.com>,
<dietmar.eggemann@....com>, <tim.c.chen@...ux.intel.com>,
<yu.c.chen@...el.com>, <gautham.shenoy@....com>, <mgorman@...e.de>,
<vschneid@...hat.com>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <rostedt@...dmis.org>,
<bsegall@...gle.com>, <bristot@...hat.com>,
<prime.zeng@...wei.com>, <jonathan.cameron@...wei.com>,
<ego@...ux.vnet.ibm.com>, <srikar@...ux.vnet.ibm.com>,
<linuxarm@...wei.com>, <21cnbao@...il.com>,
<kprateek.nayak@....com>, <wuyun.abel@...edance.com>
Subject: Re: [PATCH v10 3/3] sched/fair: Use candidate prev/recent_used CPU if
scanning failed for cluster wakeup
Hi Vincent,
On 2023/10/13 23:04, Vincent Guittot wrote:
> On Thu, 12 Oct 2023 at 14:19, Yicong Yang <yangyicong@...wei.com> wrote:
>>
>> From: Yicong Yang <yangyicong@...ilicon.com>
>>
>> Chen Yu reports a hackbench regression of cluster wakeup when
>> hackbench threads equal to the CPU number [1]. Analysis shows
>> it's because we wake up more on the target CPU even if the
>> prev_cpu is a good wakeup candidate and leads to the decrease
>> of the CPU utilization.
>>
>> Generally if the task's prev_cpu is idle we'll wake up the task
>> on it without scanning. On cluster machines we'll try to wake up
>> the task in the same cluster of the target for better cache
>> affinity, so if the prev_cpu is idle but not sharing the same
>> cluster with the target we'll still try to find an idle CPU within
>> the cluster. This will improve the performance at low loads on
>> cluster machines. But in the issue above, if the prev_cpu is idle
>> but not in the cluster with the target CPU, we'll try to scan an
>> idle one in the cluster. But since the system is busy, we're
>> likely to fail the scanning and use target instead, even if
>> the prev_cpu is idle. Then leads to the regression.
>>
>> This patch solves this in 2 steps:
>> o record the prev_cpu/recent_used_cpu if they're good wakeup
>> candidates but not sharing the cluster with the target.
>> o on scanning failure use the prev_cpu/recent_used_cpu if
>> they're still idle
>>
>> [1] https://lore.kernel.org/all/ZGzDLuVaHR1PAYDt@chenyu5-mobl1/
>> Reported-by: Chen Yu <yu.c.chen@...el.com>
>> Signed-off-by: Yicong Yang <yangyicong@...ilicon.com>
>> ---
>> kernel/sched/fair.c | 19 ++++++++++++++++++-
>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
>> index 4039f9b348ec..f1d94668bd71 100644
>> --- a/kernel/sched/fair.c
>> +++ b/kernel/sched/fair.c
>> @@ -7392,7 +7392,7 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>> bool has_idle_core = false;
>> struct sched_domain *sd;
>> unsigned long task_util, util_min, util_max;
>> - int i, recent_used_cpu;
>> + int i, recent_used_cpu, prev_aff = -1;
>>
>> /*
>> * On asymmetric system, update task utilization because we will check
>> @@ -7425,6 +7425,8 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>>
>> if (cpus_share_resources(prev, target))
>> return prev;
>> +
>> + prev_aff = prev;
>> }
>>
>> /*
>> @@ -7457,6 +7459,8 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>>
>> if (cpus_share_resources(recent_used_cpu, target))
>> return recent_used_cpu;
>> + } else {
>> + recent_used_cpu = -1;
>> }
>>
>> /*
>> @@ -7497,6 +7501,19 @@ static int select_idle_sibling(struct task_struct *p, int prev, int target)
>> if ((unsigned)i < nr_cpumask_bits)
>> return i;
>>
>> + /*
>> + * For cluster machines which have lower sharing cache like L2 or
>> + * LLC Tag, we tend to find an idle CPU in the target's cluster
>> + * first. But prev_cpu or recent_used_cpu may also be a good candidate,
>> + * use them if possible when no idle CPU found in select_idle_cpu().
>> + */
>> + if ((unsigned int)prev_aff < nr_cpumask_bits &&
>> + (available_idle_cpu(prev_aff) || sched_idle_cpu(prev_aff)))
>
> Hasn't prev_aff (i.e. prev) been already tested as idle ?
>
>> + return prev_aff;
>> + if ((unsigned int)recent_used_cpu < nr_cpumask_bits &&
>> + (available_idle_cpu(recent_used_cpu) || sched_idle_cpu(recent_used_cpu)))
>> + return recent_used_cpu;
>
> same here
>
It was thought that there maybe a small potential race window here that the prev/recent_used
CPU becoming non-idle after scanning, discussed in [1]. I think the check here won't be
expensive so added it here. It should be redundant and can be removed.
[1] https://lore.kernel.org/all/ZIams6s+qShFWhfQ@BLR-5CG11610CF.amd.com/
Thanks.
>
>> +
>> return target;
>> }
>>
>> --
>> 2.24.0
>>
>
Powered by blists - more mailing lists