Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 21 Oct 2022 19:14:47 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     Abel Wu <wuyun.abel@bytedance.com>
CC:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@kernel.org>, "Mel Gorman" <mgorman@suse.de>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Valentin Schneider <valentin.schneider@arm.com>,
        Josh Don <joshdon@google.com>,
        Tim Chen <tim.c.chen@linux.intel.com>,
        K Prateek Nayak <kprateek.nayak@amd.com>,
        "Gautham R . Shenoy" <gautham.shenoy@amd.com>,
        Aubrey Li <aubrey.li@intel.com>,
        Qais Yousef <qais.yousef@arm.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Rik van Riel <riel@surriel.com>,
        Yicong Yang <yangyicong@huawei.com>,
        Barry Song <21cnbao@gmail.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 3/4] sched/fair: Introduce SIS_CORE
Message-ID: <Y1J/JzKNOx4lQfG9@chenyu5-mobl1>
References: <20221019122859.18399-1-wuyun.abel@bytedance.com>
 <20221019122859.18399-4-wuyun.abel@bytedance.com>
 <Y1IZ/DZ1SWMn0QDs@chenyu5-mobl1>
 <9ec8a474-d923-953c-0b73-02ba2fd6ea82@bytedance.com>
 <Y1IhQPWMX3TX+sYo@chenyu5-mobl1>
 <03be9c79-bc59-cd4a-869b-ed4c85c61224@bytedance.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <03be9c79-bc59-cd4a-869b-ed4c85c61224@bytedance.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?9jCwkN7/ptkowvjNJA5w92mOm0+DvBtjxQT3LGC+Pg0Sqn6ifnE75JpdJR83?=
 =?us-ascii?Q?fKrBlQEz+Z+V6qL/uwCDOwTfkOIG+qPt5gaTOZstq77zzOmXb5fmgciXOOQD?=
 =?us-ascii?Q?kPsvkrRBPAkwpoN9QjAfAb621joNRAb2gxR3X4XR+BE/j4kEdFbFYGmlCe6J?=
 =?us-ascii?Q?AG3fQX+n6UbEqaG6YTfrS8dr9z5fyF27Ayydj2TGKB/HtryXaE5AmswzI2er?=
 =?us-ascii?Q?PVoFidpBel+tRF1PsOmqLZ3fNN8KgaepBH+EsnqHTSjbeqtTjR+Gpr+oiu8D?=
 =?us-ascii?Q?qjLe0XBhkiMm70IWMBeRND/G5jRb3MLdjxY+qSlhV0rqz1KfDa4H3eMHvcCr?=
 =?us-ascii?Q?xHcjS/t96HucCMQQDBT+SbJ3XjVfLh8sMDyo3hAtg+uKBgmwwwBnmxJHpi2y?=
 =?us-ascii?Q?/CjieXV/p3D0ar5pyyzpOAG16mK4euK055t6pBi6KCw58bEaR60YqlSWjr1V?=
 =?us-ascii?Q?dhC/YzCMijGcOlJucPErr5C+ZhD90HERCSPys7vqeEjwieSlvrHPBwkZKK8N?=
 =?us-ascii?Q?sCXQAKytScZCBWuEHvetXOx/49hVSfkzt+z1tt1zMu0PDqvaB0b9kTkZ5W4K?=
 =?us-ascii?Q?poXuipBWxo9Z6iD+wQo3hOHxxRoq62aS2lrlr6ivY5w7JyC5o0/R+C7a/wMA?=
 =?us-ascii?Q?11LB0iNmsIiPlh34wK0ZfC5ejQEWBLS9g2YOoqBwYjRHYBbGI++HwLRYgXLx?=
 =?us-ascii?Q?2HT9yPWyGJRzrIHUoyBtDniADrbXnxeYUvlZuq/ImP5iu6ghFoJ0bNjrHR3g?=
 =?us-ascii?Q?NKglOjkBtQCG1WdXOqDcfwFmYNH2npqNrGQ3UmaA5XpIuOV2HTHKKkPCnISF?=
 =?us-ascii?Q?zRgyw90r18IhwBs1pNGvaNxdXJH8AQrjNhEamH0xwiC9IFzVwczWnoYNJw6i?=
 =?us-ascii?Q?1Lfk5g5YJ3bB3xdk/upxfgmzcFwsAnlyoDI0fkKQZ9tyUxUTeZ1TWZNDbjEp?=
 =?us-ascii?Q?l3KpR5g/0rPDQA5icMftEQbt0sY3mV8+w9u3Rq9GMxdhdxsKShwzFvJRfNqr?=
 =?us-ascii?Q?9jV5KnyBFRfXyO5H0SsfwIdQcVWPFxgoHujgfHZbJPMgYJ5Y5ZPQz64f7r9j?=
 =?us-ascii?Q?ngih/F1r5LaDKQeip0oH9bf8lgCcVP7ypPyUFRWqNHEW+WKv9Tz+MSJCztnu?=
 =?us-ascii?Q?e9ZYce8ybtakM6W5ymFpSUNKrcfPXYJakS3qqFg1WtdR9TvaXanCjRzPMUw7?=
 =?us-ascii?Q?/dtJHljhfMUhqQbr/8PXUPA7/NwDm1uOiIvDIiRbWOvZYK5Ii9p611K6za1O?=
 =?us-ascii?Q?/5D3zKNK5Th+eZUABSh+uXwZnMnqi62yGf/pa9m5mXs9Bc4pctlFte8Jvc8O?=
 =?us-ascii?Q?rToNg7B9ArlcvuRwmzcsa6fB/b+//no9IyXdEHSSgi09z783DJZ5oAVRsdzH?=
 =?us-ascii?Q?IL2oNoZ9zT6UCLXm6h6R8y6669ukvZtrdppHBrM/FR6VFhD64JtPA7r49eLz?=
 =?us-ascii?Q?vowPnDVONQe1dA/7vIqLdf7loLhrhZvrJYy9gMg7Z7mlu00nWuig+IN1fzt9?=
 =?us-ascii?Q?+aesIV5VK591iQpMDC30mT0llwXFIPgEiALeRZNmrdE7lkbNFaOabpoCe38v?=
 =?us-ascii?Q?ebjupMDvx5yTbkwg6o/rlHDEm/G34K/zd3fhSQ/M?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 061e0a4b-18e8-4af8-378b-08dab35581ec
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Oct 2022 11:15:06.1710
 (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: TyNeJRhTvInUOIfojMvDnt7R8V96+Fg9ug7f5dO8vcuLuUTbl7dyvX+UsOYNsrP75euNQ13avP+B4BFC+K4Gxg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB6732
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_PASS,SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2022-10-21 at 17:35:06 +0800, Abel Wu wrote:
> On 10/21/22 12:34 PM, Chen Yu wrote:
> > On 2022-10-21 at 12:30:56 +0800, Abel Wu wrote:
> > > Hi Chen, thanks for your reviewing!
> > > 
> > > On 10/21/22 12:03 PM, Chen Yu wrote:
> > > > On 2022-10-19 at 20:28:58 +0800, Abel Wu wrote:
> > > > [cut]
> > > > > A major concern is the accuracy of the idle cpumask. A cpu present
> > > > > in the mask might not be idle any more, which is called the false
> > > > > positive cpu. Such cpus will negate lots of benefit this feature
> > > > > brings. The strategy against the false positives will be introduced
> > > > > in next patch.
> > > > > 
> > > > I was thinking that, if patch[3/4] needs [4/4] to fix the false positives,
> > > > maybe SIS_CORE could be disabled by default in 3/4 but enabled
> > > > in 4/4? So this might facilicate git bisect in case of any regression
> > > > check?
> > > 
> > > Agreed. Will fix in next version.
> > > 
> > > > [cut]
> > > > > + * To honor the rule of CORE granule update, set this cpu to the LLC idle
> > > > > + * cpumask only if there is no cpu of this core showed up in the cpumask.
> > > > > + */
> > > > > +static void update_idle_cpu(int cpu)
> > > > > +{
> > > > > +	struct sched_domain_shared *sds;
> > > > > +
> > > > > +	if (!sched_feat(SIS_CORE))
> > > > > +		return;
> > > > > +
> > > > > +	sds = rcu_dereference(per_cpu(sd_llc_shared, cpu));
> > > > > +	if (sds) {
> > > > > +		struct cpumask *icpus = to_cpumask(sds->icpus);
> > > > > +
> > > > > +		/*
> > > > > +		 * This is racy against clearing in select_idle_cpu(),
> > > > > +		 * and can lead to idle cpus miss the chance to be set to
> > > > > +		 * the idle cpumask, thus the idle cpus are temporarily
> > > > > +		 * out of reach in SIS domain scan. But it should be rare
> > > > > +		 * and we still have ILB to kick them working.
> > > > > +		 */
> > > > > +		if (!cpumask_intersects(cpu_smt_mask(cpu), icpus))
> > > > > +			cpumask_set_cpu(cpu, icpus);
> > > > Maybe I miss something, here we only set one CPU in the icpus, but
> > > > when we reach update_idle_cpu(), all SMT siblings of 'cpu' are idle,
> > > > is this intended for 'CORE granule update'?
> > > 
> > > The __update_idle_core() is called by all the cpus that need to go idle
> > > to update has_idle_core if necessary, and update_idle_cpu() is called
> > > before that check.
> > > 
> > I see.
> > 
> > Since __update_idle_core() has checked all SMT siblings of 'cpu' if
> > they are idle, can that information also be updated to icpus?
> 
> I think this will simply fallback to the original per-cpu proposal and
> lose the opportunity to spread tasks to different cores.
OK, make sense.

thanks,
Chenyu