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

[Reinette Chatre <reinette.chatre@intel.com>]

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