Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Date:   Sat, 14 Oct 2023 00:20:10 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     Vincent Guittot <vincent.guittot@linaro.org>
CC:     Yicong Yang <yangyicong@huawei.com>, <peterz@infradead.org>,
        <mingo@redhat.com>, <juri.lelli@redhat.com>,
        <dietmar.eggemann@arm.com>, <tim.c.chen@linux.intel.com>,
        <gautham.shenoy@amd.com>, <mgorman@suse.de>, <vschneid@redhat.com>,
        <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <rostedt@goodmis.org>,
        <bsegall@google.com>, <bristot@redhat.com>,
        <prime.zeng@huawei.com>, <yangyicong@hisilicon.com>,
        <jonathan.cameron@huawei.com>, <ego@linux.vnet.ibm.com>,
        <srikar@linux.vnet.ibm.com>, <linuxarm@huawei.com>,
        <21cnbao@gmail.com>, <kprateek.nayak@amd.com>,
        <wuyun.abel@bytedance.com>
Subject: Re: [PATCH v10 3/3] sched/fair: Use candidate prev/recent_used CPU
 if scanning failed for cluster wakeup
Message-ID: <ZSluOlEbnrrNj2LV@chenyu5-mobl2.ccr.corp.intel.com>
References: <20231012121707.51368-1-yangyicong@huawei.com>
 <20231012121707.51368-4-yangyicong@huawei.com>
 <CAKfTPtAdtjZobtrf846kDzscAZTrFxw21SQJbbHU0Cw35vggEQ@mail.gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CAKfTPtAdtjZobtrf846kDzscAZTrFxw21SQJbbHU0Cw35vggEQ@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?bTBhq+75AZABRBovmC12+RqUef+rF7jcPCZv8Kti20ixR7E/Z2lczD2Atx1G?=
 =?us-ascii?Q?k3LWql8nCNbG0g04C5d1lTN1HP/4O/ZZ5T5hC6Hw4FlfF5FzjQtqj7oZEb6X?=
 =?us-ascii?Q?u8PAqZb3e9bsAh8AWm+a58AJEYQyLGSvjTGL0hPLQo1FWTDrZHNriKN5Rn3x?=
 =?us-ascii?Q?7FJQSezSstYo+d9IAvEC/A5CmnssjUrZ+BT1gADhqTFCgNgFEZ6AHPeLEr58?=
 =?us-ascii?Q?S5tBUw0w9ycXrPjgjleCsgGoYDOGa9c4QCVG6196BOppl6kgepc4+vlzLor3?=
 =?us-ascii?Q?v16R2jRAgYTsQKZG2ckFpGTmGAVIvGa+8n+z5ES8cb9DKiq1AT+xKJvNFw6C?=
 =?us-ascii?Q?vyCzuHYmxl7I7A0lsLhJte3DU7fzUCtqa3QbiK1hYuDXXmmV8dlyBzp856SC?=
 =?us-ascii?Q?UBL0LRFxcaw5cBUX3/79R5SUtU7r4qU8rGjSQ//UAklyz8iMYCBFXIE02sJJ?=
 =?us-ascii?Q?2X5KzyB7zae9baVwq6MwHRuJBwd7KZHMV3WcXXp7uPR0dioLO50zvudNxdi1?=
 =?us-ascii?Q?hPajYJGb8rXsed/Psgd1qChf+r8pAhWWXwtoMCEbkl2raz3Vy3/L0k4PNz4h?=
 =?us-ascii?Q?UQBskfSFwxBBRRqPDRajlEE8hqpGYlrcUsdfGQOmkqWBQoeGCm6WbGv90pAf?=
 =?us-ascii?Q?Ok5yAPBXmUSRVbUnGBKN6YlUNLZmMBEpaWdUQXttHzfSINn6Yq0F866EM/tK?=
 =?us-ascii?Q?wqwtEx74Br11TR9pYn5JuAFFGsC3Zx4tV+5G6ZBJ0pd3RLT2qvkLVYWROjKA?=
 =?us-ascii?Q?W//Y+U2VT5zLNew6W7XSfqiKHn3ouIwS9XPeCKk86XDxJBwhbC5dV7A6zKoV?=
 =?us-ascii?Q?nHe+qBPak8qmxgZ5GQ29ENEkd0SCeFbXBUXBPzirK1M6ZpoWCdLqgTunrRcY?=
 =?us-ascii?Q?C2reyUz3Rtg2HtQQKVJ+tZ7bR/J9M84W6jJ4NEYndRyL5HJXtr0tw55Q6PCw?=
 =?us-ascii?Q?GrJ902Fzr0D44vvI+Q7ByKrVgAScAjOZfIsOXFSLnwe7iofiV4Km5LIOKgbp?=
 =?us-ascii?Q?Yigh0yF9LlOOhnWqw7qm0IvP3GeQdW82+kztzsODGEfzwwfzS2PiJbFNu51E?=
 =?us-ascii?Q?tyWsf9F5KSdLrHWoiGpqETF6XvNm/2vr18c01FcWswRHQbmnD2hsI4m46OYE?=
 =?us-ascii?Q?S3LxS0FtLsw7NcBrtfLd6CnSdgyNFbjsJRE8GlC1tVPm9mTOzklkeaVtzVKZ?=
 =?us-ascii?Q?vgmjB/zGbbqJVVzTno/iUlED5WpM7H018U0Raq/TihW7mGTjXKgn6ZqIMRlQ?=
 =?us-ascii?Q?pbEVuAfp7B7cCQlBSFCvUS0ICpQ92FFS4fDo+rQiI/YksY8EE9u8LvvJ5yDT?=
 =?us-ascii?Q?WMr/ZU08dSu4X/aj4UKzeQhs416rzWS2OaJW/PLJQgMpIhNTPF8eFmt8dHi7?=
 =?us-ascii?Q?6qcjtiK0xcaW5tV6pvfQjbQb4+oGjsm/VKxsXZQg9PHKZ2Yko/NWP+K2dnEm?=
 =?us-ascii?Q?H9TGJmYzgpyxyBkx3cG3NvbQ831nEUuOCYp9/2ODOfTLl3RnEu0PB6IraTVz?=
 =?us-ascii?Q?tKWQ1HY5iTsMSCMiVFUmRQ104tbvgQd82PkFFNDqlfNPg2q8k3xuBDAmshpK?=
 =?us-ascii?Q?Znvn9+X/hUXqB/1gMuCLxbKw1O46yKwlqFg1UxiK?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 23340c1b-5baf-4324-89fa-08dbcc084fef
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Oct 2023 16:20:27.7988
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 9KUN7wamQU0soo1jQ4jgvGvwEbzY7u/qRZ5InwwDCLhqWDR/5dGyUJxkSOgQDeuNCZSgT1hACz929mwcD7pjhg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB7125
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Fri, 13 Oct 2023 09:22:03 -0700 (PDT)

On 2023-10-13 at 17:04:32 +0200, Vincent Guittot wrote:
> On Thu, 12 Oct 2023 at 14:19, Yicong Yang <yangyicong@huawei.com> wrote:
> >
> > From: Yicong Yang <yangyicong@hisilicon.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@intel.com>
> > Signed-off-by: Yicong Yang <yangyicong@hisilicon.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 ?
>

It aims to shrink the race window that the prev idle CPU becomes non-idle during above
select_idle_cpu() scan(maybe time consuming). And it wants to make a double check.

thanks,
Chenyu
 
> > +               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
> 
> 
> > +
> >         return target;
> >  }
> >
> > --
> > 2.24.0
> >