Re: [PATCH v1 01/31] x86/resctrl: Fix allocation of cleanest CLOSID on platforms with no monitors

[Dave Martin <Dave.Martin@arm.com>]

On Mon, Apr 15, 2024 at 03:27:42PM -0500, Moger, Babu wrote:
> 
> 
> On 3/21/24 11:50, James Morse wrote:
> > commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
> > searching closid_num_dirty_rmid") added a Kconfig option that causes
> 
> This is not true. The Kconfig option is never added. It is added later in
> the series. There is also comment
> on this https://lore.kernel.org/lkml/ZhecyLQsGZ9Iv8wU@gmail.com/
> 
> 
> Shouldn't the Kconfig option added first before doing this change?
> 
> > resctrl to search for the CLOSID with the fewest dirty cache lines when
> > creating a new control group. This depends on the values read from the
> > llc_occupancy counters.

See David's comments and previous discussion on this patch.

You're right to point out that the description of the original commit
does seem a bit garbled though: CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is
not present in Kconfig here, but already referenced by other code.

We seem to have a consensus that it's OK to have a dangling IS_ENABLED()
so long as the option is added formally to Kconfig later, but it looks
like the commit message here should be reworded.

Does the following make sense?

--8<--

commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
searching closid_num_dirty_rmid") added logic that causes resctrl to
search for the CLOSID with the fewest dirty cache lines when creating a
new control group, if requested by the arch code. This depends on the
values read from the llc_occupancy counters. The logic is applicable to
architectures where the CLOSID effectively forms part of the monitoring
identifier and so do not allow complete freedom to choose an unused
monitoring identifier for a given CLOSID.

-->8--


Although it would probably have been better if the Kconfig option had
been added upstream, this patch does not create that that situation and
the series (taken as a whole) resolves it.

So I am not sure that anything would be solved or improved by changing
the body of this patch, but if people still have concerns then I guess
we can look at it.


> > 
> > This support missed that some platforms may not have these counters.
> > This causes a NULL pointer dereference when creating a new control
> > group as the array was not allocated by dom_data_init().
> > 
> > As this feature isn't necessary on platforms that don't have cache
> > occupancy monitors, add this to the check that occurs when a new
> > control group is allocated.
> > 
> > The existing code is not selected by any upstream platform, it makes
> > no sense to backport this patch to stable.
> > 
> > Signed-off-by: James Morse <james.morse@arm.com>
> > ---
> >  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index 011e17efb1a6..1767c1affa60 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -149,7 +149,8 @@ static int closid_alloc(void)
> >  
> >  	lockdep_assert_held(&rdtgroup_mutex);
> >  
> > -	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> > +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
> > +	    is_llc_occupancy_enabled()) {
> >  		cleanest_closid = resctrl_find_cleanest_closid();
> >  		if (cleanest_closid < 0)
> >  			return cleanest_closid;
> 
> -- 
> Thanks
> Babu Moger
> 

[...]

Cheers
---Dave