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

[Andrea Righi <arighi@nvidia.com>]

Hi Tejun,

On Sat, Sep 26, 2026 at 01:27:37PM -1000, Tejun Heo wrote:
> 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);

Agreed. I'll use the inclusive cluster mask you suggested, so an LLC-wide L2
remains one cluster.

> 
> 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.

Right. I'll remove the redundant clear an changed both variables to plain
declarations.

> 
> 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.

Ack to all of the above, I'll send a v3.

Thanks,
-Andrea