From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (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 3CEE91F8ACD for ; Tue, 17 Dec 2024 23:22:27 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734477749; cv=none; b=cG5jw9kMNea/3CcyFHvl6ank4tcoej9lJVIzYGRU9WhBmzz4GYwKxJ+rTgqHU0t4xTbNgqrbBXORzVQS68Qa9X+4zXaGynF4FLtExO3t4yyliILBGI/besHDRE4wvY3nw92XRhhKS+d6AwcoALtMmcoVd5mrtgJ0oSYFmw6pkVM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734477749; c=relaxed/simple; bh=Fe0MLI5IfT8cZkZVnIb509bqpITvLPwLBQz6dBSenik=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ktgMw+uoSeKwrEZlMaL0hGEyBjL9GMwkAEIk3H5NYDZ9jstT6+1WGvPcJliQQu9y8h6NnzI1RoIcFe2HBdkmCMxi0fvzBlZv9rgfO+lupvRPYulgsC23XUlPnKAkosPMa6vswpebAUNY9l8hDcwYbM5YAlqzoXqyABOiFUgHCyI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=MRQ2p5gs; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="MRQ2p5gs" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 82AA0C4CED3; Tue, 17 Dec 2024 23:22:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1734477747; bh=Fe0MLI5IfT8cZkZVnIb509bqpITvLPwLBQz6dBSenik=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=MRQ2p5gsILY+yef/572mZ1nTjPK+GnuoXIN/uhNibgyk6Xu6RoozSy4dDTNlqxtVe lOn/G4x6DsbUFY1kWQBKW8qHrsdHv/+OdeGN2isC1Nspcg9HPyKLH9OXxOUjZ0Omg+ vqa91cKvnUfSDNe+X83LzBDI31gLFseAnKi31ewLq0hLCYpNle3E1/J3ChHHegiFMv amlOOWtFowp+BK+gjDEpUdaG0u/gHFsxaiA5G6g/C37KhHITRiPHaIaU3qs2pKEkiv wm0Le3dNNRd/KUEjOKv6cdwUjqzEMMAFccV86pO7+pe/Dr1TC0+H7NEdkXvFLqWM3A 5dmz+VJ+GiFkg== Date: Tue, 17 Dec 2024 13:22:26 -1000 From: Tejun Heo To: Andrea Righi Cc: David Vernet , Changwoo Min , Yury Norov , Ingo Molnar , Peter Zijlstra , Juri Lelli , Vincent Guittot , Dietmar Eggemann , Steven Rostedt , Ben Segall , Mel Gorman , Valentin Schneider , 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> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20241217094156.577262-4-arighi@nvidia.com> On Tue, Dec 17, 2024 at 10:32:28AM +0100, Andrea Righi wrote: > +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; Are node IDs guaranteed to be consecutive? Shouldn't it be `node >= nr_node_ids`? Also, should probably add node_possible(node)? > +/* > + * 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; As the masks are allocated separately anyway, the aligned attribute can be dropped. There's no reason to align the index array. > +static struct cpumask *get_idle_mask_node(int node, bool smt) > +{ > + 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); It's odd to validate input node in an internal function. If node is being passed from BPF side, we should validate it and trigger scx_ops_error() if invalid, but once the node number is inside the kernel, we should be able to trust it. > +static struct cpumask *get_idle_cpumask_node(int node) > +{ > + return get_idle_mask_node(node, false); Maybe make the inner function return `struct idle_cpumasks *` so that the caller can pick between cpu and smt? > +static void idle_masks_init(void) > +{ > + int node; > + > + idle_masks = kcalloc(num_possible_nodes(), sizeof(*idle_masks), GFP_KERNEL); We probably want to use a variable name which is more qualified for a global variable - scx_idle_masks? > @@ -3173,6 +3245,9 @@ bool scx_prio_less(const struct task_struct *a, const struct task_struct *b, > > static bool test_and_clear_cpu_idle(int cpu) > { > + int node = cpu_to_node(cpu); > + struct cpumask *idle_cpu = get_idle_cpumask_node(node); Can we use plurals for cpumask varialbles - idle_cpus here? > -static s32 scx_pick_idle_cpu(const struct cpumask *cpus_allowed, u64 flags) > +static s32 scx_pick_idle_cpu_from_node(int node, const struct cpumask *cpus_allowed, u64 flags) Do we need "from_node"? > { > int cpu; > > retry: > if (sched_smt_active()) { > - cpu = cpumask_any_and_distribute(idle_masks.smt, cpus_allowed); > + cpu = cpumask_any_and_distribute(get_idle_smtmask_node(node), cpus_allowed); This too, would s/get_idle_smtmask_node(node)/idle_smtmask(node)/ work? There are no node-unaware counterparts to these functions, right? > +static s32 > +scx_pick_idle_cpu_numa(const struct cpumask *cpus_allowed, s32 prev_cpu, u64 flags) > +{ > + 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(); Is rcu_read_lock() necessary? Does lockdep warn if the explicit rcu_read_lock() is dropped? > @@ -3643,17 +3776,33 @@ static void set_cpus_allowed_scx(struct task_struct *p, > > 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) { > + 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); Can you do the same cpumask_and() here? I don't think it'll cause practical problems but idle_cpus can be updated inbetween and e.g. we can end up with idle_smts that have different idle states between siblings. > /** > * scx_bpf_get_idle_cpumask - Get a referenced kptr to the idle-tracking > - * per-CPU cpumask. > + * per-CPU cpumask of the current NUMA node. This is a bit misleading as it can be system-wide too. It's a bit confusing for scx_bpf_get_idle_cpu/smtmask() to return per-node mask while scx_bpf_pick_idle_cpu() and friends are not scoped to the node. Also, scx_bpf_pick_idle_cpu() picking the local node as the origin probably doesn't make sense for most use cases as it's usually called from ops.select_cpu() and the waker won't necessarily run on the same node as the wakee. Maybe disallow scx_bpf_get_idle_cpu/smtmask() if idle_per_node is enabled and add scx_bpF_get_idle_cpu/smtmask_node()? Ditto for scx_bpf_pick_idle_cpu() and we can add a PICK_IDLE flag to allow/inhibit CPUs outside the specified node. Thanks. -- tejun