Messages in this thread | | | Date | Fri, 26 Aug 2022 09:35:02 -0700 | Subject | Re: [PATCH v3 07/10] x86/resctrl: Add sysfs interface files to read/write event configuration | From | Reinette Chatre <> |
| |
Hi Babu,
On 8/26/2022 9:07 AM, Moger, Babu wrote: > On 8/24/22 16:15, Reinette Chatre wrote: >> On 8/22/2022 6:43 AM, Babu Moger wrote:
...
>>> static int mkdir_mondata_subdir(struct kernfs_node *parent_kn, >>> struct rdt_domain *d, >>> struct rdt_resource *r, struct rdtgroup *prgrp) >>> @@ -2568,6 +2591,15 @@ static int mkdir_mondata_subdir(struct kernfs_node *parent_kn, >>> if (ret) >>> goto out_destroy; >>> >>> + /* Create the sysfs event configuration files */ >>> + if (r->mon_configurable && >>> + (mevt->evtid == QOS_L3_MBM_TOTAL_EVENT_ID || >>> + mevt->evtid == QOS_L3_MBM_LOCAL_EVENT_ID)) { >>> + ret = mon_config_addfile(kn, mevt->config, priv.priv); >>> + if (ret) >>> + goto out_destroy; >>> + } >>> + >> This seems complex to have event features embedded in the code in this way. Could >> the events not be configured during system enumeration? For example, instead >> of hardcoding the config like above to always set: >> >> static struct mon_evt mbm_local_event = { >> .name = "mbm_local_bytes", >> .evtid = QOS_L3_MBM_LOCAL_EVENT_ID, >> + .config = "mbm_local_config", >> >> >> What if instead this information is dynamically set in rdt_get_mon_l3_config()? To >> make things simpler struct mon_evt could get a new member "configurable" and the >> events that actually support configuration will have this set only >> if system has X86_FEATURE_BMEC (struct rdt_resource->configurable then >> becomes unnecessary?). Being configurable thus becomes an event property, not >> a resource property. The "config" member introduced here could then be "config_name". >> >> I think doing so will also make this file creation simpler with a single >> mon_config_addfile() (possibly with more parameters) used to add both files to >> avoid the code duplication introduced by mon_config_addfile() above. >> >> What do you think? > > Yes. We could do that. Something like this. > > struct mon_evt { > u32 evtid; > char *name; > > + bool configurable; > > char *config; > struct list_head list; > }; > > Set the configurable if the system has X86_FEATURE_BMEC feature in > rdt_get_mon_l3_config.
This would work (using bool in struct is something resctrl already do in many places). I also think that "config" should rather be named to "config_name" to make clear that it is not the actual configuration of the event. Remember to update struct mon_evt's kerneldoc (I just noticed it is missing from this series).
> > Create both files mbm_local_bytes and mbm_local_config in mon_addfile. > > Change the mon_addfile to pass mon_evt structure, so it have all > information to create both the files.
Providing the structure to the function would make all the information available but I am not sure that doing so would make it easy to eliminate the duplicate code needed to create the other file. Giving more parameters to mon_addfile() is another option but it should be more clear to you as you write this code.
> > Then we can remove rdt_resource->configurable. > > Does that make sense? >
Yes.
Reinette
| |