Re: [PATCH v6 1/8] x86/resctrl: Prepare for new domain scope

[Tony Luck]

On Fri, Sep 29, 2023 at 02:09:42PM +0200, Peter Newman wrote:
> Hi Tony,
> 
> On Thu, Sep 28, 2023 at 9:14 PM Tony Luck <tony.luck@xxxxxxxxx> wrote:
> >
> > Resctrl resources operate on subsets of CPUs in the system with the
> > defining attribute of each subset being an instance of a particular
> > level of cache. E.g. all CPUs sharing an L3 cache would be part of the
> > same domain.
> >
> > In preparation for features that are scoped at the NUMA node level
> > change the code from explicit references to "cache_level" to a more
> > generic scope. At this point the only options for this scope are groups
> > of CPUs that share an L2 cache or L3 cache.
> >
> > Provide a more detailed warning message if a domain id cannot be found
> > when adding a CPU. Just check and silent return if the domain id can't
> > be found when removing a CPU.
> >
> > No functional change.
> 
> I see a number of diagnostic checks added below. Are you sure there's
> no functional change?

Those checks should be for "can't happen" cases where some "scope" that
isn't attached to a cache_level somehow made its way down to a routine
that is only expecting cache scoped.

But I'll remove the "No functional change" from the commit. comment.
> 
> 
> > diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> > index 8f559eeae08e..8c5f932bc00b 100644
> > --- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> > +++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
> > @@ -292,10 +292,14 @@ static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
> >   */
> >  static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
> >  {
> > +       int scope = plr->s->res->scope;
> >         struct cpu_cacheinfo *ci;
> >         int ret;
> >         int i;
> >
> > +       if (WARN_ON_ONCE(scope != RESCTRL_L2_CACHE && scope != RESCTRL_L3_CACHE))
> > +               return -ENODEV;
> 
> Functional change?
> 
> 
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index 725344048f85..1cf2b36f5bf8 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -1345,10 +1345,13 @@ unsigned int rdtgroup_cbm_to_size(struct rdt_resource *r,
> >         unsigned int size = 0;
> >         int num_b, i;
> >
> > +       if (WARN_ON_ONCE(r->scope != RESCTRL_L2_CACHE && r->scope != RESCTRL_L3_CACHE))
> > +               return -EINVAL;
> 
> This function returns unsigned int. That's a huge region!

I wondered for a bit what value to return for this "can't happen" case
and initially went with an error code.  But the function can already
fail (in ways that won't happen) and in that case the return is "0".

I'l update to return 0 here too.

> -Peter

Thanks for the review.

-Tony