Message-ID: <7a0a068f-7001-4e59-9e4d-cd4b3a148bb4@intel.com>
Date:   Mon, 13 Feb 2023 11:21:01 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.7.1
Subject: Re: [PATCH v7 4/6] selftests/resctrl: Cleanup properly when an error
 occurs in CAT test
Content-Language: en-US
To:     Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Shuah Khan <shuah@kernel.org>
CC:     <linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>
References: <20230213062428.1721572-1-tan.shaopeng@jp.fujitsu.com>
 <20230213062428.1721572-5-tan.shaopeng@jp.fujitsu.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230213062428.1721572-5-tan.shaopeng@jp.fujitsu.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MkF5RTd5VWZyQklDazNsZUNUZU5CUnRFUHJBZER3VUwyckNrRWJSL3p6aVcx?=
 =?utf-8?B?c292WDNFVFZ0Qkk5cDZVdm5MTUhiaUE0OWZkUklkZjlrd2lraE5TcTZ3TENq?=
 =?utf-8?B?UTByRi81cUU4NktWczdjMUZoN0EvdlpGWUVFeUNuNmxhRmJrdmVKaHkrQzNI?=
 =?utf-8?B?R1p5dXR5bnE2MGhWT0JyS3d3ZDVvKy91TGJoV1hnWnVOMU12UVR1ZThJa2JS?=
 =?utf-8?B?aTRyc2RReUNWTm9QOUpTcExKTU9aN2RSemdnY0Q2UHpVUWtJcXM4alhGOXBM?=
 =?utf-8?B?L1hLVnFTKzVqNnl6WDJwVFNsZ2tXMm16U3pPaGpvTHd0YjVmL3ZJQ1pQVTBV?=
 =?utf-8?B?RmVpZFZJekdaRmdzVWhKdVd5ODloeU5Zakc3ZHppcjJsSG0vTlhHRk82RkFY?=
 =?utf-8?B?OGM4MHpZYXdiYnhZd3J6RzJqRmJobHluRS9wUWJvekRaM3hWL0p1SGo5Tm9t?=
 =?utf-8?B?eXZUamxieVRnL1h6NHgvWXlXRHRKZjh2VTc5K1ZsVVhBVjI2VmNSSVZXNWpB?=
 =?utf-8?B?aDlsd25xdjV3NjUwZjFjRUhTVGF0cEJkcFFEdGN6T0MrcDZubHdPZElsVnVj?=
 =?utf-8?B?dUs4ZkV4T3NCaTJoZ2JIZUVhVDAzSFhEb3lkWUsyU0x2ejdSWHBrR29DQWhT?=
 =?utf-8?B?azJSU2RCZEtGM1lGK0RaaC9POFJHTHpHSll3QTZWTFJkejdPSEo3ZWVuTWxi?=
 =?utf-8?B?SXdSTlV1QmhJWDNiMGZOcDFzZk9xenBMaVo4dzh0bitUR0ZUREZVUVQwK3Ra?=
 =?utf-8?B?TGo2QW5kNTM2TWZGRkVQOVBjMEk3RUh3MlRPSFNnQzR5MVZlTGNuREkxZ0Vx?=
 =?utf-8?B?Sng5MjhxamZmWlkvRFlMTlAybXZzeEFXOERzalNKcWNpT1MwNFJQYmVjSHZJ?=
 =?utf-8?B?bTIxcDVxVVFPNk5uS2JTTDFhMHVCU1pONnBLZXdxeWd6R3J2NW1EUTJCakZY?=
 =?utf-8?B?T2N1QUhVckpFU3JkZ28ySU9qeXFjWWY5NDE1UVM1bjdibTlrQ3BYUlQyazc0?=
 =?utf-8?B?elUxTGpPYUp1a2JLVzlLYmR1U3pxSUlkakxreTYwR3JST2pPUlJscmdwN0NO?=
 =?utf-8?B?ZkM0bUNZdkFORlFBcDVIMXM4NXdTUm5GTDBSbERlb0JwRU9GWSthb1Y0YkZl?=
 =?utf-8?B?aWRxSXBoNER4TDlWcUJTWXBnREtsaTYzbmpEZlVaYmxjNmUyOVBWMlQxSERr?=
 =?utf-8?B?eitmUFJvWXVIajg0VmFwbDQ1Yjc2ZnVDZUlyQk9DUzFlMzRicktUTld0R1Y1?=
 =?utf-8?B?czh5eEhQMEVFTjdjZ1F2RjV1VEIzVFUwc0FLYm42QUhTVCt0aGxDcjNZVTVL?=
 =?utf-8?B?dS9vU1VUcExncHFsYmo5cXJzMW1xMHNpUzg4Si9NUmcyNjVuWVQ1NTV1SEc2?=
 =?utf-8?B?UmNPNU1iQ0RIS3BYdDVOQTVQTE1hM3FCYnFmRGl1eW1yT2k4dGRHT2RIcXc2?=
 =?utf-8?B?QTVjNHBWK2ErYWRYZlJySEZISHJ0ekw0ZjBqRDJGdHR3VGRBaTFDUWdDNmI3?=
 =?utf-8?B?aDFndG8yQUJCeFVLV3FDTUxmWXAwNGJpcEQ1LzZ0MCtuMTVWZzZOV0VsUDdm?=
 =?utf-8?B?NHVvZ01XZVl6UHVkc1lTY2MvdzVKNGF6UE9FYzM4VDFHL2JOMHpmRGk0RTA3?=
 =?utf-8?B?Q2pBYXRzc3g5VkJBOHZPVGQyT2tvMWNWem44eWFDRXRsS1ExYnRzWmNMamRW?=
 =?utf-8?B?R055bGVhdnlMU0IxUGpkbFRVblhycGhoUWdrczNiRXQxemRraW1wTzVJbzhG?=
 =?utf-8?B?V1NoZGkvcUtTTjlZdUJ3eHlsOXZJeXp3UDJNd2N4QWdJU1I0V2F3UVFNV0FW?=
 =?utf-8?B?QmJ6czNic3l5aU5qZFV2ZjNZOEd2R2Y0Qks2REZPdmZKMll0N0ZSMEtxM2FQ?=
 =?utf-8?B?M3hJZWJoMmVoOStmVVVzTGU0d1hJWTNURVN5bitRUHB6SEsvaThSYWw3OXVa?=
 =?utf-8?B?VlFTVVk3SEU5VG9Tb2dSWThqT3hMQzhHVzZGUUNPZnVoNWdITitJMVNFeEkz?=
 =?utf-8?B?R0o1UUdGT1Y3OUcyMVMwSlR2WWwzV0hqZ3lhQk1IRGp6WjM1b3pnZFVBRE5G?=
 =?utf-8?B?elJtbkVZdDhVenoyYThLNzBzZHc1djBLVit2MnFmU04yUVE3NkZaQ2dUa00r?=
 =?utf-8?B?clRsdkxINWQ0T1Q2T2I3c0h6Mk1UVVBoTjdBb0VUb1Q4VUpIZEtqbXdhRSt2?=
 =?utf-8?B?aUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c2b892a1-6891-4291-f68f-08db0df77281
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Feb 2023 19:21:03.4291
 (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: zb+eZNs27Kmg2HnrcmjdLhJDkSs1Kie0sFkNpBe76TsYB2Sm3ggenwEEH4kmKLJqLLSFy2vB0XNshnioI0i0FOfOGWY1jBgzptLE7Scn4zk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR11MB8240
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Shaopeng,

On 2/12/2023 10:24 PM, Shaopeng Tan wrote:
> After creating a child process with fork() in CAT test, if an error
> occurs when parent process runs cat_val() or check_results(), the child
> process will not be killed and also resctrlfs is not unmounted. Also if
> an error occurs when child process runs cat_val() or check_results(),
> the child process is returned, but the parent process will wait pipe

"child process is returned" -> "child process returns"

> message from child.

"will wait pipe message from child" -> "will wait for the pipe message
from the child"

> Synchronize the exits between the parent and child. An error occurs
> whether in parents process or child process, the parents process
> always kills child process and runs umount_resctrlfs(), and the
> child process always waits to be killed by the parent process.

I think the above could be easier to read with a few slight changes:

"
Synchronize the exits between the parent and child. An error could
occur whether in parent process or child process. The parent process
always kills the child process and runs umount_resctrlfs(). The
child process always waits to be killed by the parent process."


> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  tools/testing/selftests/resctrl/cat_test.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 6a8306b0a109..477b62dac546 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -186,23 +186,20 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>  	remove(param.filename);
>  
>  	ret = cat_val(&param);
> -	if (ret)
> -		return ret;
> -
> -	ret = check_results(&param);
> -	if (ret)
> -		return ret;
> +	if (ret == 0)
> +		ret = check_results(&param);
>  
>  	if (bm_pid == 0) {
>  		/* Tell parent that child is ready */
>  		close(pipefd[0]);
>  		pipe_message = 1;
>  		if (write(pipefd[1], &pipe_message, sizeof(pipe_message)) <
> -		    sizeof(pipe_message)) {
> -			close(pipefd[1]);
> +		    sizeof(pipe_message))
> +			/*
> +			 * Just print the error message.
> +			 * Let while(1) run and wait for itself to be killed.
> +			 */
>  			perror("# failed signaling parent process");
> -			return errno;
> -		}
>  
>  		close(pipefd[1]);
>  		while (1)
> @@ -226,5 +223,5 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>  	if (bm_pid)
>  		umount_resctrlfs();
>  
> -	return 0;
> +	return ret;
>  }

Apart from the changelog comments:

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

Thank you very much

Reinette