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

[James Morse <james.morse@arm.com>]

Hi Fenghua,

On 17/01/2023 18:29, Yu, Fenghua 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
> 
> s/allocate/allocated/
> 
>> + *                           CLOSID.
>> + * @closid: The CLOSID that is being queried.
>> + *
>> + * MPAM's equivalent of RMID are per-CLOSID, meaning a freshly allocate
> 
> s/allocate/allocated/

(Both fixed, thanks)


>> +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);
> 
> It's better to move lockdep_asser_held() after if (!IS_ENABLE()).
> Then compiler might optimize this function to empty on X86.

If you compile without lockdep it will be empty!
Is anyone worried about performance with lockdep enabled?

The reason for it being here is documentation and for the runtime check if you run with
lockdep. Having it here is so that new code that only runs on x86 (with lockdep) also
checks this, even though it doesn't have CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID.

I'd prefer to keep it so we can catch bugs early. Lockdep isn't on by default.


>> +
>> +	if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
>> +		return false;
>> +
>> +	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;
>> +}


Thanks,

James