Received-SPF: pass (google.com: domain of linux-kernel+bounces-215410-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <7cff31f8-f94f-4b76-beb9-073c369e5080@intel.com>
Date: Fri, 14 Jun 2024 11:37:42 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 1/4] selftests/resctrl: Rename variables and functions
 to generic names
To: Babu Moger <babu.moger@amd.com>, <fenghua.yu@intel.com>,
	<shuah@kernel.org>
CC: <linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>,
	<ilpo.jarvinen@linux.intel.com>, <maciej.wieczor-retman@intel.com>,
	<peternewman@google.com>, <eranian@google.com>
References: <cover.1717626661.git.babu.moger@amd.com>
 <c1b9905b5fd6b80933fddd6c576e5d34a78b2b90.1717626661.git.babu.moger@amd.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <c1b9905b5fd6b80933fddd6c576e5d34a78b2b90.1717626661.git.babu.moger@amd.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aDk2TFlxWjMyZHg5b1lQVCtmYStzNlo4bjZ5V3dnZ0NtbW4zdHBpcVo3VE1D?=
 =?utf-8?B?OFJPWjRyaDZWeHRXNllIRWc4dXVUU2dSRGZUZ3oycHVVMEtlM3Z4TlBjUmsv?=
 =?utf-8?B?R0hBNnB4Y1lydE5BZzZQRFdsUkZHci9SYlhGOFJLbFRONTFhN0JJUzFuYVJ2?=
 =?utf-8?B?T0RPbWNVS1NtOEZEUGQybVEwR0RaZ0thRGJ1WDZ5MzBjeUFPbWxDSjRiRVg4?=
 =?utf-8?B?Q2ZwVzUycDI3QWxLZ21Zc0MwUzRXUkRwNC9WWUxKbkxid29GcElqczlJQ3hK?=
 =?utf-8?B?OXZIQ2xJOUxMR2pNN21wM252NEdHSzlncUpZR3YxUWxmSEtKV1U4OHo4V0Rs?=
 =?utf-8?B?UktLTHJRMm5lVkRZMkJnV2piUHZMNGpHYWRoT0R4dG1vMmlBWXlndlUySnhs?=
 =?utf-8?B?QnZDbkY3a011UjlSNGxvQTRtOW8ySDZ2VmZ5aUlzZm56UU0zOUhCT0hMbTl2?=
 =?utf-8?B?WWxKODdncTl4SFgvaDBOY0xoRWl5MnhMYWtSTitUbW5KNHVOZW0wOWZOR2ti?=
 =?utf-8?B?VkxMSVJ0cmJLaXRJcy9vRFpGMUYzaHRyY2NFbGhQKzdaQkVMOEZvTDJIOTNj?=
 =?utf-8?B?dzJEd0s2ZGpqdG4xK0pTWlE1UnpReVQ5bm1tL2toRWUzZ05RTkxHelJuVnk4?=
 =?utf-8?B?RVlROVBTVitOUWFES1BnSW1rcDZxNFpFWE1hdk9JbnhhZmtsQ1V5bXU2S0hF?=
 =?utf-8?B?UmJyREVjeGVCWVZ0M1ZLVUpkbUc0Y29vem1FSUZSblZGblVrdUlJaU5VcWFX?=
 =?utf-8?B?TjVkbXRxMG9vMG10WmY2SDI0MWV1SXVVRHY1VEtFdjF6aTBQOTAzVmxxTE9q?=
 =?utf-8?B?b0FQSjZCUk9zd2hZWHBrT1gwcmtwaU9QTmRPa2JscTNWZ0NLanVIdlJDMXB5?=
 =?utf-8?B?ZTNIb043M09qVGlMckh2bTFHZDl5cm1Ma2p2Q0ZJUWhVRXJMT3dLVWtxd1RC?=
 =?utf-8?B?bDNYaHoxT3ZTcUJDK1dWWnNoeFBWa2VxekFXQjFIMC9iSCtzQ2x0SjdReEIv?=
 =?utf-8?B?VmFFZnZES1B5T1RGRHBYT3k1bU40TEtjWlg5NkhzbmpsRW56RVhtZlBaODVR?=
 =?utf-8?B?RzJkbHZybHl3VmZjVGVuSVVMRkhKcGdXQzRreVpQdUo1eVpJWDJkRHpXcHZV?=
 =?utf-8?B?ejYyVFZXWmYvdDQ0bkI3Vm1yeG5hRzhsb1hxeitlMHdOY0pZOGRvREdSbit0?=
 =?utf-8?B?QW5raXcvOEllcExZWkRTNHlmSXVjV2RFMW5wUXBXQ0hWTnN6NFRBbnVJeVRJ?=
 =?utf-8?B?VVdkOCtQQytEWUxrUkxiVlBJR2thdmhndXlxYmhTeElyclVHb1RrQkYyTVlP?=
 =?utf-8?B?WklBT1N6VUZ4aEVEMUQyd1pWS2VQcUlnMkVZR1d1OURBbWlJaGZsSkpHR1pn?=
 =?utf-8?B?OHkzUGFMS0xBczA0WXU0NUpyUnJleFRjaEJaaHFNK2VVRnNlNkVBUzF1UVR3?=
 =?utf-8?B?bFVmMTZGNS9rSzk3SVpZV2ZiOEhlS1BrZnYwekNSUUlocTdFZUVDeVRYTzhC?=
 =?utf-8?B?cDVWWUZHeXgyS1FlMlV5aEdWbWtLWFdIbWZVUzk0VlM1TDdxT25kUjhCTlRt?=
 =?utf-8?B?cmVZMEg0NDh2UThZSHRpeGFSTmVIc0NXbnhGNitxQm42Zy94c0c3RERYUXVV?=
 =?utf-8?B?VlRjVTNwNmtnY2dZS2lDVFdoNVVYd3BzS09XdUpudHFXN1kzVmlUS1RqWi9z?=
 =?utf-8?B?NTNpTWl5SkwwajR6UjFzcVR5M0NZNjEvcjNlS3Z2c2E1cFQ0US9HbUVSeFhU?=
 =?utf-8?B?Z0I5ZVptTWRZTE5oemdhMktvYnhBWGEwTDllaG1sR0xJSlUxb1dmN2J0cXZG?=
 =?utf-8?B?SEtUcDI0NWl3RTljYy93dUZ0MTFmQnZtd1laeFBud1NTK2NHZ1RwbTNXWmNR?=
 =?utf-8?B?eXh3aTVJMVJjYW5haXdkRVJJeks5NmR0WFQxd2V3dzVkZzNvTUY4Zit0V0Nw?=
 =?utf-8?B?bXFweUtZWTdUTkRyS0N3UVZXVGxCRlFlWFZlb0FSbCsyR2FXZStBTUNxUnEx?=
 =?utf-8?B?V3AvaVVYZHVHRWUwbENXbEJoV3RVa1NuMTcyc2JWSjVWeEdUU3ZxakhQTjNB?=
 =?utf-8?B?b3g2RlNVamIyR1U2T2VyQ2JhOHJkWnVJTUhQSFJPSlUxTXpKMW14WGdVV1lh?=
 =?utf-8?B?amRKU04rQ0dPVVJhM3ZTS0J3RFhkRk8zVkhBcXVVT21ESXRnT2RkeFhPUTda?=
 =?utf-8?B?b3c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e0347f82-7ffc-46e3-ae11-08dc8ca114ab
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2024 18:37:44.6415
 (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: 73TSVqxjtBY0+LTYI0TMuus677OSf4oPH1JaRzdRddFjNDtsvd0YDtuXCiy7NIjCY+RevizGxTELA/iBVz7LdSUySjydTuRDW1A2jBELlZo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4929
X-OriginatorOrg: intel.com

Hi Babu,

On 6/5/24 3:45 PM, Babu Moger wrote:
> In an effort to support MBM and MBA tests for AMD, renaming for variable
> and functions to generic names. For Intel, the memory controller is called

Changelog usually starts with some context and then problem to be solved. What
the patch does follows that. Maybe just something like:

	For Intel, the memory controller is called Integrated Memory
	Controller (IMC). For AMD, it is called Unified Memory Controller (UMC).
                                                                                 
	Change the names of variables and functions from imc to mc in preparation
	for support of MBM and MBA tests for AMD.
                                                                                 
	No functional change.

> Integrated Memory Controllers (IMC). For AMD, it is called Unified
> Memory Controller (UMC).
> 
> Change the names of variables and functions from imc (Integrated memory
> controller) to mc(Memory Controller). No functional change.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> @@ -349,10 +350,10 @@ static void do_imc_mem_bw_test(void)
>    *
>    * Return: = 0 on success. < 0 on failure.
>    */
> -static int get_mem_bw_imc(const char *bw_report, float *bw_imc)
> +static int get_mem_bw_mc(const char *bw_report, float *bw_mc)

The name of the function is expected to be changed in the function comments
also.

>   {
>   	float reads, writes, of_mul_read, of_mul_write;
> -	int imc;
> +	int mc;
>   
>   	/* Start all iMC counters to log values (both read and write) */

Was this intended to be MC?

>   	reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1;
> @@ -361,21 +362,21 @@ static int get_mem_bw_imc(const char *bw_report, float *bw_imc)
>   	 * Get results which are stored in struct type imc_counter_config
>   	 * Take overflow into consideration before calculating total bandwidth.
>   	 */

In below snippet imc_counter_config is renamed to mc_counter_config yet the
comment above was not changed to match.

> -	for (imc = 0; imc < imcs; imc++) {
> -		struct imc_counter_config *r =
> -			&imc_counters_config[imc][READ];
> -		struct imc_counter_config *w =
> -			&imc_counters_config[imc][WRITE];
> +	for (mc = 0; mc < mcs; mc++) {
> +		struct mc_counter_config *r =
> +			&mc_counters_config[mc][READ];
> +		struct mc_counter_config *w =
> +			&mc_counters_config[mc][WRITE];
>   

I noticed a couple of misses by just looking at this patch. You can
use grep to ensure that this patch does what you intend.

Reinette