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

["Keshavamurthy, Anil S" <anil.s.keshavamurthy@intel.com>]

On 6/4/2025 3:30 PM, Reinette Chatre wrote:
> 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

Sorry, I just picked a random header file there....ignore that part but 
my real suggestion was to reduce the lines when declaring "struct 
mon_evt mon_event_all[]" with above #define so more events can fit in 
one screen when viewing the code.