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@amd.com>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
 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;
 };
 
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;
+		}
+
 		if (is_mbm_event(mevt->evtid))
 			mon_event_read(&rr, r, d, prgrp, mevt->evtid, true);
 	}

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@amd.com>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
> ---
>  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.

> 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?

>  		if (is_mbm_event(mevt->evtid))
>  			mon_event_read(&rr, r, d, prgrp, mevt->evtid, true);
>  	}
> 
> 

Reinette

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@amd.com>
>> Reviewed-by: Ingo Molnar <mingo@kernel.org>
>> ---
>>  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

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

On 8/26/22 11:35, Reinette Chatre wrote:
> 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.
Sure.
> Remember to update struct mon_evt's kerneldoc (I just noticed it is
> missing from this series).

Oh,, Will do.

Thanks

Babu


>
>> 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
>
-- 
Thanks
Babu Moger