Message-ID: <25ca1de3-5673-6332-a1d5-59920c191739@intel.com>
Date:   Mon, 23 Jan 2023 09:36:13 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.6.1
Subject: Re: [PATCH v5 4/5] selftests/resctrl: Cleanup properly when an error
 occurs in CAT test
To:     "Shaopeng Tan (Fujitsu)" <tan.shaopeng@fujitsu.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Shuah Khan <shuah@kernel.org>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>
References: <20230111075802.3556803-1-tan.shaopeng@jp.fujitsu.com>
 <20230111075802.3556803-5-tan.shaopeng@jp.fujitsu.com>
 <3201c461-2f46-f354-6019-94ce8978fb46@intel.com>
 <TYAPR01MB6330B2248A693BD3170CC76B8BC89@TYAPR01MB6330.jpnprd01.prod.outlook.com>
Content-Language: en-US
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <TYAPR01MB6330B2248A693BD3170CC76B8BC89@TYAPR01MB6330.jpnprd01.prod.outlook.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?SVpoRHNNMjduMnBDSGZTYk84U09IZHZ1MVJIZUN1anRhVmt0Y29iekY0Y0dW?=
 =?utf-8?B?UWFZRXQ1d1NhWFZhN2dtOWxGTWlVOXFYQzAwYjRhRzNVNzBkRUpXanpjVHVp?=
 =?utf-8?B?UElRS0syRWxaVnN4VXFYb081UzFSYmo5RTFLUThWMThGMUpyVVFzUEh6U243?=
 =?utf-8?B?eFMzRE82dkZGVFNXSDNreStDckFvSTBZV1lvY3prMUFxdGFGRllNVHZOb2Zm?=
 =?utf-8?B?Y01xb0FTaHdaMHkzVERsUHNJcHJFK1h1M2cvazJxR1VWWUVkUGxNbElkZGlX?=
 =?utf-8?B?NVNnVjdscnpJSGdKTEZoRzV1YmJQbTBQTFBNazllR3Rra25Qc3R3cDRKRkUy?=
 =?utf-8?B?dFhRazVZZ1JQQVltNk9KVGdLN09HMXA3RThWbURaZWlwQWZpQ1F2Smx5UEFw?=
 =?utf-8?B?c042TzhuZjNsUXhLOFR4VWJITi9DTmd4TFdBemIxQ1NRK3R5M0pEK0hVSEtK?=
 =?utf-8?B?bXdWOVJFMmVZR2JHbDVvZWQ2Z2s4cGhmK3RkSzZMTlhXUFg5Z3l6Uko1bWY3?=
 =?utf-8?B?TzZDYjEwTExnaHZpK2Y3WFhXQXdTQ2NnMGpzTVFFdWdQQ3FVMlJYOXVCSHpq?=
 =?utf-8?B?QUJlOFFlR0NOR0dUdGtlUURHay9GaHYxbDdxS3BLV2ZzRE4yTWJLUWgzcy9u?=
 =?utf-8?B?cWYrdjNmeXBGRWJ1VHZCbHdNK0Rmak40NlRQUkxIczlNZkh3c3JyVW01WFE2?=
 =?utf-8?B?YUQwVEVJeGtQSXRqdlRvN3pINzJGWitkdWN0WjJuUk5nbS9iQUhBdTF3b2dC?=
 =?utf-8?B?dGJnQUdCdGpnWkFnNlRVcjdteFFZaU4wVVpjaFIxZXBYay9POFBhTitiVjdi?=
 =?utf-8?B?cVRiaGx0eE9sZHkvdEwxTm9iUE4vS3JZUlRTY2hsZWhYSVVMU05ISGFVS2Fp?=
 =?utf-8?B?OWV6WHY3SnlUYlJUQUFXNTJZZXI2ZDdXek1SRGJrU2g1U0RWZUFvSDdWaHli?=
 =?utf-8?B?QVZIWjRpZzdPMkpVdlAzSm1Ta1VBTDJrZ0t2SUZ6dHNMeXZjRnY4NjlCbmRj?=
 =?utf-8?B?Mko5R3IvTklPaFE2VWtobEJVUklnbzJCSTdnZHd6SWVzVHZ6c2lDMndrenZ5?=
 =?utf-8?B?WEV6eE1NWmdEc2tjYXQ3NEdGWlBxMHV6WVpPVmhmYW5UKzJFOUE0SGl4OVhB?=
 =?utf-8?B?cVQvbDZFNXdKVEZkMG9Dc2k5blNxUFBiSmdJK0RDZWEzUm9lMERJRXB4OHBk?=
 =?utf-8?B?ZGxlWUp6b01tK056VnhJNVB1bUIyUThzUElINGZKcTdxSG1HVFZlVVJIQXRp?=
 =?utf-8?B?WlpXdnA2aTZtYkZ6S3JycG1xR0V0Z0xmUUtXdk90WlZEbGUrVFdiaUQrRzFW?=
 =?utf-8?B?OHZodzBFRDRtRTM4UEFEZS96dC9RYk81ZVZqZEF2Q3JaeHgvTjBMYWYxdHBG?=
 =?utf-8?B?NkFweG1GOHNvc2ZqaTVMTmd6Q1pISnlmdkJXOVVQaHk5VmxEck9MdU1hY3RW?=
 =?utf-8?B?VlJkbkViTnYrR2lReEQzZ1hCWGMxM1J0L2hLU0xkM3o2TXlBL0lHRHdJVGRz?=
 =?utf-8?B?VDF5OGwrK090TWxwOVpyNzBVNGhaKzdKQndZSHd3RERHczYzSEN6VHp6Mmpm?=
 =?utf-8?B?SVIwN1R5MnFsbGRrbTArYkd4dDBvL2FSelZzejgwNDMrdm1OM245V2h2bTlS?=
 =?utf-8?B?YkNhVWxhTENMVmVjQmVnSjNwTzE2RC9GN3lsWnFtZzlETzRsZUlEUGFQQlV1?=
 =?utf-8?B?K0VBWEk0SFUxQkhRYkpRYVpkMW5uMHVvRGN0bk4yUUFuSGtlTm9sc3BXTUtl?=
 =?utf-8?B?L1l2OUl5U0lkeHRjbDM0a3k4SWtENVpJMFhhdGt3U3NMUjlGa2pJNTdnZ3ZJ?=
 =?utf-8?B?aEo0eThxWTJ1NGUrMmVxQ0lCQUZ5TlpGTlNCcFN5VHdNNnJDU2oxM1d1TXRZ?=
 =?utf-8?B?WTVRWXZOSVFCczJCUDdLbHp4SWFLeDZuSTU0bHFpVjM0QXNqU0piVUY1a3Ir?=
 =?utf-8?B?VDRTL0JFTWNkNDlVYmpwWERVNXNHUnVkanVLb1ZxcjdkUm05TndXMHQvQVFN?=
 =?utf-8?B?aG5iL0xtQ3pKSzZkMDZqU29zK20rQmYwRUZYRkxobEE5MS96OTRNOHB1Vmow?=
 =?utf-8?B?QnBOZC9iWnN4THp4MDZ0eWxTS285aG1FMWZLejM1R3JMRjJvTGpQdlVPQkNJ?=
 =?utf-8?B?R0JwVzVINVY3NUJRb2xseDVuUmh0ZDZjZ1pQMlRJeTdkQW9qbDd2V0pjVWhZ?=
 =?utf-8?B?dnc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d00d0198-fb81-48bb-2ec0-08dafd6854ab
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jan 2023 17:36:16.7464
 (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: CFoW5NwfBq/+JurDtRnMJGWL9ZlweNKvlnEGvlnNAUjy9aR4pQ2u3qWj2Mb7ThsEhWv7ICTQzXM98MKBRCv+31f6VG2TEg2U1U2kYfRvlfc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB4773
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Shaopeng,

On 1/22/2023 8:22 PM, Shaopeng Tan (Fujitsu) wrote:
>> On 1/10/2023 11:58 PM, Shaopeng Tan wrote:

...

>>> diff --git a/tools/testing/selftests/resctrl/cat_test.c
>>> b/tools/testing/selftests/resctrl/cat_test.c
>>> index 6a8306b0a109..87302b882929 100644
>>> --- a/tools/testing/selftests/resctrl/cat_test.c
>>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>>> @@ -103,7 +103,6 @@ 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;
>>>
>>>  	cache_size = 0;
>>>
>>> @@ -181,28 +180,29 @@ 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 {
>>> +		ret = signal_handler_register();
>>> +		if (ret)
>>> +			goto out;
>>
>> The "goto" will unregister the signal handler. Is that necessary if the registration
>> failed?
>>
>> Also, if signal_handler_register() fails then the child will keep running and run
>> its test ... would child not then run forever?
> 
> A signal handler is needed here, but it is rarely used.
> Also, the registration rarely fails.
> Therefore, if registration failed, 
> just print a warning/info message as follow.
> how about this idea?
> 
> ksft_print_msg("Failed to register signal handler, signals SIGINT/SIGTERM/SIGHUP will not be handled as expected");
> 

I do not think this is necessary considering that signal_handler_register()
already prints an error on failure. 

Adding an error message does not address the two issues I raised.

Reinette