Hi Reinette, On 6/24/25 16:28, Reinette Chatre wrote: > Hi Babu/Tony, > > On 6/13/25 2:04 PM, Babu Moger wrote: >> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h >> index 0a1eedba2b03..20e2c45cea64 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 > > nit: I still think "Properties" is more appropriate. I will let Tony take care of this. Also, there is another comment https://lore.kernel.org/lkml/b761e6ec-a874-4d06-8437-a3a717a91abb@xxxxxxxxx/ I can pick up from your "aegl" tree. Let me know otherwise. I am not in a hurry. I will plan to post the series next week. > >> * @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++) >> + > >>From what I can tell this series does not build on some core changes > made by this patch: > - note that resource ID is added to struct mon_evt and the events > are *removed* from the evt_list associated with the resource. I'll try to point > out where I see it but this series still behaves as though it is traversing > evt_list associated with the resource. Take for example > patch #24 "fs/resctrl: Add event configuration directory under info/L3_MON/": > resctrl_mkdir_counter_configs() traverses mon_event_all[] that, after this > patch, contains all events for *all* resources, yet resctrl_mkdir_counter_configs(), > even though it has a struct rdt_resource as parameter, assumes that all events are > associated its resource but there is no checking to enforce this. > - note the new for_each_mon_event() above. This should be used throughout > instead of open-coding the loop because the array starts at index 0 but > the first valid entry is at index 1. The above macro makes this easier to > get right. Yes. Make sense. Will take of this in patch #24, #28 and #29.-- Thanks Babu Moger
