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

[Moger, Babu]

Hi Dave,

On 4/16/24 11:13, Dave Martin wrote:
> 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@xxxxxxxxx/
>>
>>
>> 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--

That looks good. Thanks

> 
> 
> 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@xxxxxxx>
>>> ---
>>>  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

-- 
Thanks
Babu Moger