Re: [PATCH v5 1/4 UPDATED] x86,fs/resctrl: Consolidate monitor event descriptions

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Anil,

On 6/4/25 3:21 PM, Keshavamurthy, Anil S wrote:
> 
> On 6/4/2025 2:22 PM, Tony Luck wrote:

...

>> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
>> index 9a8cf6f11151..71963255ad36 100644
>> --- a/fs/resctrl/internal.h
>> +++ b/fs/resctrl/internal.h
>> @@ -52,19 +52,26 @@ static inline struct rdt_fs_context *rdt_fc2context(struct fs_context *fc)
>>   }
>>     /**
>> - * struct mon_evt - Entry in the event list of a resource
>> + * struct mon_evt - Description of a monitor event
>>    * @evtid:        event id
>> + * @rid:        index of the resource for this event
>>    * @name:        name of the event
>>    * @configurable:    true if the event is configurable
>> - * @list:        entry in &rdt_resource->evt_list
>> + * @enabled:        true if the event is enabled
>>    */
>>   struct mon_evt {
>>       enum resctrl_event_id    evtid;
>> +    enum resctrl_res_level    rid;
>>       char            *name;
>>       bool            configurable;
>> -    struct list_head    list;
>> +    bool            enabled;
>>   };
>>   +extern struct mon_evt mon_event_all[QOS_NUM_EVENTS];
>> +
>> +#define for_each_mon_event(mevt) for (mevt = &mon_event_all[QOS_FIRST_EVENT];    \
>> +                      mevt < &mon_event_all[QOS_NUM_EVENTS]; mevt++)
>> +
>>   /**
>>    * struct mon_data - Monitoring details for each event file.
>>    * @list:            Member of the global @mon_data_kn_priv_list list.

...

>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>> index bde2801289d3..90093e54a279 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -842,38 +842,39 @@ static void dom_data_exit(struct rdt_resource *r)
>>       mutex_unlock(&rdtgroup_mutex);
>>   }
>>   -static struct mon_evt llc_occupancy_event = {
>> -    .name        = "llc_occupancy",
>> -    .evtid        = QOS_L3_OCCUP_EVENT_ID,
>> -};
>> -
>> -static struct mon_evt mbm_total_event = {
>> -    .name        = "mbm_total_bytes",
>> -    .evtid        = QOS_L3_MBM_TOTAL_EVENT_ID,
>> -};
>> -
>> -static struct mon_evt mbm_local_event = {
>> -    .name        = "mbm_local_bytes",
>> -    .evtid        = QOS_L3_MBM_LOCAL_EVENT_ID,
>> -};
>> -
>>   /*
>> - * Initialize the event list for the resource.
>> - *
>> - * Note that MBM events are also part of RDT_RESOURCE_L3 resource
>> - * because as per the SDM the total and local memory bandwidth
>> - * are enumerated as part of L3 monitoring.
>> + * All available events. Architecture code marks the ones that
>> + * are supported by a system using resctrl_enable_mon_event()
>> + * to set .enabled.
>>    */
>> -static void l3_mon_evt_init(struct rdt_resource *r)
>> +struct mon_evt mon_event_all[QOS_NUM_EVENTS] = {
>> +    [QOS_L3_OCCUP_EVENT_ID] = {
>> +        .name    = "llc_occupancy",
>> +        .evtid    = QOS_L3_OCCUP_EVENT_ID,
>> +        .rid    = RDT_RESOURCE_L3,
>> +    },
>> +    [QOS_L3_MBM_TOTAL_EVENT_ID] = {
>> +        .name    = "mbm_total_bytes",
>> +        .evtid    = QOS_L3_MBM_TOTAL_EVENT_ID,
>> +        .rid    = RDT_RESOURCE_L3,
>> +    },
>> +    [QOS_L3_MBM_LOCAL_EVENT_ID] = {
>> +        .name    = "mbm_local_bytes",
>> +        .evtid    = QOS_L3_MBM_LOCAL_EVENT_ID,
>> +        .rid    = RDT_RESOURCE_L3,
>> +    },
>> +};
> 
> As we start adding many more events to this struct(including region aware specific events), this this becomes too stressful on the eyes to read.....can you consider simplifying this with #define something like below in your next version? NOTE: For the feature that I am adding along with Chen Yu, we started to define a macro as shown below.
> 
> #define MON_EVENT(_id, _name, _res) \
>     [_id] = {                      \
>         .name  = _name,           \
>         .evtid = _id,             \
>         .rid   = _res,            \
>     }
> 
> Above #define can to into include/linux/resctrl_types.h file and the above code reduces to using MON_EVENT as shown below.

Any reason why the events need to leak outside resctrl fs? At the moment it is kept
inside resctrl fs with helpers for only those properties (not descriptions!) that
can/should be set by archs. Enabling arch full control of the event is not ideal
since many of the properties are required to be the same across all archs and
dictate the user interface that is ABI.

Reinette