Received-SPF: pass (google.com: domain of linux-kernel+bounces-206570-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <3d6587a4-3e39-449b-a108-9f6893d4a405@intel.com>
Date: Fri, 7 Jun 2024 10:54:36 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 02/16] selftests/resctrl: Calculate resctrl FS derived
 mem bw over sleep(1) only
To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	<linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>,
	=?UTF-8?Q?Maciej_Wiecz=C3=B3r-Retman?= <maciej.wieczor-retman@intel.com>,
	Babu Moger <babu.moger@amd.com>
CC: Fenghua Yu <fenghua.yu@intel.com>, <linux-kernel@vger.kernel.org>, "Shuah
 Khan" <skhan@linuxfoundation.org>
References: <20240607125316.7089-1-ilpo.jarvinen@linux.intel.com>
 <20240607125316.7089-3-ilpo.jarvinen@linux.intel.com>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <20240607125316.7089-3-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?KytLMWpZbDhQTHV0NTRYZVhBYnpIOW40WGpmSXp1VzBOZzFjZkdPaitOMmYv?=
 =?utf-8?B?Mk1oLzVBeUdjb3BiNWdDV2FhZDNzWmt0alVsUkhNTHpXbFRBL2N2VmZSSmtD?=
 =?utf-8?B?bHZOWWhOQXljSkJBd0o1RVRNZ082eXBoZG15a2tYd0NoeXB1YkczK2lOSUQy?=
 =?utf-8?B?bmRzdzF5bEFHYjhOZ1lMZzdUSXZkeFRjS3RNcTd4ZUNxTkVCQUtON0cxZmdi?=
 =?utf-8?B?WVlMMEpJWEQ5MnhzQ1AwZzUzZ0ZJcTZvbnhQb041L0NscmtINUt6d0RZZ1hM?=
 =?utf-8?B?cnJFLzZ3QW9PcWlUWVB1TTBYM3o2MHgvcEZjc0ZLNXM1ZXZEcGFuTER6akRv?=
 =?utf-8?B?cGxKaVFvaWovR0tRT1VucUlmY0dFL0JGUHFnRDVQSU00YWtwelJjTEx5OURO?=
 =?utf-8?B?dWl4RCs4Qk1DaStXQ3RiOWRZMFpFNnBuNms3eEVhQXptTDVPT0RWUlhQUnAw?=
 =?utf-8?B?VWl4Qm9GMTd0cExrYUNKYnR6ajk1RDQxa3FtTDFueVh3aTVsRmhmVkZ5UlhE?=
 =?utf-8?B?WHJqRkllaHZyNzFrR2s2amZReURGZXhIbENwczUycWZ3dzJ5aXdoemNvTjVU?=
 =?utf-8?B?TUEwc1NOZXJpWWRxZGM5NnorQU5zc005RmpGM3c3dG4reWx3WVVQWmtWYjBS?=
 =?utf-8?B?dy9KQVJZNDY2a3ZLOE1KTldUcldNdE91VEhKRm93N0hlYTVQUjNhVVBxWjlF?=
 =?utf-8?B?SWhGUEJIWEFkd3pxQnc2dDBqbEdCTjd1aDB3QkZXcFd3MnVycmJMUmlUM21H?=
 =?utf-8?B?NGYxSkpqTFhvemlQSkhZbGc2aVhXWUJ1dEVEUEVXU3oyeXFlRTgyMmFGSDZs?=
 =?utf-8?B?akJMdGdMS3lsbTN3NTREdEpOMmtsTVNHdzVYU1l1a3VGMm9HTnFleEhvT2tw?=
 =?utf-8?B?TTA0c0tPV3dscEp6cU1oNXlWcDZFYXFCTld6UURxN1RHT2E2RmJaNGRLSlRT?=
 =?utf-8?B?MFpxUUMyTFNYSU5BSXJlTnVhWkZVdlNRdWhtUVVORDdSZkVkY1UrYUp0bWNK?=
 =?utf-8?B?Rk43ZmhrZnk3WjJLa2YyN2VjMFB3QnFVT1hOTTNXK1hybG9UWDNrRWhWc2pw?=
 =?utf-8?B?ZzJPM3hUNkZOOGdkUFdFQkpzNWJCa1dhVHRUZUNyczZnUVhKdjd0Mzljd0t3?=
 =?utf-8?B?YXlUSGxQN284U01Pa040c24yQ205bjJxeW1vdDN6SzFvVmdUYUF3WXh2YWZS?=
 =?utf-8?B?Q0JhbDJ6SlFic3lzZXJaOHZQODJyaEpHNDlqSUkwd3dnUU1NZnN2MmRvZitU?=
 =?utf-8?B?RU9JeVF4S2s0ZXhmeWZuVHJsS3hxdjhXSzNJbTdDeVAwOXBZaWRPb01aMXBF?=
 =?utf-8?B?c2ZOVnhjcDB5YVoyakwyZmFnSFpvNGdXTXBxMGMzU2x5Ry9BTEhoc3lDWlJO?=
 =?utf-8?B?cUtKUktHYXhMK3AzcW5FZG9XdkxuWEd5ZzdCS0lCR0xsVjNONE10cjFQRDB1?=
 =?utf-8?B?M3VvRTJQRllINHNha3Q3TlNHUEpWQ3JhNE9waG1FcHZJZUtqSVZGMHpEWGhX?=
 =?utf-8?B?MUZ4MVhrblh4OU1DVnMweVZLUHFSNEtOOFRRYXZlc1hhOTZXdzlBSlB6SXNE?=
 =?utf-8?B?REFSUFVQZnVYa2FrcmpDSVRrLzBsSHU4T0xwQWt0MWNmTVdCYjFFMk9SVzRq?=
 =?utf-8?B?cGQrTVk1UXV6RUZNZzhaNTF4eEV4aG8zVXhBVDJrMHZLVHRjRU9BUzd5Y0pY?=
 =?utf-8?B?SnY3QlVWdzdhSGxqQmF6Z3hzMElyZDQxQ2ZMZHE0c3dQMEtJQjVVU2RUaE16?=
 =?utf-8?B?dUhZK2U5MzZFSjJ6L1o3bWZKV2hkS0RtdWF6SmcrbE5pMnlwMmRDNDJXQkJE?=
 =?utf-8?B?QWt5L3BMZEw4cXk0WXNCeW8vNlhDRW40eGd0V1ErcER4RDR1RW5hZm54dmh2?=
 =?utf-8?B?VkxqbFFhYVEzaE4xYlFDNVEzbU9EWGwwUUp0QXdzZlpldTNPWXlJSHlZVExp?=
 =?utf-8?B?QlptRnZaWXNSMVhTTmFpQ3orMFhLTm84dFNSek40T2tidk9BTS9rMkUvbWtp?=
 =?utf-8?B?Y0xVUWRzK0ZQOERuNG9YYUFMMnRnbFNIQmZlOVRSQTNpRkZLdEpjazJRS05x?=
 =?utf-8?B?ejNiM2tqc1FLWENNb1kzU2FWaG1pbG1LUGV2MnVZeXRDYklSZWdaTnVqSmt0?=
 =?utf-8?B?VTBFV2VINkc1YkFFcVM4U0RFR0gzL2FaT0hvVE9sY2txYzFnVWx3aHZBQlpQ?=
 =?utf-8?B?bnc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b1669384-a11e-4c94-4c59-08dc871ae662
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Jun 2024 17:54:38.5355
 (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: 6Am6AP0/tU9VhcpiBoYvsb9B1Um5UuSOE/zir3be57aGkJ/0BoYUQdmUwjk3JrG5qm2iHfkkpWo0pw9Caxn7LTjnwOoMvxYLJQZRQ7cgI1E=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5237
X-OriginatorOrg: intel.com

Hi Ilpo,

On 6/7/24 5:53 AM, Ilpo Järvinen wrote:
> For MBM/MBA tests, measure_vals() calls get_mem_bw_imc() that performs
> the measurement over a duration of sleep(1) call. The memory bandwidth
> numbers from IMC are derived over this duration. The resctrl FS derived
> memory bandwidth, however, is calculated inside measure_vals() and only
> takes delta between the previous value and the current one which
> besides the actual test, also samples inter-test noise.
> 
> Rework the logic in measure_vals() and get_mem_bw_imc() such that the
> resctrl FS memory bandwidth section covers much shorter duration
> closely matching that of the IMC perf counters to improve measurement
> accuracy.
> 
> For the second read after rewind() to return a fresh value, also
> newline has to be consumed by the fscanf().
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Tested-by: Babu Moger <babu.moger@amd.com>
> ---
> 
> v6:
> - Adjust closing/rollback of the IMC perf
> - Move the comment in measure_vals() to function level
> - Capitalize MBM
> - binded to -> bound to

This change does not match the patch.

> v5:
> - Open mem bw file once and use rewind()
> - Read \n from the mem bw file to allow rewind to return a new value.
> v4:
> - Open resctrl mem bw file (twice) beforehand to avoid opening it during
>    the test
> v3:
> - Don't drop Return: entry from perf_open_imc_mem_bw() func comment
> ---
>   tools/testing/selftests/resctrl/resctrl_val.c | 141 +++++++++++-------
>   1 file changed, 91 insertions(+), 50 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index f55f5989de72..1575c5c09ac7 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -306,18 +306,13 @@ static void perf_close_imc_mem_bw(void)
>   }
>   
>   /*
> - * get_mem_bw_imc:	Memory band width as reported by iMC counters
> - * @cpu_no:		CPU number that the benchmark PID is binded to
> - * @bw_report:		Bandwidth report type (reads, writes)
> - *
> - * Memory B/W utilized by a process on a socket can be calculated using
> - * iMC counters. Perf events are used to read these counters.
> + * perf_open_imc_mem_bw - Open perf fds for IMCs
> + * @cpu_no: CPU number that the benchmark PID is bounded to

"is bounded to" -> "is bound to"?

Just the one nitpick from me. The patch looks good to me. Thank you
very much.

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

Reinette