Re: [PATCH v6 03/22] x86/resctrl: Consolidate monitoring related data from rdt_resource

[Moger, Babu]

Hi Reinette,

On 8/16/24 16:29, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/6/24 3:00 PM, Babu Moger wrote:
>> The cache allocation and memory bandwidth allocation feature properties
>> are consolidated into cache and membw structures respectively.
> 
> "are consolidated into cache and membw structures respectively" ->
> "are consolidated into struct resctrl_cache and struct resctrl_membw
> respectively"

Sure.

> 
>>
>> In preparation for more monitoring properties that will clobber the
>> existing resource struct more, re-organize the monitoring specific
>> properties to also be in a separate structure.
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> ---
> 
> ...
> 
>> @@ -182,12 +182,21 @@ enum resctrl_scope {
>>       RESCTRL_L3_NODE,
>>   };
>>   +/**
>> + * struct resctrl_mon - Monitoring related data
> 
> To capture that this is not global monitoring data but instead
> resource specific:
> "Monitoring related data" -> "Monitoring related data of a resctrl resource"

Sure.

> 
>> + * @num_rmid:        Number of RMIDs available
>> + * @evt_list:        List of monitoring events
>> + */
>> +struct resctrl_mon {
>> +    int            num_rmid;
>> +    struct list_head    evt_list;
>> +};
>> +
>>   /**
>>    * struct rdt_resource - attributes of a resctrl resource
>>    * @rid:        The index of the resource
>>    * @alloc_capable:    Is allocation available on this machine
>>    * @mon_capable:    Is monitor feature available on this machine
>> - * @num_rmid:        Number of RMIDs available
>>    * @ctrl_scope:        Scope of this resource for control functions
>>    * @mon_scope:        Scope of this resource for monitor functions
>>    * @cache:        Cache allocation related data
>> @@ -199,7 +208,6 @@ enum resctrl_scope {
>>    * @default_ctrl:    Specifies default cache cbm or memory B/W percent.
>>    * @format_str:        Per resource format string to show domain value
>>    * @parse_ctrlval:    Per resource function pointer to parse control
>> values
>> - * @evt_list:        List of monitoring events
>>    * @fflags:        flags to choose base and info files
>>    * @cdp_capable:    Is the CDP feature available on this resource
>>    */
> 
> Please add a kernel-doc entry for the new member.

Yes. Missed it.
> 
>> @@ -207,11 +215,11 @@ struct rdt_resource {
>>       int            rid;
>>       bool            alloc_capable;
>>       bool            mon_capable;
>> -    int            num_rmid;
>>       enum resctrl_scope    ctrl_scope;
>>       enum resctrl_scope    mon_scope;
>>       struct resctrl_cache    cache;
>>       struct resctrl_membw    membw;
>> +    struct resctrl_mon    mon;
>>       struct list_head    ctrl_domains;
>>       struct list_head    mon_domains;
>>       char            *name;
>> @@ -221,7 +229,6 @@ struct rdt_resource {
>>       int            (*parse_ctrlval)(struct rdt_parse_data *data,
>>                            struct resctrl_schema *s,
>>                            struct rdt_ctrl_domain *d);
>> -    struct list_head    evt_list;
>>       unsigned long        fflags;
>>       bool            cdp_capable;
>>   };
> 
> Reinette
> 

-- 
Thanks
Babu Moger