Re: [PATCH 1/2] sched: Add per_cpu cluster domain info and cpus_share_cluster API

[Gautham R. Shenoy]

Hello Vincent, Barry,

On Wed, Dec 22, 2021 at 02:33:37PM +0100, Vincent Guittot wrote:
> On Wed, 22 Dec 2021 at 14:14, Vincent Guittot
[..snip..]

> > > > > @@ -669,6 +671,12 @@ static void update_top_cache_domain(int cpu)
> > > > >       per_cpu(sd_llc_id, cpu) = id;
> > > > >       rcu_assign_pointer(per_cpu(sd_llc_shared, cpu), sds);
> > > > >
> > > > > +     sd = lowest_flag_domain(cpu, SD_CLUSTER);
> > > > > +     if (sd)
> > > > > +             id = cpumask_first(sched_domain_span(sd));
> > > > > +     rcu_assign_pointer(per_cpu(sd_cluster, cpu), sd);
> > > > > +     per_cpu(sd_cluster_id, cpu) = id;
> > > > > +
> >
> > This deserves a large comment to highlight that the new code above is
> > not self contained and  relies on the fact that it is done just after
> > looking for LLC and sd and that id and sd must not be changed in
> > between inorder to ensures that per_cpu(sd_cluster_id, cpu) equals
> > per_cpu(sd_llc_id, cpu) if there is no domain with SD_CLUSTER.
> >
> > and per_cpu(sd_cluster_id, cpu) might not be cluster but llc
> 
> Maybe you should not name this sd_cluster_id at all but
> per_cpu(sd_share_id, cpu) = id;
> 
> Then you have a function named
> 
> +bool cpus_share_resources(int this_cpu, int that_cpu)
> +{
> +     if (this_cpu == that_cpu)
> +             return true;
> +
> +     return per_cpu(sd_share_id, this_cpu) == per_cpu(sd_share_id, that_cpu);
> +}
> +
> which returns true when cpu shares resources which can be LLC or cluster

+1. This would make it more readable than overloading the meaning of cluster itself.

--
Thanks and Regards
gautham.