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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 26 Aug 2022 11:57:26 -0500
From:   "Moger, Babu" <babu.moger@....com>
To:     Reinette Chatre <reinette.chatre@...el.com>, fenghua.yu@...el.com,
        tglx@...utronix.de, mingo@...hat.com, bp@...en8.de
Cc:     eranian@...gle.com, dave.hansen@...ux.intel.com, x86@...nel.org,
        hpa@...or.com, corbet@....net, linux-kernel@...r.kernel.org,
        linux-doc@...r.kernel.org, bagasdotme@...il.com
Subject: Re: [PATCH v3 07/10] x86/resctrl: Add sysfs interface files to
 read/write event configuration


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