Re: [PATCH v2 06/18] x86/resctrl: Allow the allocator to check if a CLOSID can allocate clean RMID

[Reinette Chatre <reinette.chatre@intel.com>]

Hi James,

On 3/3/2023 10:36 AM, James Morse wrote:
> Hi Reinette,
> 
> On 02/02/2023 23:46, Reinette Chatre wrote:
>> On 1/13/2023 9:54 AM, James Morse wrote:

...

>>> +bool resctrl_closid_is_dirty(u32 closid)
>>> +{
>>> +	struct rmid_entry *entry;
>>> +	int i;
>>> +
>>> +	lockdep_assert_held(&rdtgroup_mutex);
>>> +
>>> +	if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
>>> +		return false;
> 
>> Why is a config option chosen? Is this not something that can be set in the
>> architecture specific code using a global in the form matching existing related
>> items like "arch_has..." or "arch_needs..."?
> 
> It doesn't vary by platform, so making it a runtime variable would mean x86 has to carry
> this extra code around, even though it will never use it. Done like this, the compiler can
> dead-code eliminate the below checks and embed the constant return value in all the callers.

This is fair. I missed any other mention of this option in this series so I
assume this will be a config that will be "select"ed automatically without
users needing to think about whether it is needed?

>>> +
>>> +	for (i = 0; i < resctrl_arch_system_num_rmid_idx(); i++) {
>>> +		entry = &rmid_ptrs[i];
>>> +		if (entry->closid != closid)
>>> +			continue;
>>> +
>>> +		if (entry->busy)
>>> +			return true;
>>> +	}
>>> +
>>> +	return false;
>>> +}
>>
>> If I understand this correctly resctrl_closid_is_dirty() will return true if
>> _any_ RMID/PMG associated with a CLOSID is in use. That is, a CLOSID may be
>> able to support 100s of PMG but if only one of them is busy then the CLOSID
>> will be considered unusable ("dirty"). It sounds to me that there could be scenarios
>> when CLOSID could be considered unavailable while there are indeed sufficient
>> resources?
> 
> You are right this could happen. I guess the better approach would be to prefer the
> cleanest CLOSID that has a clean PMG=0. User-space may not be able to allocate all the
> monitor groups immediately, but that would be preferable to failing the control group
> creation.
> 
> But as this code doesn't get built until the MPAM driver is merged, I'd like to keep it to
> an absolute minimum. This would be more than is needed for MPAM to have close to resctrl
> feature-parity, so I'd prefer to do this as an improvement once the MPAM driver is upstream.
> 
> (also in this category is better use of MPAM's monitors and labelling traffic from the iommu)
> 
> 
>> The function comment states "Determine if clean RMID can be allocate for this
>> CLOSID." - if I understand correctly it behaves more like "Determine if all
>> RMID associated with CLOSID are available".
> 
> Yes, I'll fix that.

I understand your reasoning for the solution chosen. Would you be ok to expand on
the function comment more to document the intentions that you summarize above (eg. "This
is the absolute minimum solution that will be/should be/could be improved ...")?

Reinette