Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a4fa6303-4637-815a-e0fa-57f33babfb10@intel.com>
Date:   Thu, 13 Jul 2023 15:52:33 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.13.0
Subject: Re: [PATCH v4 04/19] selftests/resctrl: Close perf value read fd on
 errors
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>,
        Fenghua Yu <fenghua.yu@intel.com>,
        "Sai Praneeth Prakhya" <sai.praneeth.prakhya@intel.com>,
        Babu Moger <babu.moger@amd.com>, <linux-kernel@vger.kernel.org>
References: <20230713131932.133258-1-ilpo.jarvinen@linux.intel.com>
 <20230713131932.133258-5-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230713131932.133258-5-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?cU0yWENLUERBais2Rk5hajZPUG1XdDJybHdDdkM0ZlNTYU9kcVBGN05pbjJn?=
 =?utf-8?B?bm9wNE1YUlFoMFdkRUpTeVFnWEwwUU4zdFY4aHVCOHU2czJBM2owMTVXQnpF?=
 =?utf-8?B?T3FyTkZxekc5YzdYUGxQVjlSbGhoTWRrTDdSbmQyUFhBdU1haXZSSGgvVWhJ?=
 =?utf-8?B?VnFKclFTbTBnZzB0U2NnRnAxWDd0anZvTEdjL1NQdGp5dEx2MFc5TGdrc3l0?=
 =?utf-8?B?b0JWL1ZVeEtHT1krVVp5RlF1M096WEx2TEo5OHhKcUdjck5RU3owZXBDQjVE?=
 =?utf-8?B?YkpMTWNDN0NnWHptQ1E3TnNqQUZ5cWFpV1pMN2pGSUxCd2JGbUErb2g4T0Ez?=
 =?utf-8?B?LzhNWVhqS1c2OGlVRVpidUY1NHFDSDBLWlplMDJlbWhBcUYyQWZ1L2Y2dDhR?=
 =?utf-8?B?YlZSN3FlNE5FRkIwTDhNck4xczhrOEd1TjVlZGxLSE8rbTJWd2VvamFvbWh3?=
 =?utf-8?B?ZEgwdDZ4SldDdGFPU2FiNnBvTjlNRkFrYmo1R0M3Yk5Ub1UrSWNQUnJjUThU?=
 =?utf-8?B?WWVObS9pMzl2cXhyN0VyUFhBdkNjVnhIU0VLdnA5V3J3RGZZSHhsUlNWa0hX?=
 =?utf-8?B?V1JpVkpCUWNPWFZCM1l6UERNSExiWm5Udi9OWWdUdnJtTy9ZQzBCY0FiVGlW?=
 =?utf-8?B?RFFYc3grcG5uSEJ3S1JObkFvUHUxKzlya2tsSndGUFlQSnVaNmJXbVRPUUxN?=
 =?utf-8?B?d0YzMlFpaE92ZWk2bENnL213S3ZyQmNGTWJzV202ZXR3UnlRdGFONFd0NzUy?=
 =?utf-8?B?czNCUE5ReGQveDhJWlYvWmNPUkRTbnUvRDlPUDNOemR2ZFY4eWdyREJneGxN?=
 =?utf-8?B?RmpGbnk2YTZhOUpjeUxTSXpGbnRrNVhYSXBXOVgyVUloNStxZjhZV3VhQ3Nn?=
 =?utf-8?B?SnBJVXRDdDNWemp4RUZoZElidElmTksyZlpyZVZSUitrVEV2UlpBSFVHbzhp?=
 =?utf-8?B?YitJdVN4cGdXMmFtVWVCcm5UZWVTcWpkc00yait0UW52azk5dmVhMmpFcklO?=
 =?utf-8?B?ci9KblM4VExLUHVaNUt6NDFoaDBYZzJOSlBjMkxjaXVGV1JkTTJ0LzBNcnJI?=
 =?utf-8?B?ZVlSTnB4cTY2ZmpMV1JNN3VCWlZVYVhzTlhSLzc5a3VDaWlaMk9LK0d3Yjlq?=
 =?utf-8?B?R1hReVJMOUx4azdEMVE2eTMvamVTeDR5WklRMDluV2N6dGhvcnhVclVCaWJ5?=
 =?utf-8?B?RnlTSWZ1TDI3KzZGcmJhWlBYUFFSVmFKRjNQV3FPdlNEU1JiUVNIWUZEMFZv?=
 =?utf-8?B?ZXdnUDlEdTQ2KzFPT1AzUWZBeUVMbmVVT1E5cnlOUlRodWhReGFuRFZ4Q3Bl?=
 =?utf-8?B?N1pHUUUwbFpISU44V1VYVUQ1ckRFdW1nZ3VabFBTTnV2ZzZYUjEveURPRGVH?=
 =?utf-8?B?YTlSLzBVQXFETkxjYTA4WUYzWkJ2cUJybm0wandOSGFjbGxRNjEvNTNDVjRE?=
 =?utf-8?B?bllkc1JYK1BRU1NmWXBuU0VlN1FKeVg3NXlnTnlsdnMxYWxoZjVscGZlR2pa?=
 =?utf-8?B?VmZTV2k2ODdsYWZmWi82VXYyVlRVcG5IWHBjdDN5ZlpaWEdUUjN4alBpQm0r?=
 =?utf-8?B?bDlMS1BrWnNFbk13cVR5YlVkU3JoM0VBaEYvL2tNdGlMVnhHeVcyOXFiOTI2?=
 =?utf-8?B?YjNUNEpEa1MxYTRMOG5WZ3V3RWw4NnVQajVpazcrVm5aZWNiMWdHbmd6VGJJ?=
 =?utf-8?B?Ry9jNzhXc2ZKVHo2blROWk82UURNZmp3TitwUnFQU3FmMkMrd1hNdmUwVENn?=
 =?utf-8?B?SnRQSWlKV1RxT0dzTGUyRzhXN2s5dktlMDhJTDF6RVUvTG50aTFNOFR0RXR5?=
 =?utf-8?B?STRkTEJxMGVvTGhrZXZCUWttdTk3WTYwcmtjU3BiRkJxVUdrc3BpZnhTVmV1?=
 =?utf-8?B?YU1qd3M1MWtsbE1kbVBTbE1iekRpYnNES25HVzFVZituT3lCOHJwczF2a0NR?=
 =?utf-8?B?YURvZDJaK2tkSGdPU1BCOW4reGt3a3ZJNFpPYVA4c2dqNWx6UjFzMERBbDRS?=
 =?utf-8?B?R3FZdUNpeWpOZUZSV2VQdTMvYlNYY1NOUU5ZaUpOZk04Z1dyaDJrZDVyTGhB?=
 =?utf-8?B?YlNMd0JweldkR2tkd0ZsdDFmQ2M1OGRkcnZabGE1S2IrMFAyZ2oxZHBabXFO?=
 =?utf-8?B?bnlXQVRTaHgyVTVjWUlwRHJpVHl2bXU0TDVYT3RraTEySC92RnpVWGdkVlJN?=
 =?utf-8?B?dGc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c1421416-4875-46e8-264f-08db83f3d9d9
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jul 2023 22:52:35.9912
 (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: t7BPOpa7oFugaRR88IX68xlMXzrAAHKLeHuaHU3jDVkbcPgcgqXGJM8rZzDc9oZJLGBC9DSK6EvvIAu+y0CFKNEyEm0QqR8FmA3E3eXAAd4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4689
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_BLOCKED,SPF_HELO_PASS,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Ilpo,

On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> Perf event fd (fd_lm) is not closed on some error paths.
> 
> Always close fd_lm in get_llc_perf() and add close into an error
> handling block in cat_val().
> 
> Fixes: 790bf585b0ee ("selftests/resctrl: Add Cache Allocation Technology (CAT) selftest")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/cache.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> index 8a4fe8693be6..ced47b445d1e 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -87,21 +87,20 @@ static int reset_enable_llc_perf(pid_t pid, int cpu_no)
>  static int get_llc_perf(unsigned long *llc_perf_miss)
>  {
>  	__u64 total_misses;
> +	int ret;
>  
>  	/* Stop counters after one span to get miss rate */
>  
>  	ioctl(fd_lm, PERF_EVENT_IOC_DISABLE, 0);
>  
> -	if (read(fd_lm, &rf_cqm, sizeof(struct read_format)) == -1) {
> +	ret = read(fd_lm, &rf_cqm, sizeof(struct read_format));
> +	close(fd_lm);
> +	if (ret == -1) {
>  		perror("Could not get llc misses through perf");
> -
>  		return -1;
>  	}
>  
>  	total_misses = rf_cqm.values[0].value;
> -
> -	close(fd_lm);
> -
>  	*llc_perf_miss = total_misses;
>  
>  	return 0;
> @@ -253,6 +252,7 @@ int cat_val(struct resctrl_val_param *param)
>  					 memflush, operation, resctrl_val)) {
>  				fprintf(stderr, "Error-running fill buffer\n");
>  				ret = -1;
> +				close(fd_lm);
>  				break;
>  			}
>  

Instead of fixing these existing patterns I think it would make the code
easier to understand and maintain if it is made symmetrical.
Having the perf event fd opened in one place but its close()
scattered elsewhere has the potential for confusion and making later
mistakes easy to miss.

What if perf event fd is closed in a new "disable_llc_perf()" that
is matched with "reset_enable_llc_perf()" and called
from cat_val()?

I think this raises another issue with the test trickery where
measure_cache_vals() has some assumptions about state based on the
test name.

Reinette