Received-SPF: pass (google.com: domain of linux-kernel+bounces-194531-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <fb5b4f70-a983-457a-84df-59a237055f5f@intel.com>
Date: Wed, 29 May 2024 10:48:15 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 10/16] selftests/resctrl: Add ->init() callback into
 resctrl_val_param
To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	<linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>, Babu Moger
	<babu.moger@amd.com>, =?UTF-8?Q?Maciej_Wiecz=C3=B3r-Retman?=
	<maciej.wieczor-retman@intel.com>
CC: <linux-kernel@vger.kernel.org>, Fenghua Yu <fenghua.yu@intel.com>, "Shuah
 Khan" <skhan@linuxfoundation.org>
References: <20240520123020.18938-1-ilpo.jarvinen@linux.intel.com>
 <20240520123020.18938-11-ilpo.jarvinen@linux.intel.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <20240520123020.18938-11-ilpo.jarvinen@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OGFqdlpkZnFXZTRRWDRBeDBhZUpTYzhKMjVJUmVRREVtMTRDNVlUUmVrQUhm?=
 =?utf-8?B?M3AvU2t1UlV0cS8rQW5odW04ZjlIOUFGeW13Z0gwbWRKOXNjWnhERXd2Ykdy?=
 =?utf-8?B?cERpcXRheG5LMEVHN2QyMllYYzMvTEgzSzMvNXVUbFd5UGFOL0JMREVScm9U?=
 =?utf-8?B?VW1UL09waFBnL21DMmxTWlo5K3pBNmxhRzAvWEVLSnJ4NVkycU8wNUlUTFpt?=
 =?utf-8?B?L1RPU3J6OHNkUDA4d3drSy9Fb1poZXNocU9NaXZYNmdhMVN4Q0VDckdoUXNV?=
 =?utf-8?B?SG5zN25zcmF4cXB2V0JYZnAzcDErcnFJNm82VVRMMmo0NVIwcVpqSzRyRjJh?=
 =?utf-8?B?MVZaVG1rUHhja28rM0x6czhLVjNIWkt5Q3Q4bUpTUVFheFZlblh1Nllla25y?=
 =?utf-8?B?ZGFmTmZ3ZnZtSjlrTzlBTitNcDFuNTFyaFRBcjlsYkJGSm9pblRNc1N1bnZm?=
 =?utf-8?B?OW8zMWtBSDBxVjFreGljcWRtUzJTSFFyQmw0ZGxCN3ZTcWdJc2VoVS9BRzR2?=
 =?utf-8?B?UGl4Unpvbkt4cTdwRDVhdGt5b015WUo2OEFtZlhzQzlMR0xKc01zNXpUOU5W?=
 =?utf-8?B?L1BsQmVYNlczWjZhWlhaanlERTZOaktZL2lSRmV6bU9QTG9SQTNaeXhWdGNr?=
 =?utf-8?B?aGFRTEM5aXdBY3IveGhMZWQ4OU45eWVtc0Z2blI0MlMwQ25kOVExL3N4R01w?=
 =?utf-8?B?TUdkOWwwdFBVdnYxUEc3WkgvbjFWakRJT25oMkFBOHV0Y3k1UzlFbW1lcjRM?=
 =?utf-8?B?RmxLZ1BEODVGMXI0WS9jZ2pVWkplUkJEUXNSTDJ0bSt0c1cwOEk2U05xQ05o?=
 =?utf-8?B?N0o3Tlh5RmhzT3RrRGZUMEpRRThnNFY4Z2lNcFRSYTN0NGxheTh6K2xZM2cr?=
 =?utf-8?B?ODJGK3R4WEUvdy9VSGR6OGNPVmhLUnptUmJSTXVQU2F2dk5rMktOUUdiN2Vy?=
 =?utf-8?B?M2lLM1EzUk1CaG44a2FXbFBBaWNUSHBHU3FNNEdFdHFpc0gralVlUjc1YWwz?=
 =?utf-8?B?UTduVnhZYlgxNTlSMWMxNjFoTEZ4bndnUHZaR2hpZEZ6Z2ZjYjVxNjdKYUlw?=
 =?utf-8?B?UFV4dm81U29sb1lEM3k0VUhwdW5kMFR0WFJIMVFqaENydlNqZmJxUFRpRitm?=
 =?utf-8?B?YStDMlR2Y0NidXRYOWN0b2NxbnhnNjIzZWRzMnBCdSs3dTNGYmdDdUN6SEFk?=
 =?utf-8?B?NmRDUGhJSE5TVzFaUDE0bE9TSmxQYmNERWRjNHQ3WGtJSzh3cHZVMU8ralVs?=
 =?utf-8?B?YVUvUUt2WWFnVjVHYXFEVGh2Z2R2SmZrOExscFE0aE1jdVhjSldycGQyVk1m?=
 =?utf-8?B?TFhBYUFmcnpSL0JVMzhHNXllUHlibGpWMk9sM0lVOTdkU3U1Q0MyNGpOMjds?=
 =?utf-8?B?U0VNN3VPaDB4NWdiZmRGNW5KYXd1eUxIaHFZb2k5Y2Jvc2VZQ1JBdGtFQmxF?=
 =?utf-8?B?b1ZFa0lobEl2VFN2eGVnQklncmFKMUpVVzV5YWZqdHk2eThFVHd3dVVOUE9r?=
 =?utf-8?B?WWZqaEU0K1RpbW5DeVNnNjR3dWFQMnlyV1VMMjlGbjdUNkc1cWlJaDNJWTZs?=
 =?utf-8?B?WWRoWnhIOFdXSlFIaVV2YzhyTVU3d0l5UFR3ZkZvUk1zZzhnbkZKTktBcEtp?=
 =?utf-8?B?M05QSytkaWUzWFVpT0lvN2VWN2dRVXlsY3NHWlc3eVpRR0JuU1htRm5EaHVY?=
 =?utf-8?B?WFFJOEg0KzJ3VXk0TlpIVG1RRXgrc3JkeG1acklKN2NsRzZPSjFsdWc0K2pM?=
 =?utf-8?B?c2t6MkRXb2h4OHhhMWlhRzRkRlVoWlZJUGZhcTdHNVMvb2ZOSlFqZFJoeXNr?=
 =?utf-8?B?c1EyWmtvbmNZaTUxL2JWNktYc0dZSU5EV1d6RW5uSExBNXJGdllnRktiSzJE?=
 =?utf-8?B?TlhrcE5yR1k2Vi9YSldCN3FibDFmdXFrNUJTL29rL1U0M1NNODZ3VXlxRjJj?=
 =?utf-8?B?cVhGSG1jMFczcU95L1JxQjhyVGZMbDl1V3lwMVVVWVh0ZkVjeGNia29iMjNs?=
 =?utf-8?B?T1o4TFFURXlQQzM3OUordFZaWjR2RDBEZXA4Ri9GWEFMeDc1dXgrRm5WYTZa?=
 =?utf-8?B?RE5VcEY0cmRSaTQ0cThkMC9CNVQyM255VFJYeFVaMHNVUkt2b1hGeHR2RDZa?=
 =?utf-8?B?MEo5blRTaXdBYTFtRk1jQVhkdlJ1YVdCN2ZmaS9yY2IzcUV4ZmNlRWVZOHdL?=
 =?utf-8?B?YWc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0590e152-7994-45fa-8b91-08dc800785fe
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 May 2024 17:48:18.1711
 (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: 1G5OVzPpmkg+KFMrUaTIbIpkapZ5ou1yw03CN1eKkt+4h2R97KG5n/vyCYyYON2U23HzEJxvZ+RUxEy3LYIH92PLeeEKiUhO6eBAwBTtce0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4587
X-OriginatorOrg: intel.com

Hi Ilpo,

On 5/20/24 5:30 AM, Ilpo Järvinen wrote:

..

> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 5dc3def70669..d3fbb957309d 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -86,7 +86,8 @@ struct resctrl_test {
>    * @mongrp:		Name of the monitor group (mon grp)
>    * @filename:		Name of file to which the o/p should be written
>    * @bw_report:		Bandwidth report type (reads vs writes)
> - * @setup:		Call back function to setup test environment
> + * @init:		Callback function to initialize test environment
> + * @setup:		Callback function to setup per test run environment

hmmm ... ok ... this unrelated change could sneak in with the stretch argument of
making the text consistent.

>    * @measure:		Callback that performs the measurement (a single test)
>    */
>   struct resctrl_val_param {
> @@ -97,6 +98,8 @@ struct resctrl_val_param {
>   	char		*bw_report;
>   	unsigned long	mask;
>   	int		num_of_runs;
> +	int		(*init)(const struct resctrl_val_param *param,
> +				int domain_id);
>   	int		(*setup)(const struct resctrl_test *test,
>   				 const struct user_params *uparams,
>   				 struct resctrl_val_param *param);
> @@ -149,8 +152,11 @@ unsigned char *alloc_buffer(size_t buf_size, int memflush);
>   void mem_flush(unsigned char *buf, size_t buf_size);
>   void fill_cache_read(unsigned char *buf, size_t buf_size, bool once);
>   int run_fill_buf(size_t buf_size, int memflush, int op, bool once);
> +int initialize_mem_bw_imc(void);
>   int measure_mem_bw(const struct user_params *uparams,
>   		   struct resctrl_val_param *param, pid_t bm_pid);
> +void initialize_mem_bw_resctrl(const struct resctrl_val_param *param,
> +			       int domain_id);
>   int resctrl_val(const struct resctrl_test *test,
>   		const struct user_params *uparams,
>   		const char * const *benchmark_cmd,
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 76ae89e1aea3..23a82bd547a3 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -23,18 +23,6 @@
>   #define CON_MBM_LOCAL_BYTES_PATH		\
>   	"%s/%s/mon_data/mon_L3_%02d/mbm_local_bytes"
>   
> -#define CON_MON_LCC_OCCUP_PATH		\
> -	"%s/%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
> -
> -#define CON_LCC_OCCUP_PATH		\
> -	"%s/%s/mon_data/mon_L3_%02d/llc_occupancy"
> -
> -#define MON_LCC_OCCUP_PATH		\
> -	"%s/mon_groups/%s/mon_data/mon_L3_%02d/llc_occupancy"
> -
> -#define LCC_OCCUP_PATH			\
> -	"%s/mon_data/mon_L3_%02d/llc_occupancy"
> -
>   struct membw_read_format {
>   	__u64 value;         /* The value of the event */
>   	__u64 time_enabled;  /* if PERF_FORMAT_TOTAL_TIME_ENABLED */
> @@ -268,7 +256,7 @@ static int num_of_imcs(void)
>   	return count;
>   }
>   
> -static int initialize_mem_bw_imc(void)
> +int initialize_mem_bw_imc(void)
>   {
>   	int imc, j;
>   
> @@ -430,24 +418,18 @@ static int get_mem_bw_imc(char *bw_report, float *bw_imc)
>   
>   /*
>    * initialize_mem_bw_resctrl:	Appropriately populate "mbm_total_path"
> - * @ctrlgrp:			Name of the control monitor group (con_mon grp)
> - * @domain_id:			Domain ID (cache ID; for MB, L3 cache ID)
> + * @param:	parameters passed to resctrl_val()

Please let description consistently start with upper case.

> + * @domain_id:	Domain ID (cache ID; for MB, L3 cache ID)
>    */
> -static void initialize_mem_bw_resctrl(const char *ctrlgrp, int domain_id)
> +void initialize_mem_bw_resctrl(const struct resctrl_val_param *param,
> +			       int domain_id)
>   {
>   	sprintf(mbm_total_path, CON_MBM_LOCAL_BYTES_PATH, RESCTRL_PATH,
> -		ctrlgrp, domain_id);
> +		param->ctrlgrp, domain_id);
>   }

With typo fixed:

| Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette