Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7cabdefc-7624-84ab-4914-396e94a3e683@intel.com>
Date:   Fri, 26 Aug 2022 09:35:02 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.2.0
Subject: Re: [PATCH v3 07/10] x86/resctrl: Add sysfs interface files to
 read/write event configuration
Content-Language: en-US
To:     <babu.moger@amd.com>, <fenghua.yu@intel.com>, <tglx@linutronix.de>,
        <mingo@redhat.com>, <bp@alien8.de>
CC:     <eranian@google.com>, <dave.hansen@linux.intel.com>,
        <x86@kernel.org>, <hpa@zytor.com>, <corbet@lwn.net>,
        <linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <bagasdotme@gmail.com>
References: <166117559756.6695.16047463526634290701.stgit@bmoger-ubuntu>
 <166117583337.6695.3477964609702763678.stgit@bmoger-ubuntu>
 <c5777707-746e-edab-2ce2-3405ff55be56@intel.com>
 <3a306901-4e3c-f11a-f947-9afaa4431b36@amd.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <3a306901-4e3c-f11a-f947-9afaa4431b36@amd.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Nm9mVnBtQ1UyVFpqcWNxWTUvd2NodXVOS0I0azdySkIwbjJBS3pGZUNFSU9M?=
 =?utf-8?B?RVRBN0h0ZFBGSllLdXZZcVltS0VSYUJ5U3pwSkVpdGhra29PK3pCK3ZtcjR5?=
 =?utf-8?B?QkRtV2RPd25XRTlsQzF6YU1zSElNSFJ3UjVUWDByZHBMdEpBdHNDdTdPdkFD?=
 =?utf-8?B?OWVrZHJlS0c2ZjZuRnNRanNTYXZ0Vy9GQUVQbWw3WnRicnl0NmJ4eFlPZHIz?=
 =?utf-8?B?Y0xqbDI4eXVoaUErNmxaRE96RzZ6UUNFZVJPUDVWNE4rWUcxNi9uSGtUeUYz?=
 =?utf-8?B?TU5iRGhFYmZLRjdGZ0NsaVBJdnk5dTFEM2RpTXQ0ellsQWZCU1VqazI2bnhC?=
 =?utf-8?B?ejBEUXJoT1UxSVdiMFEzcFp2aDVmUUhDM2ZISDIwM3lRampwWE1FQ0JtcXk5?=
 =?utf-8?B?V280UTRJSm9PMWx4a0hxeUFMaThiaDVEQXl4VjV5emQvbzRnRkdNcG9aeEdQ?=
 =?utf-8?B?UnluK2MvYkZKWWZIQlhRTTgzTzR2MU5oMjJ4bThhZlBqSDJlMTExekpZVEUx?=
 =?utf-8?B?SEdKNEdMY3BGSmQ3STdQTUg5bTV0L3l4M0o1d2kvQUpYWXB0cjUwT3JwOTJo?=
 =?utf-8?B?a2VyYXBVbDVpcE81V2tmVEl5d3l1TUdTc3ZBR2tUMjljejdjODBCUU9lKy84?=
 =?utf-8?B?a2RldUJEQ3ZuMkxiR0h4NHZBUGUzVk5MaHZVcFE1UkZZSFV1eEp4Y24wM2F1?=
 =?utf-8?B?aWtMb1IrdndSQzFmNFF3NDVLY200UHN5OERrNG9Qbm1OcUJlZGhNT2xrYU81?=
 =?utf-8?B?NytXTTlBWmx6N1VVZDNJTzRMUjJoeUF6b0xIS3ZqMlhuOHNXNjY3akNEc0xB?=
 =?utf-8?B?S0JWZXlVV2NJVmM0eFpMaGsrUDhBNkZjYW55TzF6aWl2eFVXSmNNUENUUElL?=
 =?utf-8?B?R3FoUXc1WHhaQk53MWExVFNoaGViSFQrV1E5Tkh1UndxL25zbzREZ3E3VW4w?=
 =?utf-8?B?OFVZQVBjSzl3OHFJUnZVZ3dEckFHNWR6cXFXOTVzSEJSdlU3Z1o0K1owS1J3?=
 =?utf-8?B?V3dxSmdSZEtrRTBzbXRQMUtvMkF5TVhCR1YyWllPaWkvZS9Wbis1Y2lRaEx6?=
 =?utf-8?B?UjBaMFZxZDdnYjA2NHJraStpUWQxd0xoNCtMSkNBczhtRVoyMDFiVVdOMkdR?=
 =?utf-8?B?elhUL2FFZmFHYnNPeGV4dFlRZU1JUmxkYk5yWEhuRXJxekpETDBQMy96aUxV?=
 =?utf-8?B?L1BrbXBFSnA2S0YrNUxuTTJ3SDJ2bTJjUUxWRElTV2JFYnJUVWNtenJUR0Jx?=
 =?utf-8?B?blgwNGd4RXdCMW5OSVpTTVhrN1QzSU1lT2t6aXR1cHVYQjhBT042Qkd1bENt?=
 =?utf-8?B?S1BLNitmZ3V2V3pLSkpFNTRhdWhqTTFpWXZUWjBVRjYzZ1dyV2hOUW42RG53?=
 =?utf-8?B?S2Y2YVJ2clZBRU1EVUcrcmpERE9MNktheWRxYTc4a3E3SkY0ZGNHNys5c1JD?=
 =?utf-8?B?bkh4NktzOGJtRTZ1WjBMbTQvSGhaVFZrRnJXWXZYY0p0L0R5ZjlsYlZUd1c1?=
 =?utf-8?B?V1k4bncrdWRlb0VYWEtRVFdCZGZJVURoSmxWUmdwQmxRbElvSXZWRnpVSURu?=
 =?utf-8?B?TENQYkkwbTRqKzc1MHlGbzRGNEFLTytaSDljZjA2emJuQzNCc2poY0R6M29o?=
 =?utf-8?B?Yk1KOTlhV1E5NW41c0h5azdGUFZJdHQyY0RBYzRlWEVINFRxSXdwdFQxeFY1?=
 =?utf-8?B?SHNoZitCa3BGY0J4NzZNbUs2QUFqdjRxVTZTRU9wOGc1SUljSnF5Nno4NGt4?=
 =?utf-8?B?UENPalZBSlluYUhEV1psZmhKdHMydThValowWThEQ1JyTE1QTVVCNmo2N0hl?=
 =?utf-8?B?eHJJRmYrLzFhMHAvVG9GNnZwVU1lUWZseFI5Z1dKY2U2Vzl4ZHRVZU5yTVlW?=
 =?utf-8?B?dmU1TUltTlRGLzBjV0pKTWJjdS9tSEF0VEp2cFJPSDZVS0FvSzlGakNkRUdC?=
 =?utf-8?B?S3lpdlh2L3dCdTVibC85NFV1YVZzejJSWGZrMStJUEM3dnVyKzVzbEJvb1FE?=
 =?utf-8?B?THRFaHk5N2I4a2ppdVNsckF1eWlZVEhxenpYSWI1VXJNMFF2L0pFdWxObktu?=
 =?utf-8?B?c1IvaWhtbUhER3FUeU1xMmFCN1RneldVUWZvc3oxQ3Fyb2ZUZWF1Y0s2aTBy?=
 =?utf-8?B?WnFEQW96cFdsZlJiUGZNVmFTVkovVGl2UGFWM1o0bm1pUXFXRG85ekp3NDFC?=
 =?utf-8?B?b1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 480cb4d0-b16a-4390-7726-08da8780ee37
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Aug 2022 16:35:05.0241
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: hPwL4Klc5YuoxkXbIOkpsutQEMX0GaiASoCMO0YjX1CUaTIFicjg9/PpD4iF/yYo/xlVX1H1rfyYJ68So+wGQ6cVZJ7PC5YMLMKQ6dVMuB4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR11MB3975
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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