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

  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.

Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com>
Signed-off-by: James Morse <james.morse@arm.com>

---
Changes since v1:
 * Removed superflous IS_ENABLED().
---
 arch/x86/kernel/cpu/resctrl/internal.h |  1 +
 arch/x86/kernel/cpu/resctrl/monitor.c  | 31 ++++++++++++++++++++++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 ++++++++------
 3 files changed, 42 insertions(+), 7 deletions(-)
  

Hi, James,

> 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.
> 
> Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> 
> ---
> Changes since v1:
>  * Removed superflous IS_ENABLED().
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
> arch/x86/kernel/cpu/resctrl/monitor.c  | 31 ++++++++++++++++++++++++++
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 ++++++++------
>  3 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
> b/arch/x86/kernel/cpu/resctrl/internal.h
> index 013c8fc9fd28..adae6231324f 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -509,6 +509,7 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup
> *rdtgrp);  void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp);  struct
> rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r);  int
> closids_supported(void);
> +bool resctrl_closid_is_dirty(u32 closid);
>  void closid_free(int closid);
>  int alloc_rmid(u32 closid);
>  void free_rmid(u32 closid, u32 rmid);
> 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/

> +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 (!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;
> +}
> +
>  /*
>   * As of now the RMIDs allocation is the same in each domain.
>   * However we keep track of which packages the RMIDs diff --git
> a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index ac88610a6946..e1f879e13823 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -93,7 +93,7 @@ void rdt_last_cmd_printf(const char *fmt, ...)
>   * - Our choices on how to configure each resource become progressively more
>   *   limited as the number of resources grows.
>   */
> -static int closid_free_map;
> +static unsigned long closid_free_map;
>  static int closid_free_map_len;
> 
>  int closids_supported(void)
> @@ -119,14 +119,17 @@ static void closid_init(void)
> 
>  static int closid_alloc(void)
>  {
> -	u32 closid = ffs(closid_free_map);
> +	u32 closid;
> 
> -	if (closid == 0)
> -		return -ENOSPC;
> -	closid--;
> -	closid_free_map &= ~(1 << closid);
> +	for_each_set_bit(closid, &closid_free_map, closid_free_map_len) {
> +		if (resctrl_closid_is_dirty(closid))
> +			continue;
> 
> -	return closid;
> +		clear_bit(closid, &closid_free_map);
> +		return closid;
> +	}
> +
> +	return -ENOSPC;
>  }
> 
>  void closid_free(int closid)
> --
> 2.30.2

Thanks.

-Fenghua
  

Hi James,

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.
> 
> Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> 
> ---
> Changes since v1:
>  * Removed superflous IS_ENABLED().
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 31 ++++++++++++++++++++++++++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 ++++++++------
>  3 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 013c8fc9fd28..adae6231324f 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -509,6 +509,7 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp);
>  void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp);
>  struct rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r);
>  int closids_supported(void);
> +bool resctrl_closid_is_dirty(u32 closid);
>  void closid_free(int closid);
>  int alloc_rmid(u32 closid);
>  void free_rmid(u32 closid, u32 rmid);
> 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..."? 

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

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

Reinette
  

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
  

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
  

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
  

Hi Reinette,

On 10/03/2023 19:59, Reinette Chatre wrote:
> On 3/3/2023 10:36 AM, James Morse wrote:
>> 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?

Yes, MPAM platforms would unconditionally enable it, as it reflects how MPAM works. [0]
Users would never need to know it exists.


>>>> +	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 ...")?

Sure thing,


Thanks,

James

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/morse/linux.git/tree/drivers/platform/mpam/Kconfig?h=mpam/snapshot/v6.2&id=ef6b11256ba2cceaff846c96183e8eb6019642d7