Re: [PATCH 01/18] x86/resctrl: Track the closid with the rmid

[James Morse]

Hi Fenghua,

On 06/01/2023 02:57, Yu, Fenghua wrote:
>> James Morse <james.morse@xxxxxxx> writes:
>>
>> x86's RMID are independent of the CLOSID. An RMID can be allocated, used and
>> freed without considering the CLOSID.
>>
>> MPAM's equivalent feature is PMG, which is not an independent number, it
>> extends the CLOSID/PARTID space. For MPAM, only PMG-bits worth of 'RMID'
>> can be allocated for a single CLOSID.
>> i.e. if there is 1 bit of PMG space, then each CLOSID can have two monitor
>> groups.
>>
>> To allow rescrl to disambiguate RMID values for different CLOSID, everything in
> 
> s/rescrl/resctrl/
> 
>> resctrl that keeps an RMID value needs to know the CLOSID too. This will always
>> be ignored on x86.


>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 5f7128686cfd..4b243ba88882 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -519,7 +519,7 @@ struct rdt_domain *get_domain_from_cpu(int cpu,
>> struct rdt_resource *r);  int closids_supported(void);  void closid_free(int closid);
>> int alloc_rmid(void); -void free_rmid(u32 rmid);
>> +void free_rmid(u32 closid, u32 rmid);
>>  int rdt_get_mon_l3_config(struct rdt_resource *r);  void mon_event_count(void
>> *info);  int rdtgroup_mondata_show(struct seq_file *m, void *arg); diff --git
>> a/arch/x86/kernel/cpu/resctrl/monitor.c
>> b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index efe0c30d3a12..f1f66c9942a5 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -25,6 +25,7 @@
>>  #include "internal.h"
>>
>>  struct rmid_entry {
>> +	u32				closid;
> 
> Could you please add a comment for this closid field? It's expected to be form x86 RMID entry.
> So it's deserved a comment to explain a bit more on this field.

Sure ... what does it need to convey?
I'm not sure what you mean by "expected to be form x86 RMID entry".

My medium verbosity version looks like this:
|        /*
|         * Some architectures's resctrl_arch_rmid_read() needs the CLOSID value
|         * in order to access the correct monitor. This field provides the
|         * value to list walkers like __check_limbo(). On x86 this is ignored.
|         */


Does this cover it?


Thanks,

James


>>  	u32				rmid;
>>  	int				busy;
>>  	struct list_head		list;