Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <93113f94-9663-4cbb-962f-c415bc975f12@intel.com>
Date:   Thu, 2 Nov 2023 10:49:26 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 10/24] selftests/resctrl: Remove nested calls in perf
 event handling
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: <20231024092634.7122-1-ilpo.jarvinen@linux.intel.com>
 <20231024092634.7122-11-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20231024092634.7122-11-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?dnIyckdpaTA0cW1XTnJ3MUU2QlhSMzhlWCtzYWNKaWhmYzE1MVRWM2psYjVT?=
 =?utf-8?B?WVFqT2pOcjFtOWNUdVl6U2JUaW0zanN6dUYreXNhcmswVXlFeUFocFBNL0Mw?=
 =?utf-8?B?d1gveFRYaUV1Y0s4cEZpWm81dXVpd21DdklKYW1LZWlhSkRTRU9YT0tXZnBR?=
 =?utf-8?B?YlUwTEUwSGZpNTdKdHJYWlpROG40VmwyT2NGcy9BOXZaRlhTRGdKUkNGcEpi?=
 =?utf-8?B?Z00xUlM2WnFlWnBxZHo3bHY5ek1LbjZ4TjU3OEV1bmdPZDNreEFFQ0RUeExl?=
 =?utf-8?B?OFp0TmlVaHZ3dm1BTjFPMFE5R2U5NFF0WXVpcVlVZ0pVRnNqQVdhS005dEhz?=
 =?utf-8?B?OXBCQkNhYjVWWGpqZUJ5cTFFWU9HcjR6UEhHRFNZVW9QUDNYSzEyc1psNXZW?=
 =?utf-8?B?R1hPemZKWWg0UVNLV3pvUDdWRWE3WnBhWVFEeFBjZU9Wd05Xc0V0bXhVM0ZF?=
 =?utf-8?B?ZmNOakVnUk5aYkVWbVR1L1dBaWdxdnRYWlRzM0FFRlV5U0Y1cE1Nd215OHZU?=
 =?utf-8?B?TFJ1YzI2MC9tcHZtbTV4SmZvN3lUQjYvemo2WmxjSDRmYXBqRy9YcUhrTzhO?=
 =?utf-8?B?RkFxSVM1bnJpT2tFOHdxUVVSaWlzcXIvNnZDUnVYZFRrSGZNVVZLUW9FdGNK?=
 =?utf-8?B?N3NxNHg2eGhxOGp5MTlqMkFCQi9RU0M3ZDJ2WEJ0L1YvckFVRGovNmN5aFpS?=
 =?utf-8?B?WXo5eCtFMmo4cjFxd21LZE5tZlhrQ21xTndCM2FsUVdncFNXcS9HWllnRmps?=
 =?utf-8?B?dTVUeUttZE8wN21LTllJUGROWVVxWWFWNWozRmt6TnNTcExjOEJkSFhUWEpT?=
 =?utf-8?B?aUt1N3EzSmZ3aEdEVElodk5DVGltUU5CcENrL2Y5UE1DQmdvL3cyNE41TEs2?=
 =?utf-8?B?dHJUcnRxQysyMnd1OU9NME1HSEIwMStxMGN3Nnk5c01vOXRCNmJSbzBwa3Na?=
 =?utf-8?B?R1ltVmRKQ2VBVkYyNmlkVHpwOExVVTM3L0RWV29sN1ZSRUdCdENmbWJpcFVh?=
 =?utf-8?B?cW1DdXUvYmt3MlVPQ2xoNkxTanBpd0taTVdWWjlqNElQK0JUZ2pzWW5rMTZP?=
 =?utf-8?B?Y1g0eVVsMlNzR0dUWXJtTi9RWnl3V0w2VEN1MFdTK3Fhb0tScFcxckhYSnFr?=
 =?utf-8?B?UXhBQit6WTN0UVVpQWVNNFF1WGlYaHZnUmtvVUVaZnhiV2VMZmlUYmhMS2NP?=
 =?utf-8?B?ekFZanFwbFdXNTUyaXM1a0FCOC9BblVWKytDbnJ4V3FWbHYxUldoNVdyczdM?=
 =?utf-8?B?Wjg0U3VoaEp0eVFIMWM5ZElUdnRwSU1jWjhZZ3pIL3pHSGhycnd0aVo5Mmk1?=
 =?utf-8?B?YWhSMmcvTm5EZC9qV2RqWE1MVVN6UUJ1VkdaS3NzNEc0S3lNWmdidExpS1pu?=
 =?utf-8?B?U2QvamkvRGZzbmRiYWNOWlRNZ2x2UDd4bTJvb3dQT1hSdGVhOTdGSm9aU3Z1?=
 =?utf-8?B?ZUg4Y0JuL25QQkE1NWJsa25GSktXNmVXekR1UTE3aFJZeERyWjE1clV4Ty90?=
 =?utf-8?B?K2N1WE45UDRaWXA0YUZ0ekNDVXZZdldITEdXOUV3MDJ4VDQ1RjBaNmFESWxF?=
 =?utf-8?B?RVd2c2s0dlkwb0NLVExaUmNzSkNtWElkR3QzYTIxaElkRXhlVlRhUGR6Kzdm?=
 =?utf-8?B?VU1NdlUxcXBqc0FRTElQVXV1bUhHUzJyKzhCMjNjWWRTSzdrWlVZZ1B5ZmZY?=
 =?utf-8?B?blovZEhOc28yNnY1NlIraUw2K05ySG1HQmV1SzZpaVdsVy9vWG1FM2Q5c3VT?=
 =?utf-8?B?bXRVRnlVV0xYei8xUFUxQ3c2OHZTdEJpL0Y0U05DZHRmZ2Vhd3Ixc2s3MHlw?=
 =?utf-8?B?RW5RZ3dqci9rUzRHazhoUFFkckxpN3BkSHVsSEEzWDl6emJHa0VQV25CTXRU?=
 =?utf-8?B?cnRXSlh6eHo3MWt0bDc0SU1MSDJRSW90S0lMMjRTdVJhNmZ6Sll4WEtSWHk1?=
 =?utf-8?B?K1FnTUZOTHFFZ3NyQThCRG01RVQ4R2FlUU0reCtLWkdYQ244VDREYjRHNDBm?=
 =?utf-8?B?RVVrOGZGVjJXWnpqS3NVdXRlN016bjRKOGJ3WGU0NkRjWEF1T1hoeWVQUDFM?=
 =?utf-8?B?Y0gvSXZITFpVYlRHNmFkYi9EdHBMalFscTFMV25nYzhRS2pqVW5MUnNiUWlS?=
 =?utf-8?B?N1h5RE1IVnVmcG1aUTFKeU1oV3dNWHBQRmhvV3EyVUJUSVZhNUNWNVBRbWtn?=
 =?utf-8?B?SVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 129f7573-5bcc-4c44-6a8a-08dbdbcc0f30
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Nov 2023 17:49:27.9170
 (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: BX6gpBVMhQvh2OLnO+WpIMIeMBupuAWNiJJA39rTR2GcMm6xyGZ9yaBrNil6QqZeCrHjiUADy7DV0aj4CSGBWZRwmH/iZiG7CTVjqAisN1c=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB7250
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-1.3 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,
	URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.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 (fry.vger.email [0.0.0.0]); Thu, 02 Nov 2023 10:50:13 -0700 (PDT)

Hi Ilpo,

On 10/24/2023 2:26 AM, Ilpo Järvinen wrote:
> Perf event handling has functions that are the sole caller of another
> perf event handling related function:
>   - reset_enable_llc_perf() calls perf_event_open_llc_miss()
>   - reset_enable_llc_perf() calls ioctl_perf_event_ioc_reset_enable()
>   - measure_llc_perf() calls get_llc_perf()
> 
> Remove the extra layer of calls to make the code easier to follow by
> moving the code into the calling function.
> 
> In addition, converts print_results_cache() unsigned long parameter to
> __u64 that matches the type coming from perf.

Is this referring to work from previous patch?

> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/cache.c | 86 +++++++------------------
>  1 file changed, 25 insertions(+), 61 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> index d39ef4eebc37..208af1ecae28 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -29,25 +29,6 @@ static void initialize_perf_event_attr(void)
>  	pea_llc_miss.disabled = 1;
>  }
>  
> -static void ioctl_perf_event_ioc_reset_enable(void)
> -{
> -	ioctl(fd_lm, PERF_EVENT_IOC_RESET, 0);
> -	ioctl(fd_lm, PERF_EVENT_IOC_ENABLE, 0);
> -}
> -
> -static int perf_event_open_llc_miss(pid_t pid, int cpu_no)
> -{
> -	fd_lm = perf_event_open(&pea_llc_miss, pid, cpu_no, -1,
> -				PERF_FLAG_FD_CLOEXEC);
> -	if (fd_lm == -1) {
> -		perror("Error opening leader");
> -		ctrlc_handler(0, NULL, NULL);
> -		return -1;
> -	}
> -
> -	return 0;
> -}
> -
>  static void initialize_llc_perf(void)
>  {
>  	memset(&pea_llc_miss, 0, sizeof(struct perf_event_attr));
> @@ -63,42 +44,16 @@ static void initialize_llc_perf(void)
>  
>  static int reset_enable_llc_perf(pid_t pid, int cpu_no)
>  {
> -	int ret = 0;
> -
> -	ret = perf_event_open_llc_miss(pid, cpu_no);
> -	if (ret < 0)
> -		return ret;
> -
> -	/* Start counters to log values */
> -	ioctl_perf_event_ioc_reset_enable();
> -
> -	return 0;
> -}
> -
> -/*
> - * get_llc_perf:	llc cache miss through perf events
> - * @llc_perf_miss:	LLC miss counter that is filled on success
> - *
> - * Perf events like HW_CACHE_MISSES could be used to validate number of
> - * cache lines allocated.
> - *
> - * Return: =0 on success.  <0 on failure.
> - */
> -static int get_llc_perf(__u64 *llc_perf_miss)
> -{
> -	int ret;
> -
> -	/* Stop counters after one span to get miss rate */
> -
> -	ioctl(fd_lm, PERF_EVENT_IOC_DISABLE, 0);
> -
> -	ret = read(fd_lm, &rf_cqm, sizeof(struct read_format));
> -	if (ret == -1) {
> -		perror("Could not get llc misses through perf");
> +	fd_lm = perf_event_open(&pea_llc_miss, pid, cpu_no, -1, PERF_FLAG_FD_CLOEXEC);
> +	if (fd_lm == -1) {
> +		perror("Error opening leader");
> +		ctrlc_handler(0, NULL, NULL);

I understand you just copied the code here ... but it is not clear to me
why this particular error handling deserves a ctrlc_handler().

>  		return -1;
>  	}
>  
> -	*llc_perf_miss = rf_cqm.values[0].value;
> +	/* Start counters to log values */
> +	ioctl(fd_lm, PERF_EVENT_IOC_RESET, 0);
> +	ioctl(fd_lm, PERF_EVENT_IOC_ENABLE, 0);
>  
>  	return 0;
>  }
> @@ -166,20 +121,29 @@ static int print_results_cache(char *filename, int bm_pid, __u64 llc_value)
>  	return 0;
>  }
>  
> +/*
> + * measure_llc_perf:	measure perf events
> + * @bm_pid:	child pid that runs benchmark

I expected "bm_pid" to reflect a "benchmark pid" that
is not unique to the child. Are both parent and child
not running the benchmark?

Missing doc of a parameter here.

> + *
> + * Measure things like cache misses from perf events.

"things like cache misses" is vague. The function's name 
still contains "llc" which makes me think it is not quite
generic yet.


> + *
> + * Return: =0 on success.  <0 on failure.
> + */
>  static int measure_llc_perf(struct resctrl_val_param *param, int bm_pid)
>  {
> -	__u64 llc_perf_miss = 0;
>  	int ret;
>  
> -	/*
> -	 * Measure cache miss from perf.
> -	 */
> -	ret = get_llc_perf(&llc_perf_miss);
> -	if (ret < 0)
> -		return ret;
> +	/* Stop counters after one span to get miss rate */
> +	ioctl(fd_lm, PERF_EVENT_IOC_DISABLE, 0);
>  
> -	ret = print_results_cache(param->filename, bm_pid, llc_perf_miss);
> -	return ret;
> +	ret = read(fd_lm, &rf_cqm, sizeof(struct read_format));
> +	close(fd_lm);

I am not able to tell where this close() moved from.

> +	if (ret == -1) {
> +		perror("Could not get perf value");
> +		return -1;
> +	}
> +
> +	return print_results_cache(param->filename, bm_pid, rf_cqm.values[0].value);
>  }
>  
>  int measure_llc_resctrl(struct resctrl_val_param *param, int bm_pid)

Reinette