Received-SPF: pass (google.com: domain of linux-kernel+bounces-172588-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <d67f4f57-4e9a-4715-b6dd-7b83a240b7dd@intel.com>
Date: Tue, 7 May 2024 18:21:36 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] selftests/resctrl: fix clang build warnings related to
 abs(), labs() calls
To: John Hubbard <jhubbard@nvidia.com>, Shuah Khan <shuah@kernel.org>
CC: Nathan Chancellor <nathan@kernel.org>, Nick Desaulniers
	<ndesaulniers@google.com>, Bill Wendling <morbo@google.com>, Justin Stitt
	<justinstitt@google.com>, Fenghua Yu <fenghua.yu@intel.com>, Valentin Obst
	<kernel@valentinobst.de>, <linux-kselftest@vger.kernel.org>, LKML
	<linux-kernel@vger.kernel.org>, <llvm@lists.linux.dev>
References: <20240503234051.21217-1-jhubbard@nvidia.com>
 <9ae11dcb-62e8-4361-9f78-971d4c6e6054@intel.com>
 <997d7fe0-46c8-4b38-824d-083ab29f54ce@nvidia.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <997d7fe0-46c8-4b38-824d-083ab29f54ce@nvidia.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?LzhWVFJKSkNqVE12WkZzenpVZURwN3g1emdLZ2ZqL2l4Y2wzcjJpaXhmRGdP?=
 =?utf-8?B?THJ2OHVnZTVqMGlPVjlua090T2VDRXBxVHlWWnZIK08rYmp2QitxaDFqMW9z?=
 =?utf-8?B?VEFrOEprVnFvZEx5SzhNMUxKKzExak5hclA1RmNnc05xSlVOb282MU11R2Fa?=
 =?utf-8?B?SnJkNHZVVGtsTGRVeCtsL2hicnhhM08xeUVLUG1DSWpOUElaMkZzVjRQMHhE?=
 =?utf-8?B?WmZTQm4xWWt5NVhUL0REYzZLalQ5TXRiTmR5SXBRaWlVZTh1ZUZoTlZaZHdQ?=
 =?utf-8?B?NmF3VDRLdVp2NWNJb1g0dEFGMkxoWlJPU3hOa3hxNDVEbGVvNFJkcGJadk5Z?=
 =?utf-8?B?MFNESXN5ZldNQ0N1Sm01VkM1YjNIT3JiSnhLS2krNEkzV2pqc3AydkJqc25K?=
 =?utf-8?B?V0M4SGJpZjRZNURKM2FjdTNiMlI3RXgxQjBXNGQrMkFBNW0ydlQ3eVo0K0Jr?=
 =?utf-8?B?Wkt0d2lIVTJ1WUhEZGhNMWdpQTlhSWtaMFpGblpyaXR3V2lsczJQODkwa05E?=
 =?utf-8?B?THJyV1VCYjFETEkwaldHYUNZc1I1anhUaTVPckFoeHFrbDBwaWZWeEF0d0xw?=
 =?utf-8?B?NVc0NmpwRkVkeGoxSmdFam9venY0NTNPWXowSnRrZFNXTmI4NzVwT0lCclE5?=
 =?utf-8?B?dEgyWXkycUNvcnpWM0IwclRGY1lZbmhUSDVFbHdMekpPNElzaXVCY21NTlA4?=
 =?utf-8?B?RnhTRVFaRVF4TWpwRmFJNFZGTWwyUU93bW85MHU2NmdQa1ZqWFJydkRFZERk?=
 =?utf-8?B?ellnekY0dmV4WUIxeDdWdFhrYkppVVl1VUllUGtNdjBlamllL3kxMmpwTW5K?=
 =?utf-8?B?UmxmcjQrdW9RSTZPRTBxcEhOayt1MXNvWnZYOFoyemp5T21XYmtOR01yOFp2?=
 =?utf-8?B?L3JlQUhTUzBnd0UxVmZLSythK3NQbyt6anRCOWtnakVGTS80Qjc5YldLRlp4?=
 =?utf-8?B?cVdJbnc2U3IzbTNuNnlRcEVKcFZ0eWZKUzZ6SWo2Q3ErcHh0NmxiVHdKd1RM?=
 =?utf-8?B?VENrSUw5NE04ZE9Jb1liWVpyZUtGVFNOWkNQR3IrY2xRUmRheWhCTkFzZUtC?=
 =?utf-8?B?RVM0NlNtd0RxQXg3Mmk4SWhxZ2ZYbnJsYVdab2IrTC9odjMzVjZpOVdYa21Y?=
 =?utf-8?B?MnVKcXZFTGRyZ2ZkV21EcmxxOENvdmY2RTN1TlgwenpkNlNxY1JOUlpNWHZl?=
 =?utf-8?B?M25TMkQ1V2p5R09UeUQremNTUlVBQWs4S2RCQlE1RUI0TWVkQU10UG9vZm1U?=
 =?utf-8?B?SFVTVElLdDhucjNVNGpyZEtsMk5tbkJoT1R0NlpHdE1rNVNpbmNvRFlwczd0?=
 =?utf-8?B?RFk4SlR1bUJvNWJVMmU1TVpnQ2FhNWtFQ1ZEcW1MMHgzdGdHeFgzdGtEQW42?=
 =?utf-8?B?ZkhYcGhjTE44dmhVY2l1ZmtWVC9MM2lOWGVkb0tIWTUvUkh4K3pLWXRqUVpr?=
 =?utf-8?B?RFY3SW1pa3MzR3JvMXJJUnVUNWEwbWgxVGVGWSsxS3QvUjhVa0RjWmlNb00z?=
 =?utf-8?B?dlkvSjg2d2dsSVljQXJPOWhXbUxsT1FOVXVXWW1HNUNtcHhBR0taeVJxRC9Z?=
 =?utf-8?B?d0ZYa1hOcGUvc2E1R1NLdytpNnVYeVorNTFVZVBWcU9sdEVFT0czUnlMTlE4?=
 =?utf-8?B?dE5uY3FmS0ExckhjTjJPNDgwZWUrN0NmUjBHb1pPdVRHdDV2czlUQnc1OWZN?=
 =?utf-8?B?ajY2WnJLckZEYzBJWHQwWGNJMzJHQk5zSFhNZzFzN0lKek9YeWh4NmJ2OXVo?=
 =?utf-8?B?VmJ5bFA4WVI5VXRNNmhmSnJnNW9JQWpSK1RpaGJVcktGRDYxV1F5RnJneFNW?=
 =?utf-8?B?T1NRUDdETWx3b0dDMjdGVFpZU1lPWU03YWk3SlovYXlNNHhzMWJMYm1GRjRx?=
 =?utf-8?B?SUhxakRPWllmSGNmMHBlckl1Q2hIM0dhK3ZvV2pxT242c0wwcjVGaEFJanlM?=
 =?utf-8?B?ckRib1hjeXA4N1NFRGdvQ2NGYnRzS0xwZVJjMWc4aVcydEplK242UXJRQTRF?=
 =?utf-8?B?TW9rczlrNTg2eUFCREtHazI1NS9BUlV3bkhXbjlCcTA2d1JxSWdvMlM3a1Bs?=
 =?utf-8?B?QmFxSlpHdSttd2hMYkhiekFYTHpTa3UvZkxkYkxjUThMUlQwdFIzVE00dzVs?=
 =?utf-8?B?MjBMRzF3UWNpUUdHdzBZaVlLRkRkTzVrd3RIYVZ4b3dEUmZLcG1SWmltN2ty?=
 =?utf-8?B?RkE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9953247e-9495-466d-29ac-08dc6efd3571
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 May 2024 01:21:38.3488
 (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: PsMQ+C1iYeGu+de2t2GIumaVOtHgs3G/agZ6cAYO+SUoevE0nVs4lLucVDU67cKlCs6OVbyUoK48OCwD4i/00SP7V9jhXJYc4Cy/mwGZB9M=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB7573
X-OriginatorOrg: intel.com

Hi John,

On 5/7/2024 6:16 PM, John Hubbard wrote:
> On 5/7/24 3:30 PM, Reinette Chatre wrote:
> ...
>>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>>> index a81f91222a89..af33abd1cca7 100644
>>> --- a/tools/testing/selftests/resctrl/cmt_test.c
>>> +++ b/tools/testing/selftests/resctrl/cmt_test.c
>>> @@ -29,22 +29,22 @@ static int cmt_setup(const struct resctrl_test *test,
>>>       return 0;
>>>   }
>>>   -static int show_results_info(unsigned long sum_llc_val, int no_of_bits,
>>> -                 unsigned long cache_span, unsigned long max_diff,
>>> -                 unsigned long max_diff_percent, unsigned long num_of_runs,
>>> +static int show_results_info(long sum_llc_val, int no_of_bits,
>>> +                 long cache_span, long max_diff,
>>> +                 long max_diff_percent, long num_of_runs,
>>>                    bool platform)
>>>   {
>>> -    unsigned long avg_llc_val = 0;
>>> +    long avg_llc_val = 0;
>>>       float diff_percent;
>>>       long avg_diff = 0;
>>>       int ret;
>>>         avg_llc_val = sum_llc_val / num_of_runs;
>>> -    avg_diff = (long)abs(cache_span - avg_llc_val);
>>> +    avg_diff = labs(cache_span - avg_llc_val);
>>>       diff_percent = ((float)cache_span - avg_llc_val) / cache_span * 100;
>>>         ret = platform && abs((int)diff_percent) > max_diff_percent &&
>>> -          abs(avg_diff) > max_diff;
>>> +          labs(avg_diff) > max_diff;
>>>         ksft_print_msg("%s Check cache miss rate within %lu%%\n",
>>>                  ret ? "Fail:" : "Pass:", max_diff_percent);
>>
>> The changes in this hunk are unexpected. The changes to this area made by previous
>> version was ok, no? It really seems like this just does a brute force of everything
> 
> Well, not entirely. That first version was when I still believed clang's
> claim that abs()/labs() was a no-op. I've since been corrected! :)
> 
>> to long (while taking labs() twice) unnecessarily.
> 
> Which part exactly is unnecessary? Are you looking at the function args?
> Or something else? I've stared at it too much and am not spotting the
> issue yet.
> 

The following (what was in v1) looks good to me. What am I missing?

diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
index a81f91222a89..05a241519ae8 100644
--- a/tools/testing/selftests/resctrl/cmt_test.c
+++ b/tools/testing/selftests/resctrl/cmt_test.c
@@ -40,11 +40,11 @@ static int show_results_info(unsigned long sum_llc_val, int no_of_bits,
 	int ret;
 
 	avg_llc_val = sum_llc_val / num_of_runs;
-	avg_diff = (long)abs(cache_span - avg_llc_val);
+	avg_diff = (long)(cache_span - avg_llc_val);
 	diff_percent = ((float)cache_span - avg_llc_val) / cache_span * 100;
 
 	ret = platform && abs((int)diff_percent) > max_diff_percent &&
-	      abs(avg_diff) > max_diff;
+	      labs(avg_diff) > max_diff;
 
 	ksft_print_msg("%s Check cache miss rate within %lu%%\n",
 		       ret ? "Fail:" : "Pass:", max_diff_percent);

Reinette