Hi Reinette,
On 8/24/22 16:15, Reinette Chatre wrote:
> Hi Babu,
>
> On 8/22/2022 6:43 AM, Babu Moger wrote:
>> Add two new sysfs files to read/write the event configuration if
>> the feature Bandwidth Monitoring Event Configuration (BMEC) is
>> supported. The file mbm_local_config is for the configuration
>> of the event mbm_local_bytes and the file mbm_total_config is
>> for the configuration of mbm_total_bytes.
>>
>> $ls /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local*
>> /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local_bytes
>> /sys/fs/resctrl/mon_data/mon_L3_00/mbm_local_config
>>
>> $ls /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total*
>> /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_bytes
>> /sys/fs/resctrl/mon_data/mon_L3_00/mbm_total_config
>>
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> Reviewed-by: Ingo Molnar <mingo@xxxxxxxxxx>
>> ---
>> arch/x86/kernel/cpu/resctrl/internal.h | 3 +++
>> arch/x86/kernel/cpu/resctrl/monitor.c | 2 ++
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 32 ++++++++++++++++++++++++++++++++
>> 3 files changed, 37 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index c049a274383c..fc725f5e9024 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -72,11 +72,13 @@ DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);
>> * struct mon_evt - Entry in the event list of a resource
>> * @evtid: event id
>> * @name: name of the event
>> + * @config: current configuration
>> * @list: entry in &rdt_resource->evt_list
>> */
>> struct mon_evt {
>> u32 evtid;
>> char *name;
>> + char *config;
>> struct list_head list;
>> };
>>
>> @@ -95,6 +97,7 @@ union mon_data_bits {
>> unsigned int rid : 10;
>> unsigned int evtid : 8;
>> unsigned int domid : 14;
>> + unsigned int mon_config : 32;
>> } u;
>> };
>>
> This does not seem to be used in this patch.
I will move it next patch if required.
>
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index b9de417dac1c..3f900241dbab 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -656,11 +656,13 @@ static struct mon_evt llc_occupancy_event = {
>> static struct mon_evt mbm_total_event = {
>> .name = "mbm_total_bytes",
>> .evtid = QOS_L3_MBM_TOTAL_EVENT_ID,
>> + .config = "mbm_total_config",
>> };
>>
>> static struct mon_evt mbm_local_event = {
>> .name = "mbm_local_bytes",
>> .evtid = QOS_L3_MBM_LOCAL_EVENT_ID,
>> + .config = "mbm_local_config",
>> };
>>
>> /*
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 855483b297a8..30d2182d4fda 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -254,6 +254,10 @@ static const struct kernfs_ops kf_mondata_ops = {
>> .seq_show = rdtgroup_mondata_show,
>> };
>>
>> +static const struct kernfs_ops kf_mondata_config_ops = {
>> + .atomic_write_len = PAGE_SIZE,
>> +};
>> +
>> static bool is_cpu_list(struct kernfs_open_file *of)
>> {
>> struct rftype *rft = of->kn->priv;
>> @@ -2534,6 +2538,25 @@ void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r, unsigned int dom_id)
>> }
>> }
>>
>> +static int mon_config_addfile(struct kernfs_node *parent_kn, const char *name,
>> + void *priv)
>> +{
>> + struct kernfs_node *kn;
>> + int ret = 0;
>> +
>> + kn = __kernfs_create_file(parent_kn, name, 0644,
>> + GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, 0,
>> + &kf_mondata_config_ops, priv, NULL, NULL);
>> + if (IS_ERR(kn))
>> + return PTR_ERR(kn);
>> +
>> + ret = rdtgroup_kn_set_ugid(kn);
>> + if (ret)
>> + kernfs_remove(kn);
>> +
>> + return ret;
>> +}
>> +
>> 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.
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.
Then we can remove rdt_resource->configurable.
Does that make sense?
Thanks
Babu Moger
