Re: [PATCH v3 07/10] x86/resctrl: Add sysfs interface files to read/write event configuration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux