Re: [PATCH v10 12/24] x86/resctrl: Introduce cntr_cfg to track assignable counters at domain

["Moger, Babu" <bmoger@amd.com>]

Hi Reinette,

On 12/19/2024 4:33 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> Did subject intend to use name of new struct?

Yes. Will change it to "mbm_cntr_cfg.
> 
> On 12/12/24 12:15 PM, Babu Moger wrote:
>> In mbm_assign_mode, the MBM counters are assigned/unassigned to an RMID,
>> event pair in a resctrl group and monitor the bandwidth as long as it is
>> assigned. Counters are assigned/unassigned at domain level and needs to
>> be tracked at domain level.
>>
>> Add the mbm_assign_cntr_cfg data structure to struct rdt_ctrl_domain to
> 
> "mbm_assign_cntr_cfg" -> "mbm_cntr_cfg"

Sure.

> 
>> manage and track MBM counter assignments at the domain level.
> 
> This can really use some more information about this data structure. I think
> it will be helpful to provide more information about how the data structure
> looks ... for example, that it is an array indexed by counter ID where the
> assignment details of each counter is stored. I also think it will be helpful
> to describe how interactions with  this data structure works, that a NULL
> rdtgrp means that the counter is free and that it is not possible to find
> a counter from a resource group and arrays need to be searched instead and doing
> so is ok for $REASON (when considering the number of RMID and domain combinations
> possible on AMD). A lot is left for the reader to figure out.

How about this?


In mbm_assign_mode, the MBM counters are assigned/unassigned to an RMID,
event pair in a resctrl group and monitor the bandwidth as long as it is
assigned. Counters are assigned/unassigned at domain level and needs to
be tracked at domain level.

Add the mbm_cntr_cfg data structure to struct rdt_ctrl_domain to
manage and track MBM counter assignments at the domain level.

Each domain will contain num_mbm_cntrs entries, indexed by cntr_id. 
During initialization, all entries will be set to zero. When a counter 
is allocated, its corresponding entry will be populated with the 
assigned struct rdtgroup and enum resctrl_event_id. When the counter is 
released, its entry will be reset to zero.

>>
>> Suggested-by: Peter Newman <peternewman@google.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v10: Patch changed completely to handle the counters at domain level.
>>       https://lore.kernel.org/lkml/CALPaoCj+zWq1vkHVbXYP0znJbe6Ke3PXPWjtri5AFgD9cQDCUg@mail.gmail.com/
>>       Removed Reviewed-by tag.
>>       Did not see the need to add cntr_id in mbm_state structure. Not used in the code.
>>
>> v9: Added Reviewed-by tag. No other changes.
>>
>> v8: Minor commit message changes.
>>
>> v7: Added check mbm_cntr_assignable for allocating bitmap mbm_cntr_map
>>
>> v6: New patch to add domain level assignment.
>> ---
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 11 +++++++++++
>>   include/linux/resctrl.h                | 12 ++++++++++++
>>   2 files changed, 23 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 682f47e0beb1..1ee008a63d8b 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -4068,6 +4068,7 @@ static void __init rdtgroup_setup_default(void)
>>   
>>   static void domain_destroy_mon_state(struct rdt_mon_domain *d)
>>   {
>> +	kfree(d->cntr_cfg);
>>   	bitmap_free(d->rmid_busy_llc);
>>   	kfree(d->mbm_total);
>>   	kfree(d->mbm_local);
>> @@ -4141,6 +4142,16 @@ static int domain_setup_mon_state(struct rdt_resource *r, struct rdt_mon_domain
>>   			return -ENOMEM;
>>   		}
>>   	}
>> +	if (is_mbm_enabled() && r->mon.mbm_cntr_assignable) {
>> +		tsize = sizeof(*d->cntr_cfg);
>> +		d->cntr_cfg = kcalloc(r->mon.num_mbm_cntrs, tsize, GFP_KERNEL);
>> +		if (!d->cntr_cfg) {
>> +			bitmap_free(d->rmid_busy_llc);
>> +			kfree(d->mbm_total);
>> +			kfree(d->mbm_local);
>> +			return -ENOMEM;
>> +		}
>> +	}
>>   
>>   	return 0;
>>   }
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index c8ab3d7a0dab..03c67d9156f3 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -94,6 +94,16 @@ struct rdt_ctrl_domain {
>>   	u32				*mbps_val;
>>   };
>>   
>> +/**
>> + * struct mbm_cntr_cfg -Assignable counter configuration
> 
> Please compare with style use in rest of the file. For example,
> "-Assignable" -> "- assignable"

Sure.

> 
>> + * @evtid:		Event type
> 
> This description is not useful. Consider: "MBM event to which
> the counter is assigned. Only valid if @rdtgroup is not NULL."
> (This was the first thing that came to my mind, please improve)
> 
>> + * @rdtgroup:		Resctrl group assigned to the counter
> 
> Can add "NULL if counter is free"

Sure.

> 
>> + */
>> +struct mbm_cntr_cfg {
>> +	enum resctrl_event_id	evtid;
>> +	struct rdtgroup		*rdtgrp;
>> +};
>> +
>>   /**
>>    * struct rdt_mon_domain - group of CPUs sharing a resctrl monitor resource
>>    * @hdr:		common header for different domain types
>> @@ -105,6 +115,7 @@ struct rdt_ctrl_domain {
>>    * @cqm_limbo:		worker to periodically read CQM h/w counters
>>    * @mbm_work_cpu:	worker CPU for MBM h/w counters
>>    * @cqm_work_cpu:	worker CPU for CQM h/w counters
>> + * @cntr_cfg:		Assignable counters configuration
> 
> Match capitalization of surrounding text.
> Will be helpful to add that this is an array indexed by counter ID.

ok. Sure.

> 
>>    */
>>   struct rdt_mon_domain {
>>   	struct rdt_domain_hdr		hdr;
>> @@ -116,6 +127,7 @@ struct rdt_mon_domain {
>>   	struct delayed_work		cqm_limbo;
>>   	int				mbm_work_cpu;
>>   	int				cqm_work_cpu;
>> +	struct mbm_cntr_cfg		*cntr_cfg;
>>   };
>>   
>>   /**
> 
> Reinette
>