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

[James Morse]

Hi Reinette,

On 02/02/2023 23:46, Reinette Chatre wrote:
> On 1/13/2023 9:54 AM, James Morse wrote:
>> MPAM's PMG bits extend its PARTID space, meaning the same PMG value can be
>> used for different control groups.
>>
>> This means once a CLOSID is allocated, all its monitoring ids may still be
>> dirty, and held in limbo.
>>
>> Add a helper to allow the CLOSID allocator to check if a CLOSID has dirty
>> RMID values. This behaviour is enabled by a kconfig option selected by
>> the architecture, which avoids a pointless search for x86.

>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index 347be3767241..190ac183139e 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -327,6 +327,37 @@ static struct rmid_entry *resctrl_find_free_rmid(u32 closid)
>>  	return ERR_PTR(-ENOSPC);
>>  }
>>  
>> +/**
>> + * resctrl_closid_is_dirty - Determine if clean RMID can be allocate for this
>> + *                           CLOSID.
>> + * @closid: The CLOSID that is being queried.
>> + *
>> + * MPAM's equivalent of RMID are per-CLOSID, meaning a freshly allocate CLOSID
>> + * may not be able to allocate clean RMID. To avoid this the allocator will
>> + * only return clean CLOSID.
>> + */
>> +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.


> 
>> +
>> +	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.


Thanks!

James