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: <2ab9ca20-c757-7dd8-b770-2b84d171cbfb@intel.com>
Date:   Mon, 7 Nov 2022 14:31:34 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.4.1
Subject: Re: [PATCH v3 4/5] selftests/resctrl: Cleanup properly when an error
 occurs in CAT test
Content-Language: en-US
To:     Shuah Khan <skhan@linuxfoundation.org>,
        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: <20221101094341.3383073-1-tan.shaopeng@jp.fujitsu.com>
 <20221101094341.3383073-5-tan.shaopeng@jp.fujitsu.com>
 <39586b57-aad2-d9ca-df12-67f1dfe60258@linuxfoundation.org>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <39586b57-aad2-d9ca-df12-67f1dfe60258@linuxfoundation.org>
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?MXlOWGYyQTRkYWdjR24yOHdRaU5OM2JjOVhmWW0vUjhqY0ZVcXB6aGpqYWlH?=
 =?utf-8?B?QllOT2prM2ZPRDdMYlA4VnJFLytxYTV0YkpwSkZjRzJJS3RhZjRZYlM4WHpj?=
 =?utf-8?B?R0xjMUNIdGsvT0QwaVJBU0xrajRpVHREanpGNlJRZURKR0JIZ2Ixam51TFRh?=
 =?utf-8?B?dXBHUUJobnRqRDBoUEtEWFpBbzdKeEtYcXJxQ2NHR3gyVklNUk9GNGNRS1FJ?=
 =?utf-8?B?WC91c2Q3cC91b282TzZPY04ybHdoRjV1RkJabXpTZE1VZmthNEQyRXBVeitt?=
 =?utf-8?B?QTZVSUNwd3hlcjNKc3dwQ3BySlpocFJJRVBSRktNQVlhaU9sNWdOc3lkTktr?=
 =?utf-8?B?bFdka2lleEFwbmxBOUtPS2pkSjQ0ZVBJUCtUSHMzSlVLaGhXbW9WNDk0QXdI?=
 =?utf-8?B?WW5jMnU0TERJYmY3aHpVVTZkRENRV0FIcm5JdjgzcHZnRW5kcm5MUVZCdlM2?=
 =?utf-8?B?ME9uamg5dm5xMlp2czJDdjdDM1pTc0pTUXZjMDZ2SmdWT0lUbW5qa2ZPNUJn?=
 =?utf-8?B?Nk4xYWxrS1dZN2t1dXkwQkNxWDZUQ201VFhUeDBmR3QzY2FrbGp2Z05YTVBF?=
 =?utf-8?B?TGc0aGhLUUFEYkVpdjVzeGJCdGE4bFlvWGgwL084dm1Yc3BpMDBLRGVYWERB?=
 =?utf-8?B?eHExVVVjOVFEdUdVWkUyeWFFbUtyKzdNWmwydFAwNC80TmlIQzJTbHVKNHpQ?=
 =?utf-8?B?WWFzWWtJRzZwL1N1UmNlczVYZ2Q3a1FITDZHSWd6amlYaitCQXMrdDljRTBV?=
 =?utf-8?B?N2lPMUMxN2RpVmFISnhVc2ZyOTRYa3Nzd2lLNEEvSDA5MStRbnBPcllMVXpO?=
 =?utf-8?B?ZUdMUzZSdmErN0VNM29aeHhIbzZBelF4b1NTTFhPelhHa0NXRm5DMW1MalJN?=
 =?utf-8?B?aGoyaTZoOWVnOCs1aXVwQ09PMWdnZDB5dzhsa2ZlQTREVmIvMlBpbWZqV2s5?=
 =?utf-8?B?ZmhLNHQ0dnBDZEkxSDFkblRXUjM3WFd1L0tOYWkxYlJTTFdzTDY0bWxidFBV?=
 =?utf-8?B?MUhlRkdZVGtzdVROUlhSYlVwVmNid09WbUo0MENTZVQ5a0tPRU5wMU8zT0c1?=
 =?utf-8?B?SitrZGxuYjlTVmJCTHpUb3dJN0k4Q1dxRys4T0ZHcHRpVFJvVHFia0MwVGl2?=
 =?utf-8?B?S0FlYXNuR1BKeUxocEFGTUk0K2M1a05Xem1XU3pDcWpRNVp1OG5lNFJrNm1w?=
 =?utf-8?B?eEtCdlZxOHBOYmkrUGkzd1NlcFBTTkYzNXFLbU9SMkdLbG9hRVRXbVFkNkI2?=
 =?utf-8?B?ekowRmh1ZVFwQitpaWpUK05ZdkEyK2UyT2ZiSk1yU0xiL2pMR1kvWWFVVm1O?=
 =?utf-8?B?T2tMeDVMd3h0NkFLSmFVbU4wdzRWUFJwNXVZc296RHlzTVJTMktQa01NRXFk?=
 =?utf-8?B?dXdRT3gySmlmWFRtRnhLbXhPRVFMK2dOcnlDb09rb2lsL0thYnZJaW9vQkhm?=
 =?utf-8?B?c2tqY1drSDc5aXhPTmVZYm44Rmt2VFQyaWVYa3JDbDdVKzBidnlYbVFianFw?=
 =?utf-8?B?OUlmdWNnUzVKc2F2Q2F4dXVSRFZxMzJQWXZFS3lPemdLUnVUSmpOOVpkZC9u?=
 =?utf-8?B?bXdaWmhKNWNCVEJ4eGhlUVE3ck0wRlZhT0xhZ2VqNEYwR2JMUlpXYWI0QUhU?=
 =?utf-8?B?TGlVS3N3VU1iaWc5OEVQY3h0czJMaHExRHZtWjlqZ3Bpdkh5dDJrTmNsOXlS?=
 =?utf-8?B?MnIzY05xMmh2RGthdEIxa2luclFxazZubmFmQUZIa1dZTGVoTnFaQ0xhdW5Y?=
 =?utf-8?B?V3FIL2lXNlpyYmdJMHdaazJpM0swVzB1bHc5SFJOODVxQVdpVk9vaXZvSTNr?=
 =?utf-8?B?bERmYzhlbFRodzBJOFlIT1duVHVxd0p6Zy8wNml3c0wzcUlpZkIyQ3NhWTFY?=
 =?utf-8?B?Tlo3OFNWUXZFY0M1dS9EOW1sTitBRGYveXIwQnBRTlMxQUdTaXlLTk1Ic3RK?=
 =?utf-8?B?M0hhQTFva1hMc0hOeTRvMVFwYndsVlNZRGcrcDJ2TkRkcGwzUmxqdVJRWkVy?=
 =?utf-8?B?ejNPd2JvWXQ2MmpOOFBHYStRdGUvdEhrYk5VSFF3VCtIQXUxMFk2K2JMVXZI?=
 =?utf-8?B?UWNTNlkyUGk1OVhyeTJ0UkIzdm5yTzY4TWRoNW5ETStqbmNTc1c5SHBraklO?=
 =?utf-8?B?Z1dJR01YNzVtNmNrRVczeFdXc0s3VHNhWXZXSlVueXFXRGwzeDA5SzNoMUZZ?=
 =?utf-8?B?NVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: bb35c83f-434d-4f29-35bf-08dac10fd53b
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Nov 2022 22:31:37.4162
 (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: Ry8AKFPkOSD5KaSQBX9N/+JTkXtNkRFFN3td6U9Spka4Kd+P32REJIYJPB94cv45XWQEMCFnoOBTWjHLUvlMuT03zZ0Kt1Vr5MpggBjWXxE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB5941
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 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_MED,SPF_HELO_PASS,SPF_NONE 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 Shaopeng and Shuah,

On 11/2/2022 2:41 AM, Shuah Khan wrote:
> On 11/1/22 03:43, Shaopeng Tan wrote:
>> After creating a child process with fork() in CAT test, if there is
>> an error occurs or such as a SIGINT signal is received, the parent
>> process will be terminated immediately, but the child process will not
>> be killed and also umount_resctrlfs() will not be called.
>>
>> Add a signal handler like other tests to kill child process, umount
>> resctrlfs, cleanup result files, etc. when an error occurs.
>>
>> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
>> ---
>>   tools/testing/selftests/resctrl/cat_test.c | 28 +++++++++++++++-------
>>   1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>> index 6a8306b0a109..5f81817f4366 100644
>> --- a/tools/testing/selftests/resctrl/cat_test.c
>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>> @@ -98,12 +98,21 @@ void cat_test_cleanup(void)
>>       remove(RESULT_FILE_NAME2);
>>   }
>>   +static void ctrl_handler(int signo)
>> +{
>> +    kill(bm_pid, SIGKILL);
>> +    umount_resctrlfs();
>> +    tests_cleanup();
>> +    ksft_print_msg("Ending\n\n");
> 
> Is there a reason to print this message? Remove it unless it serves
> a purpose.

This function appears to be a duplicate of existing
resctrl_val.c:ctrlc_handler(). Could the duplication be avoided
instead of refining the copy?

> 
>> +
>> +    exit(EXIT_SUCCESS);
>> +}
>> +
>>   int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>>   {
>>       unsigned long l_mask, l_mask_1;
>>       int ret, pipefd[2], sibling_cpu_no;
>>       char pipe_message;
>> -    pid_t bm_pid;
> 
> Odd. bm_pid is used below - why remove it here?

There is a global bm_pid in resctrl_val.c that is made available
via extern in resctrl.h. This is what causes this code to still
compile but I would also like to learn why moving to that is
desired as a change here. I expect such a big change to get a
mention in the commit message.

> 
>>         cache_size = 0;
>>   @@ -181,17 +190,19 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
>>           strcpy(param.filename, RESULT_FILE_NAME1);
>>           param.num_of_runs = 0;
>>           param.cpu_no = sibling_cpu_no;
>> +    } else {
>> +        /* set up ctrl-c handler */
>> +        if (signal(SIGINT, ctrl_handler) == SIG_ERR ||
>> +            signal(SIGHUP, ctrl_handler) == SIG_ERR ||
>> +            signal(SIGTERM, ctrl_handler) == SIG_ERR)
>> +            printf("Failed to catch SIGNAL!\n");
> 
> Is perror() more appropriate here?

Should we be using signal() at all? "man signal" reads:
"WARNING: the behavior of signal() varies across UNIX versions,
and has also varied historically across different versions of Linux.
Avoid its use: use sigaction(2) instead."

"Failed to catch SIGNAL" also seems unclear to me. This is
where a signal handler is set up, the signal for which the handler
is installed has not arrived.


> 
>>       }
>>         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);
> 
> Why not use a goto in error case to do umount_resctrlfs() instead of changing
> the conditionals?

My understanding is the code that follows is needed to
synchronize the exits between the parent and child. It is the parent
that will run umount_resctrlfs() and it should only do so
after the child is done. A goto by the parent may thus cause
umount_resctrlfs() to be run while the child still relies on it while
a goto by the child may cause the parent not to receive the message
that the child is complete.

Reinette