From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM10-BN7-obe.outbound.protection.outlook.com (mail-bn7nam10on2085.outbound.protection.outlook.com [40.107.92.85]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9084221D5AE for ; Fri, 20 Dec 2024 17:53:01 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=fail smtp.client-ip=40.107.92.85 ARC-Seal:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734717183; cv=fail; b=iMRgeLj1zGg7H9YLlN2BCRZoZmQJUQaWyhSHH0TaXP62hPQoLB94oL6lBY5pwvkKB34+GWzrJwHUD/av7nMxfnoMh4NGkQH4LGlzicgIgdStQj9CZRJPpWMEzFC3IaUyPO2m5etNPgkN2lI9izGpFTLH/peiEVhYCtxGaAKvOuI= ARC-Message-Signature:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734717183; c=relaxed/simple; bh=44TeSYDWDD2b57G+KQx9wjZptiTBt7Y5OXP6Sn0Yzqg=; h=Date:From:To:Cc:Subject:Message-ID:References:Content-Type: Content-Disposition:In-Reply-To:MIME-Version; b=oAPaa6Zb2NryOiJEQgkRq7mWMCcOU+lEqAwYyneZ67eishclk08Vliw3jkBXHZTVdzRVWvSyLF8DOfQ1KGl0w2W3Z5rQPKaXNQ0B8KAi2i0I5ZycH0jmBPyfjWnQwOJrIC4WxF6fQEI/3EyK9Ex3yH1/NLXDRwG8B7pjpe5S+lo= ARC-Authentication-Results:i=2; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=nvidia.com; spf=fail smtp.mailfrom=nvidia.com; dkim=pass (2048-bit key) header.d=Nvidia.com header.i=@Nvidia.com header.b=ldPXTJm9; arc=fail smtp.client-ip=40.107.92.85 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=nvidia.com Authentication-Results: smtp.subspace.kernel.org; spf=fail smtp.mailfrom=nvidia.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=Nvidia.com header.i=@Nvidia.com header.b="ldPXTJm9" ARC-Seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=v2gNmXPvGctFPRqpWSemDQpWGXy32bd+9i2+uBbxP4CVtKQ4pStX8tVyJ8F5f5JCy3BkDvpJreUGuEpc1dF52+MXTun8pQJnjIVZi6L64uKMHn5FOLq8uniNQ/Ulb/+q0gz1vRHZcxh5MkdYZuugkhvlwX28mcQNh5e2ZRCH1UEx2xFRhFp0ESSBszaD3t+RHE423ta92Y1GUzrbCmk93mgQKiAoDcGWTCXkC94LSgMvsMlEPyoenJx/GOnPBtV/4KnwwvXNuh++m2IuU0iWoFKNwWhT8Ih0LLImOJpHVd5wSQHLRWy3WRAovnV2JgMATahVGcG34zcoGXnjCccvpg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=uTSBw3NM2CNzz+i3kgHI/ZBWygXc1OSK0kKJ0eqVT44=; b=O1JiWtOnW12+TP/qVVnOr9oPotqnAC5jb6z3YL6dSjSX4Urv3SGlg6HLA00E5f+jTnynzcsAGWzkXfPFN2zGwi7ypdy6OrzeQ7Jyimm2lrfNbjHPdLJ3Od1/37lWOSORAX1e0sxMALnYS7/oUsTGRev/XsWFkgrYsdh+Ue84u8XgjxGzDoJ7fu8lcn8N2QSQYqsJ0w4swf2HbOGX932DzMJ4G89j3p9HGGUrRLuQ5TKF7c7hyp6e31xzInZ/97ZPEak2Ydpuhjo9Ca1Yss5ZIPLw7/gTra1GiVG8ZLvyY9xG13ixTHr+S0EunOkgK6UFTO/d7ZSqe8N7MPuSPAmxxQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nvidia.com; dmarc=pass action=none header.from=nvidia.com; dkim=pass header.d=nvidia.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Nvidia.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=uTSBw3NM2CNzz+i3kgHI/ZBWygXc1OSK0kKJ0eqVT44=; b=ldPXTJm95KnMzwgsvi0Y4WhQA8t/K81B6ue9VYO4yPkYy6Vcd5YUtuk6zJLqoGegIbqQbo75Sm2/LIrCoBFNZWBq55ekc8cCHsozcys6HyHMku/xDWTnJEzKgtuggi5VFqGHLFphuL94TolufafLXbd3w/+fUS/vJJndswZiCVTmIbqqocjCaG3jP5fkyAzlBrdS4HSZ0ccXSgEFgFTDymD9Ep13O/ffSSilQz9E6iAuf8C9ZpAaNPB7tyCLk2dFVUYvTHAQWGbHdFBR8bKpt6Cessk+AuTCnL8FZ7+Cngp4AKnk/mSr8KdGAl5l/tVzhVP0SmD2TGTAzqSvH7da8Q== Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=nvidia.com; Received: from CY5PR12MB6405.namprd12.prod.outlook.com (2603:10b6:930:3e::17) by DM4PR12MB7696.namprd12.prod.outlook.com (2603:10b6:8:100::10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.8272.14; Fri, 20 Dec 2024 17:52:58 +0000 Received: from CY5PR12MB6405.namprd12.prod.outlook.com ([fe80::2119:c96c:b455:53b5]) by CY5PR12MB6405.namprd12.prod.outlook.com ([fe80::2119:c96c:b455:53b5%3]) with mapi id 15.20.8272.005; Fri, 20 Dec 2024 17:52:58 +0000 Date: Fri, 20 Dec 2024 18:52:53 +0100 From: Andrea Righi To: Yury Norov Cc: Tejun Heo , David Vernet , Changwoo Min , Ingo Molnar , Peter Zijlstra , Juri Lelli , Vincent Guittot , Dietmar Eggemann , Steven Rostedt , Ben Segall , Mel Gorman , Valentin Schneider , Rasmus Villemoes , linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/6] sched_ext: Introduce per-node idle cpumasks Message-ID: References: <20241217094156.577262-1-arighi@nvidia.com> <20241217094156.577262-4-arighi@nvidia.com> Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-ClientProxiedBy: FR5P281CA0022.DEUP281.PROD.OUTLOOK.COM (2603:10a6:d10:f1::8) To CY5PR12MB6405.namprd12.prod.outlook.com (2603:10b6:930:3e::17) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: CY5PR12MB6405:EE_|DM4PR12MB7696:EE_ X-MS-Office365-Filtering-Correlation-Id: 4e6c68cd-02f3-4240-dc78-08dd211f2395 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0;ARA:13230040|376014|7416014|1800799024|366016; X-Microsoft-Antispam-Message-Info: =?us-ascii?Q?polTpsQWoNGxFT054g0uncR9eKl0XYBFpatkhTr8YMIaS3vj1o7qGpmPssvX?= =?us-ascii?Q?2lnJfh7rhgODXbftRrS1ZSexPp56kLIK5vluwqzTepiyVKfPpd0ubNIiOp9k?= =?us-ascii?Q?S8hjeUcyn18ZvmTSFD+nGDo5y7iLdmVrsY/sbHzPFjK2FrzgeC55AVytjxu+?= =?us-ascii?Q?ORTl+ntKEPLmlI9VXZOmsTP8cNxr+gO7iZv6UBDs+PHoqhnhYagk0Qj5eCkv?= =?us-ascii?Q?a55YWhmo3pi+am3sA47jxDAdSk8tCkGD9XsCe7R68hXY6gXxWfrCJIyVMD6X?= =?us-ascii?Q?grasnMajpkwt2DVW7UCjz42v7gyYVbtZtN/ps4KeF3xozMXYZhFwaAHLUBPd?= =?us-ascii?Q?rTSS7G4aIrzgvjx4G7OCw61IUQR/7junjziJZslB57mD3Ix+MJPEkg3GScXY?= =?us-ascii?Q?T4Gj9Qgs3PkFOfumtGMg4HdqqAYfKFWuZBkZydhQNptdQBWIDv7M5wQ3nXRG?= =?us-ascii?Q?2vefvko+Ctwej2oSzf5ZW1m8KhwRyfv0UEhyvrhceJT0GAsOngAhv2LtEK7a?= =?us-ascii?Q?aqeo449L+ay8mEsHYm92s5X2vmCWb2nLzko3QYkzf2cTfvjUcikydJjvYA42?= =?us-ascii?Q?mXmwYg1j5mT2lDXteCO4hvIaUqqMHzYIhbV2XCBYQ3VC5P151jrKdNMhh4f3?= =?us-ascii?Q?RLkimCwnEudFGtcH15G1LGcsUJOyaumXLDF/fOvvV23ottAYwBmDoXyLj+EZ?= =?us-ascii?Q?XVrETgsdwNNPbAv+8JCZW56pMomYuCJiwAEHQaSvR4pgx3aiVzzhOx5RCSg5?= =?us-ascii?Q?a6btR66t2ewmlOnMYVCg8spqSUkbYCMApYLN4wuezg3ijp7h1399Mn37YfNO?= =?us-ascii?Q?NjANTl7ks7YDITNH2O9nKVwRUzbH5umx7qe19+yiWRPjSP26CP0gl+DDF2uY?= =?us-ascii?Q?unhZKHMH0ItNXVv5shqxNu4ohMZ8DHYWk/Uu0zxDHJt9/k+lCl3R1xuEdUq0?= =?us-ascii?Q?QxblbUb1oNPekkTxufb3JYd/U8yYWlWTDhw8fvmOxTRYynCsilMWKbtL5BWq?= =?us-ascii?Q?351yiUyvqzbi9wUGw6YFWNQ+m9Ekc6aLhq3w88WNmb/zHNB4umtEFxB3f80N?= =?us-ascii?Q?RPu8aegvPSAf+mViBQGIDioryzea0VPnFJtUqrR2IV7Ci+z4QYTU9G3NSBPO?= =?us-ascii?Q?+DdIytoB/4dmYYLcSiKQdCi5zztXWC0p3D7a/fplpqiLIBsWP8KoWR3Px8Zh?= =?us-ascii?Q?UXG3B/GyJ6DTedboSp2Gv3tCjH3Q9Wtx/7CluO7BEfEiPi4ZwFexuECZq5Pv?= =?us-ascii?Q?d4XmCNZeNZI2P8l5yxaP0IsbQ0I9qJo0DVVqk4AN8nhf/mpWxpOyJiOekMDU?= =?us-ascii?Q?R2BridPuenItOcjNJr+TRI2F+PdZMY+SbRW4FVlcXsMm5uP3HbQiCXQP0MhB?= =?us-ascii?Q?CkmiBqH++0hV+Fxw4/b0+zvuYOcc?= X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:CY5PR12MB6405.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230040)(376014)(7416014)(1800799024)(366016);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?+trp7YIswiHGd3H/KhTk5bwCqCGW8GVCQmnNPFgsZBlqqC0SFzWXDM1QYpR4?= =?us-ascii?Q?O5OpfHdjysdAcyf7Y+83pidae66DuJagdBAThwNLC3ma3BDhPcnWibL8BN2K?= =?us-ascii?Q?tCyVDsj409JckqCvCGU3cF2/MumeJzUpNYt1QuJ9fUMUoqil3vxghPrvfNPm?= =?us-ascii?Q?6V5iE3yfuw6EIJCinenkS3H7V9j5CjhsH44k4NfsthHEcfwxY5QT1yo+nRXM?= =?us-ascii?Q?KpDdHkpLbxVZngDI/ohwXdFtGR+UxUBlh0DUG6lMf01NmPpzLgaeWdp96fPK?= =?us-ascii?Q?/+nSPl8pgQcyqqw49nSBQ4BBLccezI0xZ0xhjhXOz6tnTJESwYGBH5awkQKy?= =?us-ascii?Q?mSyrvys72MHCsUsTGvSsgvGnzv674HflnZF2XyTJpn16CD3NnrClmEqnG1VL?= =?us-ascii?Q?p//4h1tn8w1jo7iyY0u4E6Kaw48uz5OcbvXQ3RiKNe0V+9UIOwdzF0wStl+x?= =?us-ascii?Q?zZA0pxupN4WlsbhbzyC/JJHGSMT8kyiG+v8RK7ELGRx5GjtwJMxTTaAorRgC?= =?us-ascii?Q?9vW3JDFttccM9UoV1+S7wmA1nfOw++DCiPN0QWf7O3rZStIRuFe4MmiabB0s?= =?us-ascii?Q?DWuk80buiaGirZLWfsZtJvGdkq+gkDVySppB7+gw7Oge7FUQz7MuihVrEfb3?= =?us-ascii?Q?JFskejqWERFnCR/KwsOEpRGEJcxD9+rpXZG9WBvkA/lXS0/mm/cgzab2YSWJ?= =?us-ascii?Q?7sikc7A5QOkHgvQf6UtcvNFi0r0IXC6zPL6oytryKRlL5ldcLUoBhWleU1lt?= =?us-ascii?Q?qah5i93z7amPVyuFnBcYp8yoQG532mHXmXbS+TuGfovdk/N2hZ3/fCw/HmAs?= =?us-ascii?Q?zhROwJ1KG4crULITxGnAl2XU3dV6xIgvIY5LSI/KjD6jn0cJaFoJeE71bClH?= =?us-ascii?Q?5DlHlU3DyvzopkLMQvN2BpjNhkajr1C1bmez6bjHSgbGpWu/3zP1Guhzsnst?= =?us-ascii?Q?Bw6vrrMF2j+OSGwXX85rsBpJ/FKj9sYzCh5Uh2sTRiqeOmXjMz5S25br3mUn?= =?us-ascii?Q?rUQ01oJJwbOh1UStUgoJAr4otjsR0lKTnDbfr/HnCXFfGbMcDLUjFrPpnsEf?= =?us-ascii?Q?r7cmc+WiwMvXz54G53FT48mU8XDggLOgyF9isbfKS59dNI6uZXtWcJtzXe0R?= =?us-ascii?Q?oBBo+CcdUz6fO7wLTyQQttoSYTZJq1F8np26gtKVkzep+TGuunazBHXjc3uz?= =?us-ascii?Q?4wc2UKULd75KWfrJgJkfU8tYAwx/KPGfKzCLsPr9c0pHanhPn240zjRfFnxv?= =?us-ascii?Q?brSmsSYsUPUxgH4Tuw88SbWcD8YVSLDoQ5pRJtYqRAQEqSdV5XWYr6Z3//MP?= =?us-ascii?Q?dbQRmWlh/epxvWyE7WjMmDXJInnMh1BagfeiV1XFH+Bm1D/aGafstlw9CXFL?= =?us-ascii?Q?Qiufb4IeJ8YTz5CoI2QE09ac3Fsvgf415fVSdf9BN+2WscMGCCANHriqscOb?= =?us-ascii?Q?tai1sajSQuuLZgo/ZIgEF1ts9LnYsHkI0lymKd4e5QAoLuGjZXb1Nv8+jhxY?= =?us-ascii?Q?34OcGNDPgqZtgJzrtsH3dThbOqp1/r3jjtYS/Hpew06r4bxq+SWIrxd5p3L9?= =?us-ascii?Q?YtGSC7t1tvgQmZoUG6AALGm218+p2a3L9dfjqsoZ?= X-OriginatorOrg: Nvidia.com X-MS-Exchange-CrossTenant-Network-Message-Id: 4e6c68cd-02f3-4240-dc78-08dd211f2395 X-MS-Exchange-CrossTenant-AuthSource: CY5PR12MB6405.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Dec 2024 17:52:58.3815 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: j0SA2lZo4JcILWI+/tsU37/I96gv2f7gVgasV+bGVRsyjxlwMiuVmWxGuakUZdomKq+zVpdCYZP0RST13RKHjg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB7696 On Fri, Dec 20, 2024 at 08:48:55AM -0800, Yury Norov wrote: > On Tue, Dec 17, 2024 at 10:32:28AM +0100, Andrea Righi wrote: > > Using a single global idle mask can lead to inefficiencies and a lot of > > stress on the cache coherency protocol on large systems with multiple > > NUMA nodes, since all the CPUs can create a really intense read/write > > activity on the single global cpumask. > > > > Therefore, split the global cpumask into multiple per-NUMA node cpumasks > > to improve scalability and performance on large systems. > > > > The concept is that each cpumask will track only the idle CPUs within > > its corresponding NUMA node, treating CPUs in other NUMA nodes as busy. > > In this way concurrent access to the idle cpumask will be restricted > > within each NUMA node. > > > > NOTE: with per-node idle cpumasks enabled scx_bpf_get_idle_cpu/smtmask() > > are returning the cpumask of the current NUMA node, instead of a single > > cpumask for all the CPUs. > > > > Signed-off-by: Andrea Righi > > --- > > kernel/sched/ext.c | 281 +++++++++++++++++++++++++++++++++------------ > > 1 file changed, 209 insertions(+), 72 deletions(-) > > > > diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c > > index a17abd2df4d4..d4666db4a212 100644 > > --- a/kernel/sched/ext.c > > +++ b/kernel/sched/ext.c > > @@ -894,6 +894,7 @@ static DEFINE_STATIC_KEY_FALSE(scx_builtin_idle_enabled); > > #ifdef CONFIG_SMP > > static DEFINE_STATIC_KEY_FALSE(scx_selcpu_topo_llc); > > static DEFINE_STATIC_KEY_FALSE(scx_selcpu_topo_numa); > > +static DEFINE_STATIC_KEY_FALSE(scx_builtin_idle_per_node); > > #endif > > > > static struct static_key_false scx_has_op[SCX_OPI_END] = > > @@ -937,11 +938,82 @@ static struct delayed_work scx_watchdog_work; > > #define CL_ALIGNED_IF_ONSTACK __cacheline_aligned_in_smp > > #endif > > > > -static struct { > > +struct idle_cpumask { > > cpumask_var_t cpu; > > cpumask_var_t smt; > > -} idle_masks CL_ALIGNED_IF_ONSTACK; > > +}; > > + > > +/* > > + * Make sure a NUMA node is in a valid range. > > + */ > > +static int validate_node(int node) > > +{ > > + /* If no node is specified, return the current one */ > > + if (node == NUMA_NO_NODE) > > + return numa_node_id(); > > + > > + /* Make sure node is in the range of possible nodes */ > > + if (node < 0 || node >= num_possible_nodes()) > > + return -EINVAL; > > + > > + return node; > > +} > > This is needed in BPF code, right? Kernel users should always provide > correct parameters. This should be used only in the kfuncs, I should probably move this down to the BPF helpers section. For internal kernel functions we may want to WARN_ON_ONCE(). > > > +/* > > + * cpumasks to track idle CPUs within each NUMA node. > > + * > > + * If SCX_OPS_BUILTIN_IDLE_PER_NODE is not specified, a single flat cpumask > > + * from node 0 is used to track all idle CPUs system-wide. > > + */ > > +static struct idle_cpumask **idle_masks CL_ALIGNED_IF_ONSTACK; > > + > > +static struct cpumask *get_idle_mask_node(int node, bool smt) > > Like Rasmus said in another thread, this 'get' prefix looks weird. > > > +/** > > + * get_parity8 - get the parity of an u8 value > > I know it's prototypical bikeshedding, but what's with the "get_" > prefix? Certainly the purpose of any pure function is to "get" the > result of some computation. We don't have "get_strlen()". > > Please either just "parity8", or if you want to emphasize that this > belongs in the bit-munging family, "bit_parity8". > > So maybe idle_nodes(), idle_smt_nodes() and so on? It's a bit different here, because in theory we get a "reference" to the object (even if we're not refcounting it), it's not a pure function. But I'm also totally fine to get rid of the "get_" prefix. > > > +{ > > + if (!static_branch_maybe(CONFIG_NUMA, &scx_builtin_idle_per_node)) > > + return smt ? idle_masks[0]->smt : idle_masks[0]->cpu; > > + > > + node = validate_node(node); > > + if (node < 0) > > + return cpu_none_mask; > > cpu_none_mask is a valid pointer. How should user distinguish invalid > parameter and empty nodemask? You should return -EINVAL. > > Anyways... > > This would make a horrid mess. Imagine I'm your user, and I'm > experimenting with this idle feature. I decided to begin with > per-node idle masks disabled and call it for example like: > > get_idle_mask_node(random(), true) > > And it works! I get all my idle CPUs just well. So I'm happily build > my complex and huge codebase on top of it. > > Then one day I decide to enable those fancy per-node idle masks > because you said they unload interconnect and whatever... At that > point what I want is to click a button just to collect some perf > numbers. So I do that, and I find all my codebase coredumped in some > random place... Because per-node idle masks basic functions > simply have different interface: they disallow node = random() as > parameter, while global idle mask is OK with it. So, the idea here is to use validate_node() to catch incorrect nodes provided by the user (scx scheduler) and trigger an scx_ops_error(), which will force the scheduler to exit with an error. In this context cpu_none_mask serves as a "temporary harmless value" that can be returned to the caller, that will just exit. Again, we could add a sanity check for the kernel code here as well, like a WARN_ON_ONCE() to catch potential incorrect values that might be used by other internal kernel functions (and still return cpu_none_mask). > > > + > > + return smt ? idle_masks[node]->smt : idle_masks[node]->cpu; > > +} > > + > > +static struct cpumask *get_idle_cpumask_node(int node) > > inline or __always_inline? Ok. > > + > > +static s32 > > +scx_pick_idle_cpu_numa(const struct cpumask *cpus_allowed, s32 prev_cpu, u64 flags) > > We have a sort of convention here. If you pick a cpu based on NUMA > distances, the 'numa' should be a 2nd prefix after a subsystem where > it's used. Refer for example: > > sched_numa_find_nth_cpu() > sched_numa_hop_mask() > for_each_numa_hop_mask() > > So I'd name it scx_numa_idle_cpu() Ah, that's good to know, thanks! (the _numa variant has been removed in v8, but it's still good to know) > > > +{ > > + nodemask_t hop_nodes = NODE_MASK_NONE; > > + int start_node = cpu_to_node(prev_cpu); > > + s32 cpu = -EBUSY; > > + > > + /* > > + * Traverse all online nodes in order of increasing distance, > > + * starting from prev_cpu's node. > > + */ > > + rcu_read_lock(); > > RCU is a leftover from for_each_numa_hop_mask(), I guess? Correct! Already removed in the next version. > > > + for_each_numa_hop_node(node, start_node, hop_nodes, N_ONLINE) { > > + cpu = scx_pick_idle_cpu_from_node(node, cpus_allowed, flags); > > + if (cpu >= 0) > > + break; > > + } > > + rcu_read_unlock(); > > + > > + return cpu; > > +} > > + > > +static s32 scx_pick_idle_cpu(const struct cpumask *cpus_allowed, s32 prev_cpu, u64 flags) > > +{ > > + /* > > + * Only node 0 is used if per-node idle cpumasks are disabled. > > + */ > > + if (!static_branch_maybe(CONFIG_NUMA, &scx_builtin_idle_per_node)) > > + return scx_pick_idle_cpu_from_node(0, cpus_allowed, flags); > > + > > + return scx_pick_idle_cpu_numa(cpus_allowed, prev_cpu, flags); > > } > > > > /* > > @@ -3339,7 +3453,7 @@ static bool llc_numa_mismatch(void) > > * CPU belongs to a single LLC domain, and that each LLC domain is entirely > > * contained within a single NUMA node. > > */ > > -static void update_selcpu_topology(void) > > +static void update_selcpu_topology(struct sched_ext_ops *ops) > > { > > bool enable_llc = false, enable_numa = false; > > unsigned int nr_cpus; > > @@ -3360,6 +3474,14 @@ static void update_selcpu_topology(void) > > if (nr_cpus > 0) { > > if (nr_cpus < num_online_cpus()) > > enable_llc = true; > > + /* > > + * No need to enable LLC optimization if the LLC domains are > > + * perfectly overlapping with the NUMA domains when per-node > > + * cpumasks are enabled. > > + */ > > + if ((ops->flags & SCX_OPS_BUILTIN_IDLE_PER_NODE) && > > + !llc_numa_mismatch()) > > + enable_llc = false; > > pr_debug("sched_ext: LLC=%*pb weight=%u\n", > > cpumask_pr_args(llc_span(cpu)), llc_weight(cpu)); > > } > > @@ -3395,6 +3517,14 @@ static void update_selcpu_topology(void) > > static_branch_enable_cpuslocked(&scx_selcpu_topo_numa); > > else > > static_branch_disable_cpuslocked(&scx_selcpu_topo_numa); > > + > > + /* > > + * Check if we need to enable per-node cpumasks. > > + */ > > + if (ops->flags & SCX_OPS_BUILTIN_IDLE_PER_NODE) > > + static_branch_enable_cpuslocked(&scx_builtin_idle_per_node); > > + else > > + static_branch_disable_cpuslocked(&scx_builtin_idle_per_node); > > } > > > > /* > > @@ -3415,6 +3545,8 @@ static void update_selcpu_topology(void) > > * 4. Pick a CPU within the same NUMA node, if enabled: > > * - choose a CPU from the same NUMA node to reduce memory access latency. > > * > > + * 5. Pick any idle CPU usable by the task. > > + * > > I would make it a separate patch, or even send it independently. It > doesn't look related to the series... Right, it's a separate preparation patch in the next version. But I'll probably send this as a totally separate patch. > > static void reset_idle_masks(void) > > { > > + int node; > > + > > + if (!static_branch_maybe(CONFIG_NUMA, &scx_builtin_idle_per_node)) { > > + cpumask_copy(get_idle_cpumask_node(0), cpu_online_mask); > > + cpumask_copy(get_idle_smtmask_node(0), cpu_online_mask); > > + return; > > + } > > + > > /* > > * Consider all online cpus idle. Should converge to the actual state > > * quickly. > > */ > > - cpumask_copy(idle_masks.cpu, cpu_online_mask); > > - cpumask_copy(idle_masks.smt, cpu_online_mask); > > + for_each_node_state(node, N_POSSIBLE) { > > for_each_node(node) > > If the comment above is still valid, you don't need to visit every > possible node. You need to visit only N_ONLINE, or even N_CPU nodes. > > This adds to the discussion in patch #1. Taking care of CPU-less nodes > is useless for the scheduler purposes. And if you don't even initialize > them, then in for_each_numa_hop_node() you should skip those nodes. Don't we need to worry about nodes going online/offline and dealing with mem_hotplug_lock as mentioned by Tejun? Maybe it's safer to initialize all of them here and visit the N_CPU nodes when looking for idle CPUs? Or am I missing something? Or maybe we could even initialize and navigate the N_CPU nodes and trigger a scheduler restart on hotplug events if SCX_OPS_BUILTIN_IDLE_PER_NODE is enabled... > > > + const struct cpumask *node_mask = cpumask_of_node(node); > > + struct cpumask *idle_cpu = get_idle_cpumask_node(node); > > + struct cpumask *idle_smt = get_idle_smtmask_node(node); > > + > > + cpumask_and(idle_cpu, cpu_online_mask, node_mask); > > + cpumask_copy(idle_smt, idle_cpu); > > + } > > } > > > > void __scx_update_idle(struct rq *rq, bool idle) > > { > > int cpu = cpu_of(rq); > > + int node = cpu_to_node(cpu); > > + struct cpumask *idle_cpu = get_idle_cpumask_node(node); > > > > if (SCX_HAS_OP(update_idle) && !scx_rq_bypassing(rq)) { > > SCX_CALL_OP(SCX_KF_REST, update_idle, cpu_of(rq), idle); > > @@ -3661,27 +3810,25 @@ void __scx_update_idle(struct rq *rq, bool idle) > > return; > > } > > > > - if (idle) > > - cpumask_set_cpu(cpu, idle_masks.cpu); > > - else > > - cpumask_clear_cpu(cpu, idle_masks.cpu); > > + assign_cpu(cpu, idle_cpu, idle); > > Can you also make it a separate preparation patch? Yep, done in the next version. > > > > > #ifdef CONFIG_SCHED_SMT > > if (sched_smt_active()) { > > const struct cpumask *smt = cpu_smt_mask(cpu); > > + struct cpumask *idle_smt = get_idle_smtmask_node(node); > > > > if (idle) { > > /* > > - * idle_masks.smt handling is racy but that's fine as > > - * it's only for optimization and self-correcting. > > + * idle_smt handling is racy but that's fine as it's > > + * only for optimization and self-correcting. > > */ > > for_each_cpu(cpu, smt) { > > - if (!cpumask_test_cpu(cpu, idle_masks.cpu)) > > + if (!cpumask_test_cpu(cpu, idle_cpu)) > > return; > > } > > So, we continue only if all smt cpus are idle. This looks like an > opencoded cpumask_subset(): > > if (!cpumask_subset(idle_cpu, smt)) > return; > > Is that true? If so, can you please again make it a small > preparation patch? Good point, if all the CPUs in smt are all idle (== smt is a subset of idle_cpu), we want to set all the smt bits in idle_smt. So I think the following should be correct: if (!cpumask_subset(smt, idle_cpu)) return; cpumask_or(idle_smt, idle_smt, smt); Will test this & make a preparation patch. Thanks! -Andrea