Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <c303ba1b-d7bd-47cf-9e81-8ea0c60b973c@intel.com>
Date:   Tue, 28 Nov 2023 14:13:19 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 08/26] selftests/resctrl: Split measure_cache_vals()
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        <linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>,
        "Shaopeng Tan" <tan.shaopeng@jp.fujitsu.com>,
        =?UTF-8?Q?Maciej_Wiecz=C3=B3r-Retman?= 
        <maciej.wieczor-retman@intel.com>,
        Fenghua Yu <fenghua.yu@intel.com>
CC:     <linux-kernel@vger.kernel.org>
References: <20231120111340.7805-1-ilpo.jarvinen@linux.intel.com>
 <20231120111340.7805-9-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20231120111340.7805-9-ilpo.jarvinen@linux.intel.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?cUpib3NZK3NuMTdwWGdOTWFTNXIxN3hvcExIQkZ4OUJvRGc4L3JGQ0NHWnRC?=
 =?utf-8?B?Nm9UeFlRN2ZBVmY1bWN3RDl4WmJmcnp6MTVtajhZcUtEVGlPa0xNbzRna0U5?=
 =?utf-8?B?ZXZndW1zNHV4MVEyVUhjTVNSWklYWmozZkFxZTQ3Y2pjUDE1enltVlQzTVhh?=
 =?utf-8?B?ZDdZQXVCdHFodnV1UWRmME9zSmZobXpMTktmNm5BakxYWTBOOVJVMFhBVDFv?=
 =?utf-8?B?bExhdFh6b2NpSUZSZDdJWkI4bWowcW1pOEpBVkFpZFJTSGpFV1RXck9UY2Fi?=
 =?utf-8?B?NjdlQldwNExTbTJoOFZYWTJJUitlc3A4TUJMWTRLYXlDRnFRcFZOYmdtWGRW?=
 =?utf-8?B?ek5pV25yeGFNVkU1L0xMdmxRczkxRUwzaXBSRzlSNS9VdHVpYldKUnUvd24y?=
 =?utf-8?B?STQ2bVdaaURWWHhQQmlkdzRYeUZzaVRnOG8zcUQ5TU5ZMDhXVUZZaSthZUtR?=
 =?utf-8?B?RG51NW5tSU9BTXZ4eFR1Q09Bek9nL21YZkxSQitWZ3FqYlc0SDBFTkx1alQ5?=
 =?utf-8?B?cmQ0UGNzRXpWSTRuTEV2R01sc2RZOWtTVmE0OGJyNFh5am9KTTNQcGJKeFNl?=
 =?utf-8?B?QXE0ak9EQW50NFBCK2tOcTRQREhpZTVJSVQzMDJ6Sk9GR0VucFNxWmVIVW5p?=
 =?utf-8?B?aGVOT1NpVElDdmpFUWFkazJYT09jbXZHejRZaUIxbXhSNmg0Y3owSHdETnlj?=
 =?utf-8?B?bHZhNUE1dE96OW9lYmVjM0RFbVBlVFVVUDgyaU53WGk3ZmU2cCtLY2MrcUYv?=
 =?utf-8?B?ejh6WHdNTk5YM1Nzbm9YYU9tWkFJakRIY21MTTdpelU0WnFZNnNFSm81WThn?=
 =?utf-8?B?Ym10QlFlOHNIV0puQW9TenZTMVJFRWQwN01kSSs2eENiK05wejB6cFN3TWlk?=
 =?utf-8?B?NXd4RVF5cmRNM05HVWIwU21rT3ZLVWxtU09YYUhCOTAyZFM5SHdzRHRHTHlk?=
 =?utf-8?B?YzNJd2xPMG9vNkNORHordVc3RjR1UllIKzY1cDZVMlJncmRrZGxCUmVhSkRT?=
 =?utf-8?B?TXhKY0dTL1BxU2pUYW5uU2JmSmNKcGdKNXozSHI2L3hlMWxZbzBRMWxVcC9Y?=
 =?utf-8?B?dENHUVpFcFdlMnU2T1lHSllZK2tON1NaV0hZK05mY0QrT1YyRVJEM2xLdXRl?=
 =?utf-8?B?UU9HemJkcU8ydnZ4aDR2UGdCcHdMbFFWNVF1c1A2cHNPS0pLalNiSU1mdDgz?=
 =?utf-8?B?S0lRNmJuTUxXdHQ3bnhkaStISDQxREREYlNNVjBOa3NSMjJETGtUalpCcUhT?=
 =?utf-8?B?YXUwRDZMWDlkUlFBVlZXUExSajRXeW5nRXBUWHdZK1pHN3dZRmZIOGR3TWRl?=
 =?utf-8?B?d0tzY1Q5SFBsdU1raWFvRDZMdTY0ZVFrbE5wZVNZS1NwdlJxTzcvb1BPSzFP?=
 =?utf-8?B?MVNwdkJVa3NpcE0vYWp3VkFEVlo0YUh1WXAzV21SYlg4REhMc2kvV2xCMlEy?=
 =?utf-8?B?d0RDSk1UWVZjbldobFZJdHYzczViNzAvQ0lLUC9xNG4wdWpNczh0ZlJ5TE9Q?=
 =?utf-8?B?RUNnYmhqM2RRR3hvZk1rYkZzQzVTWlowQWV3cmNHZnZGNFF2TUFHK3EveGwz?=
 =?utf-8?B?NEp4UG5yUG1haDdqVW1ZNW1kR2hRSTRUYk53Wk5GdzlTeHp0d2JldTI5YzRu?=
 =?utf-8?B?RUVtaUx2NkJzUnY1ckxiNjluRDQzMlVnTytTVU0zb1p5cHN3OThtZnJHOERN?=
 =?utf-8?B?ME1XaitYaXJrOThVVWllWDg0NUdWUXZCNVVtT3psNkVTb0gxdXlPbUZ1UmdN?=
 =?utf-8?B?UnZaUk1EVjRjUThKZUt1WFlXWkUrTldIZks1ZVhRTDhnazNOcXc0L09HTkJD?=
 =?utf-8?B?Yy9OcHQyQ0JkMW9jcGlPRmRLWWpudjJBRG1leFFONC9aRXl3MXpVYW1ONE5w?=
 =?utf-8?B?RloyK3pVTlpHdGxnc0xuc3BsWXJVamtKbTRlQngydi85Vm1tY0hmSXFEZzly?=
 =?utf-8?B?cVU3U0dVN2R3akw4M3duQkE3MjY4dHJ4cGZUa25nM1VsYjAzSzNQdTJMZTRI?=
 =?utf-8?B?MHg0TVlISG9VZDZNRU14WWwxZnhJOW5IMzhzSFZFUzUzYXRjSHZvK3RiMWN0?=
 =?utf-8?B?OEQ4ODQzNHpqRkh2b1RsTFlseldQWFdDdWtPRWM2SXd5bE1WbDFDS0NseUNK?=
 =?utf-8?B?WjlWbnlnSTgwRkxyazRpWlVEQ0o5b0txdURFTWFPV0V4TFVsTUdaaGpneUlO?=
 =?utf-8?B?Ync9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4d44a806-d66a-44c2-1896-08dbf05f3a5b
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Nov 2023 22:13:19.5938
 (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: SYPmI184zW3gVv39CaqeYRb58ji4Wa54QxzBEvjzxSgG+wO2ZkhziFTCjyPLp0wFD9kOGH3rWUHRVp1IzqUzQ0uDquYlBsaFIS95KMG+R64=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB8284
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Tue, 28 Nov 2023 14:13:45 -0800 (PST)

Hi Ilpo,

On 11/20/2023 3:13 AM, Ilpo Järvinen wrote:
> measure_cache_vals() does a different thing depending on the test case
> that called it:
>   - For CAT, it measures LLC misses through perf.
>   - For CMT, it measures LLC occupancy through resctrl.
> 
> Split these two functionalities into own functions the CAT and CMT
> tests can call directly. Replace passing the struct resctrl_val_param
> parameter with the filename because it's more generic and all those
> functions need out of resctrl_val.
> 
> Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/cache.c       | 66 ++++++++++++-------
>  tools/testing/selftests/resctrl/resctrl.h     |  2 +-
>  tools/testing/selftests/resctrl/resctrl_val.c |  2 +-
>  3 files changed, 43 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> index 8aa6d67db978..129d1c293518 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -147,7 +147,7 @@ static int get_llc_occu_resctrl(unsigned long *llc_occupancy)
>   *
>   * Return:		0 on success. non-zero on failure.
>   */
> -static int print_results_cache(char *filename, int bm_pid,
> +static int print_results_cache(const char *filename, int bm_pid,
>  			       unsigned long llc_value)
>  {
>  	FILE *fp;
> @@ -169,35 +169,51 @@ static int print_results_cache(char *filename, int bm_pid,
>  	return 0;
>  }
>  
> -int measure_cache_vals(struct resctrl_val_param *param, int bm_pid)
> +/*
> + * perf_event_measure - Measure perf events
> + * @filename:	Filename for writing the results
> + * @bm_pid:	PID that runs the benchmark
> + *
> + * Measures perf events (e.g., cache misses) and writes the results into
> + * @filename. @bm_pid is written to the results file along with the measured
> + * value.
> + *
> + * Return: =0 on success. <0 on failure.

I do not think this is accurate. It looks like this function returns
the return value of print_results_cache() which returns errno on failure.
If this is the case then I think this proves that returning a
positive integer on failure should be avoided since it just creates
traps.

> + */
> +static int perf_event_measure(const char *filename, int bm_pid)
>  {
> -	unsigned long llc_perf_miss = 0, llc_occu_resc = 0, llc_value = 0;
> +	unsigned long llc_perf_miss = 0;
>  	int ret;
>  
> -	/*
> -	 * Measure cache miss from perf.
> -	 */
> -	if (!strncmp(param->resctrl_val, CAT_STR, sizeof(CAT_STR))) {
> -		ret = get_llc_perf(&llc_perf_miss);
> -		if (ret < 0)
> -			return ret;
> -		llc_value = llc_perf_miss;
> -	}
> +	ret = get_llc_perf(&llc_perf_miss);
> +	if (ret < 0)
> +		return ret;
>  
> -	/*
> -	 * Measure llc occupancy from resctrl.
> -	 */
> -	if (!strncmp(param->resctrl_val, CMT_STR, sizeof(CMT_STR))) {
> -		ret = get_llc_occu_resctrl(&llc_occu_resc);
> -		if (ret < 0)
> -			return ret;
> -		llc_value = llc_occu_resc;
> -	}
> -	ret = print_results_cache(param->filename, bm_pid, llc_value);
> -	if (ret)
> +	ret = print_results_cache(filename, bm_pid, llc_perf_miss);
> +	return ret;
> +}

Perhaps print_results_cache() can be made to return negative error
and this just be "return print_results_cache(...)" and the function
comment be accurate?

> +
> +/*
> + * measure_llc_resctrl - Measure resctrl llc value from resctrl

llc -> LLC

> + * @filename:	Filename for writing the results
> + * @bm_pid:	PID that runs the benchmark
> + *
> + * Measures llc occupancy from resctrl and writes the results into @filename.

llc -> LLC

> + * @bm_pid is written to the results file along with the measured value.
> + *
> + * Return: =0 on success. <0 on failure.

same issue ?


Reinette