Message-ID: <e10e4e8f-97a0-3fba-bd51-a2b14f40499e@intel.com>
Date:   Thu, 6 Jan 2022 15:46:56 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.4.0
Subject: Re: [PATCH v2 1/5] selftests/resctrl: Kill the child process created
 by fork() when the SIGTERM signal comes
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: <20211213100154.180599-1-tan.shaopeng@jp.fujitsu.com>
 <20211213100154.180599-2-tan.shaopeng@jp.fujitsu.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20211213100154.180599-2-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?YXNMMmZzSHg0VzFWVVJnVWpDeEdjakZXclBhYlE2WWpaUHl1bnpLeHNYdHpE?=
 =?utf-8?B?M1BGRy9LQXQrL0FtcndaQldwWC9oLzYrZnZaZXhndXJVQzFyV3QxWjVFaGRy?=
 =?utf-8?B?ODFST015T1l4MmtJR3ZkZEVrclFwQ2xxNnJNZldIbDlTdzRwRjROSm1PQnZt?=
 =?utf-8?B?UFh6LzVLVHJ1R3pZT2JxWnJ4WU5SK0hJWWk4amZhSWs3Z2hkYzlWWTVoaW4z?=
 =?utf-8?B?UWhDenZQY09WeFBkSmlJMVl6UklKeWpHZTJpclBCRnNPSFg3clpVVEdaRmd5?=
 =?utf-8?B?bEVOZ1NQWGxlaWprSkcxT1gvS24vNHZGRTBia0JmSnhvYnJML1hjNUt4d0o0?=
 =?utf-8?B?ODM2WFliSVVORFMwdE5UZ0tPNXlYMUh5aTc3anBrV1VHbE04NjIxUFd0S0N3?=
 =?utf-8?B?eHpDUFB0M3JOVm5MSGNIb2FnRmt5RFlqZFgyekhPZWcxVmQ1Wm5QcnRXU3NM?=
 =?utf-8?B?ZDk1UW16QnMyUUhkMEZmdFJFYU5sVHpDcTlIWUI4STRlZ3J5MVhHRUVTMjBL?=
 =?utf-8?B?V2RkOVdKMUcvQS9wMlRITWRyQTZJaXk1SUd3a2tITGN1bUZseDZkb0txOVUz?=
 =?utf-8?B?NGlFQng0RnhMMUJLUkJHaFJSbGcxQnB0MThsT1dyMkEvWnFkTzFoQkV5cG9X?=
 =?utf-8?B?QU83NzYxNUhVVHpibXJEcEpYUXl6OFlXV2FGK0pZMUFXV0wwRWxUU2p1SGJU?=
 =?utf-8?B?Um9oa3ppTVBLZmt4OEI1MkNOTm1IQnB6Z2UybThyWCtFMlhTWkxYdUo4VnlE?=
 =?utf-8?B?S296ZUNIdkIwZGtiakswU0dzMHBjajdoUnZiNGh0TTJhc3Y1RlFSRklXZWtu?=
 =?utf-8?B?cEl6SE1UMEl2Y3IxNittQVdVNW8yUnBTalczQVB5MlpNWGxTeFlQZjNIRVQx?=
 =?utf-8?B?WXJVSDdJdmxzZW1sUWk4WStmSkF1OEVJUEljNUEycDZrQ3FEbGgxdVBtNXUw?=
 =?utf-8?B?Zk15Qk9SNDd3UzdwV0V1OElFOTZwZ2xjcXpNNHhuOWdBTHhMSExNeWQ5K0JY?=
 =?utf-8?B?RW9KSDV4UWhmSlB4YjZMZFphL29JZDl0Z2dqRGNCK2hMQ1o0LzR1ODV0Mzd2?=
 =?utf-8?B?b082a0h2ZTZPdDBPc2UyMG5wMmxnNURzT2Z2ODNNWlJtdFFveUVMQStTZk5D?=
 =?utf-8?B?VWpxTnRXMDJta2MyakNKQXI1NzRHRldUaHhGdHpTa0ZDSTJwcEpKd2tyVUVl?=
 =?utf-8?B?RVNMcUdoRjBTcUFaTCtaRlF5aldBcVNPN2ZsblF0MDhKcVFmODdST2dWTnJX?=
 =?utf-8?B?b0RGbW1zTjljU04rWTd3QVh1YytBZGJTOFZ2S3FYMjF0YTF0bm5WZEZObmMv?=
 =?utf-8?B?ZkFxak04RTZFVFNLNlZmU0VNWXpldW9COGI2NTBNNkNXcXFMRVFyaHc4MjFR?=
 =?utf-8?B?ZXVQMVR3MnJIVFNGRlhuVE9Tbm9jeC93Rm82R0pLNXNVSkd4b254S2FvK005?=
 =?utf-8?B?RmcyOFdiR2tjaVRZSUZOUDcxU3NTVnFHMGhHdkxuVmJUQTZyNG50UHlNSCtz?=
 =?utf-8?B?NnVIb0MwbExOM3dDazBobzRNZWl2OHZsVm9UZmMxR3l1cDFONUVlNWdXakI3?=
 =?utf-8?B?aVgycWdSTGo3Y3d3KzZGaFl5SWpLcHJNeWY3ZWN5NEdNR0Y3NGRJK1pqalZJ?=
 =?utf-8?B?dmIzZERPQVVWWC9BQngydWxKbkdXS09ZOG9xN0dzVEpyKzJZTDNzMyszOE1i?=
 =?utf-8?B?QlJkYXZlMzV2V3lyd3BjdUxtUnRhckpXdkl1NzRkS0drREJOdkd2Nm93amJu?=
 =?utf-8?B?bjRCUzUwU2lQV1FwMVJjT09LeGFxdEx1VW9RRjZZTGRzanlJUHJTTGw1dmlx?=
 =?utf-8?B?aTg3R3J6S3pUdkh3N3NDaWtPa3RMeVdtcjZTM2FXS2hHM2tKUjcxcEdURGtp?=
 =?utf-8?B?cXp5eFppL0tNazdnTnl6SFZSTkM4akkrWWpHdnhDSzJqMWhsRS9yNUpTRzdI?=
 =?utf-8?B?UnNyR0tMK0hRTlJuZW9CWXM1SjBYU0ZVTHVDblFiekNXcHk3eHdrQjZuQ2M2?=
 =?utf-8?B?bnV5UzJKMFlvdWJ2WXVDa0x1dllsSVMrY2ZPN3lsNHR0NldRZDNOK1F5amJs?=
 =?utf-8?B?YU82SnArbkMvVjdQRE9na2kvRkUxaGlWQVFqWmVkaDhWUS95WVkxaTU4b3Bo?=
 =?utf-8?B?NG1iTlVySGlmcjdQaWY1amVsT2puN25MdEZwTGQ5aFJ2WndVUkNWZi9idytS?=
 =?utf-8?B?VUF0QmFtUnN3b3gxck1VdEY1OTd0WDQyV3pva1lOWWNjWE5pSGc3dUJIajlo?=
 =?utf-8?Q?QvL2RJau49JXFpzoPliY8Zq5JSnYrXdH05NA0I5Prk=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 71443388-27f4-43d5-e771-08d9d16ed4b1
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jan 2022 23:46:59.9046
 (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: evb6g1qmz8Yb8NRqXfMXtmg6fK2nqigz42VNMGgcLiMV073XfU+VTeObmCrF5WuPAdGj4f8rUSebZphTbsWMPF9YFhQcWhO+WuVDZw0riAU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR11MB1298
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Shaopeng Tan, 

On 12/13/2021 2:01 AM, Shaopeng Tan wrote:
> In kselftest framework there is a limited time for each sub test,
> when the time limit comes SIGTEM signal will be sent to sub test by

SIGTEM -> SIGTERM ?

> "timeout --foregroup <seconds>" command.

foregroup?

This is a bit confusing though since it mentions that the "timeout" utility
is called after the test times out. Perhaps you can just describe that, if
present, the test is run using the timeout utility and it will
send SIGTERM to the test upon timeout.

> In resctrl_tests, fork() is used to create a child process.
> This commit ensures child process to be killed before parent process

Especially since I know you are planning more changes in the x86 area
where this is enforced more strictly, please do get into the habit of
describing your changes in imperative mood. Please search for
"This patch" in Documentation/process/submitting-patches.rst (your
usage of "This commit" is equivalent to "This patch") for more details.

Please address this in all your patches where "This commit" is frequently
used. 

> exiting if SIGTERM signal comes.
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_val.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index 95224345c78e..b32b96356ec7 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -678,6 +678,7 @@ int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param)
>  	sigemptyset(&sigact.sa_mask);
>  	sigact.sa_flags = SA_SIGINFO;
>  	if (sigaction(SIGINT, &sigact, NULL) ||
> +	    sigaction(SIGTERM, &sigact, NULL) ||
>  	    sigaction(SIGHUP, &sigact, NULL)) {
>  		perror("# sigaction");
>  		ret = errno;

The change looks good. Since this snippet is preceded with a comment
that describes its usage you could also update it with the expanded
use of the kselftest framework.

Reinette