Re: [PATCH v2 sched_ext/for-7.4] sched_ext: cid: Represent clusters explicitly

[Tejun Heo <tj@kernel.org>]

Hello, Andrea.

The following is a Claude-generated review.

On Sat, Sep 26, 2026 at 11:20:17PM +0200, Andrea Righi wrote:
> +	if (cpumask_subset(cluster, topology_sibling_cpumask(cpu)))
> +		return NULL;
> +	if (cpumask_subset(llc_cpus, cluster))
> +		return NULL;

The second drop inverts the hardware on a part whose cores all share one
L2, such as a single E-core module or a DSU whose L2 is the LLC: each core
becomes its own cluster and a scheduler looking for L2 sharers through
cluster_cid finds none. The sched domain doesn't drop the cluster there
either, it drops MC and keeps CLS. Can we make the level inclusive instead
and skip the conditions altogether?

	cpumask_or(cluster_scratch, topology_cluster_cpumask(xcpu),
		   topology_sibling_cpumask(xcpu));
	cpumask_and(cluster_scratch, cluster_scratch, llc_scratch);

Private-L2 cores yield the core, modules yield the module and an LLC-wide
L2 yields one cluster per LLC, with the walk order unchanged. That also
drops the helper with its NULL and empty checks, which no arch can
produce, and the lines past 94 columns.

> +			s32 cluster_cid = next_cid, cluster_idx = next_cluster_idx;

cluster_scratch is always empty when an LLC starts, so the refill block
assigns both before any use. Plain declarations, and the per-LLC
cpumask_clear() is redundant for the same reason.

A few smaller things:

- The comments in cid.c and types.h describe SD_CLUSTER and when the sched
  domain drops the level. This layer's contract is enough: the
  cache-sharing level between the core and the LLC, and a core without one
  is its own cluster. The sched domain also doesn't exist with
  CONFIG_SCHED_CLUSTER=n while the L2 masks do.
- scx_bpf_cid_override()'s kerneldoc still says core/LLC/node is cleared.
- The test paragraph can be one sentence, and the Based on note under the
  separator is moot now that the fix is in for-7.4. Maybe "sched_ext: Add
  the cluster level to the cid topology" for the subject; nothing in the
  tree uses a cid: prefix.

Thanks.

-- 
tejun