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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9e8c9810-1b5c-4e30-8b10-c3702810b529@intel.com>
Date: Wed, 4 Jun 2025 15:30:18 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: "Keshavamurthy, Anil S" <anil.s.keshavamurthy@...el.com>, Tony Luck
	<tony.luck@...el.com>, Babu Moger <babu.moger@....com>
CC: Fenghua Yu <fenghuay@...dia.com>, Maciej Wieczor-Retman
	<maciej.wieczor-retman@...el.com>, Peter Newman <peternewman@...gle.com>,
	James Morse <james.morse@....com>, Drew Fustini <dfustini@...libre.com>,
	"Dave Martin" <Dave.Martin@....com>, Chen Yu <yu.c.chen@...el.com>,
	<x86@...nel.org>, <linux-kernel@...r.kernel.org>, <patches@...ts.linux.dev>
Subject: Re: [PATCH v5 1/4 UPDATED] x86,fs/resctrl: Consolidate monitor event
 descriptions

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