Received-SPF: pass (google.com: domain of linux-kernel+bounces-168202-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <46a6019c-b029-4764-8c66-ad61f4191716@intel.com>
Date: Fri, 3 May 2024 13:46:22 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] selftests/resctrl: fix clang build warnings related to
 abs(), labs() calls
To: John Hubbard <jhubbard@nvidia.com>, =?UTF-8?Q?Ilpo_J=C3=A4rvinen?=
	<ilpo.jarvinen@linux.intel.com>
CC: Shuah Khan <shuah@kernel.org>, 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: <20240503023209.80787-1-jhubbard@nvidia.com>
 <793bd068-c3b4-6330-41a4-bea597b1d820@linux.intel.com>
 <f908ba74-86c0-409c-854d-9da5f3917b05@nvidia.com>
 <26f3effc-6ea1-4670-a301-76df3a710fa9@intel.com>
 <b88e73ea-d3f6-42d0-b9e0-f97665546178@nvidia.com>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <b88e73ea-d3f6-42d0-b9e0-f97665546178@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?NTl2eDV2aHRLM0p2M2VTYjJCMFFSWG9KMDVBaktpakhEYzBkci9nQ2lMMnFP?=
 =?utf-8?B?ZWxFYXBuYTVCS251MjkyMURVSkFjSUhkTlM0amE1R3UwelVyRVllbmFlbTBh?=
 =?utf-8?B?Q2pRV0gyLzlSNm9KbFdNTmVoZ0tFcERUdTN1SXhJWlMyNUY2aGxsRmpBS3VR?=
 =?utf-8?B?akZ4VnU5U3h2amV1dWQ2Zk50cFlkMEx5M2FjUW5hL1VTeGRoSm05WkE4V040?=
 =?utf-8?B?M1AyeWRWT0VpWG01eFhMeTlxbEx5TU5PLy9UNUZEOHBLZmVoWFlXZnhYYVFQ?=
 =?utf-8?B?Rmcza3hlUGZId2lsU3FPdEVSU2s4TWdpRlRETU0zSjRLMG9LM1lXYjVRSUtI?=
 =?utf-8?B?ZkcrcERHN1QvWkl1WTZwYStlUGNlVU4rU3pHaDU2ZHlHUG11emI2UkpyR1FL?=
 =?utf-8?B?b2FrZHg5cU94ajRGWmdlaTZNay9SUldJR3A1UFRDdXlrZ1dQNWlrc3pOOWpU?=
 =?utf-8?B?NDBBdUZwWnZlWEJKT1pvemRNUmUzV0RCRGRLUDU3MjB0dU1VMmluQ3Jrem41?=
 =?utf-8?B?NWpjZ05wMmo4TElSY05sNVcwbncyRFhzVzBzYkRFT2hqMElWcWw0ZUFqbTF2?=
 =?utf-8?B?bno3NGNzdFhwMmI5WUxtMjhnK21yRTNub2h0MjFkVyttNXoxajRTd2FYdkJz?=
 =?utf-8?B?N1o4WG9xZjlMYTI2TnMydklIUjBKQzRnclR4aFZpS0VRb2F3ZkM4WVRSc0sx?=
 =?utf-8?B?aGJva0s5Syt2VWdGNFRaY05IZHp0V1Zuam5XYWQrQkF1WXNUc3NxTE9XcGVO?=
 =?utf-8?B?U01ldFFOZnRIL3FxN1NjRG5tbEoxcFU2KzVML0RqS08rTE5ST2NnYi9qa0NG?=
 =?utf-8?B?eXhDWUkzeWlqVGhMVElsQlR0REVuT1ErYVdzaU1mcEplQkMxNTJKR3FaQjYr?=
 =?utf-8?B?cjd5OS9ndkRjbHJIdi9oNDQxdUh4UUVuU0ZKYkpOSjJGai9TOFMwZG9UL0xU?=
 =?utf-8?B?bHFLWEFhQmc4WDJrS3hpZjdYQkUrZWxFdmpSeGh2dHR4NGxrWm5jTld2bDVh?=
 =?utf-8?B?blVwT2FDQ1JKbFpUNXNtTUJJUUNPVGZSeUNaaVY2bDNnTWhITGJDWkRmS3Bx?=
 =?utf-8?B?WVJFK1VtZ0gyL1VBMko1NEN2MDdRcGdoU2plZmRFeGYxWUVyZzA5UzZJS1N2?=
 =?utf-8?B?K3MwbXNyYUh4MzgvZ0I5Y0ZXYXFkcVdCdFhWeDVPRU1kVXZuNlVBS1RhY3o0?=
 =?utf-8?B?QWlGMWwxL1Fac3J6V0NQNHlqNkpnVE5KSllhRVQ2bU1yMm1VQ01IQk5JMG05?=
 =?utf-8?B?V28xc1RpcGVsL2lVK2ZPNUd4UTcxYUswSmwySnluOGwzcjNTZEEwMjFLL1pP?=
 =?utf-8?B?b1pFanBtQllrUlFKaE8ycWZLR2pCdlBJbTNVYlpVeEpvQTVwS0NkdURoL2tt?=
 =?utf-8?B?T0puMG5XbzZiTjdSM3RIY2ZzbHVBNnVFL1pOdFIySnltVjk3LzFmRFBZVjIx?=
 =?utf-8?B?ZkV0bzE4N1ZjcXRwYlhya1BjMmFQdWI5WnNERHdoT1MxZkthdXVDelRiVnVz?=
 =?utf-8?B?TGpUZXlFZUNxc2lTNmI1UStreVgyM2tRRjVUakpBcFo0NGEvbzR2dTN0M3Rk?=
 =?utf-8?B?TUI2aTR2YW1uaXMvV0hMeWg3VlpLSEw0NmxWYnAwMnBNc1FvU2htT2JsQnhB?=
 =?utf-8?B?RWxPbUVncUI4ZmNVcmhrOXhOREhhRE55WDNZRzhsVmY1N1MvdEJwWm5MUHpX?=
 =?utf-8?B?UmF2SENlNlR1YnQ2eGJTL1k2bEtiM29ONGRJNEhhSHdJTWpubS9lenQyL3BS?=
 =?utf-8?B?WGk1MVI0TWZ0d1UzUUV3Zk1aSzd6VGtqSDhzSlBZa3p6T0tNcVRwU2hMQ0ta?=
 =?utf-8?B?M3lFbnFZYitOa2Q0RTFTVEE3SGdxdUs5ZUxwc0xGUTBsQjZVMS9YaGNjb3J5?=
 =?utf-8?B?emZtZzRzL3JaWUZyeGF5K2lwNHc0UGRUTE5IREhLNWxkSVNsQWIzNTh4MGJs?=
 =?utf-8?B?YnlnemxLU1JxS1BDR2luUFM5enBuemFYeGlZQWUzWlpjUkJUekc3ZXNKSTRS?=
 =?utf-8?B?bTZERTYyVEhjNUNXaG5WaWo0TkVLaEorWWhOclB5UG9wcmkvdnQ0VUMyVWdz?=
 =?utf-8?B?QVZOTEpMQ2NYVjBHMkU5V0Jhc0ZEdS9uTWJSb2dkQkJyZkFDRVJNZTFiWC9z?=
 =?utf-8?B?dU1wNngwazNGS2JBTjVwaW0wVWpRb1JrWDZrdEEzOUN5N2oyd1N1YzYzdVJx?=
 =?utf-8?B?aWc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 16923af6-ad80-4d14-f7f8-08dc6bb21943
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 May 2024 20:46:25.3107
 (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: 1bzP8iUNs/CJYR3f2EsSbLGN/wg2YAqaZFMhrIKuhn7EideXeRH+uaq40FdBDwukjMpSGkvBxQ4WMKP8QX34zKhO4farZWSVl0+0QVQvX/Y=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB8499
X-OriginatorOrg: intel.com

Hi John,

On 5/3/2024 12:12 PM, John Hubbard wrote:
> On 5/3/24 11:37 AM, Reinette Chatre wrote:
>> On 5/3/2024 9:52 AM, John Hubbard wrote:
>>> On 5/3/24 1:00 AM, Ilpo Järvinen wrote:
>>>> On Thu, 2 May 2024, John Hubbard wrote:
>>> ...
>>>>> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
>>>>> index d67ffa3ec63a..c873793d016d 100644
>>>>> --- a/tools/testing/selftests/resctrl/mbm_test.c
>>>>> +++ b/tools/testing/selftests/resctrl/mbm_test.c
>>>>> @@ -33,7 +33,7 @@ show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
>>>>>          avg_bw_imc = sum_bw_imc / 4;
>>>>>        avg_bw_resc = sum_bw_resc / 4;
>>>>> -    avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>>>>> +    avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
>>>>>        avg_diff_per = (int)(avg_diff * 100);
>>>>>          ret = avg_diff_per > MAX_DIFF_PERCENT;
>>>>
>>>> But how are these two cases same after your change when you ended up
>>>> removing taking the absolute value entirely?
>>>
>>> All of the arguments are unsigned integers, so all arithmetic results
>>> are interpreted as unsigned, so taking the absolute value of that is
>>> always a no-op.
>>
>> It does not seem as though clang can see when values have been casted.
>> I tried to do so explicitly with a:
>>       avg_diff = labs((long)avg_bw_resc - avg_bw_imc) / (float)avg_bw_imc;
> 
> The subtraction result will get promoted to an unsigned long, before being
> passed into labs(3).
> 
>>
>> But that still triggers:
>> warning: taking the absolute value of unsigned type 'unsigned long' has no effect [-Wabsolute-value]
> 
> As expected, yes.
> 
>>
>> Looks like we may need to be more explicit types and not rely on casting so much
>> to make the compiler happy.
>>
> 
> I assumed that this code did not expect to handle negative numbers,
> because it is using unsigned math throughout.
> 
> If you do expect it to handle cases where, for example, this happens:
> 
>    avg_bw_imc > avg_bw_resc

The existing code seems to handle this ok. A sample program with this
scenario comparing existing computation with your first proposal is
below:

#include <stdio.h>
#include <stdlib.h>

void main(void) {
	unsigned long avg_bw_resc = 20000;
	unsigned long avg_bw_imc = 40000;
	float avg_diff;

	/* Existing code */
	avg_diff = (float)labs(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
	printf("Existing code: avg_diff = %f\n", avg_diff);

	/* Original proposed fix */
	avg_diff = (float)(avg_bw_resc - avg_bw_imc) / avg_bw_imc;
	printf("Original proposed fix: avg_diff = %f\n", avg_diff);
}

output:
Existing code: avg_diff = 0.500000
Original proposed fix: avg_diff = 461168590192640.000000

> 
> ...then a proper solution is easy, and looks like this:
> 
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index c873793d016d..b87f91a41494 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -17,8 +17,8 @@
>  static int
>  show_bw_info(unsigned long *bw_imc, unsigned long *bw_resc, size_t span)
>  {
> -       unsigned long avg_bw_imc = 0, avg_bw_resc = 0;
> -       unsigned long sum_bw_imc = 0, sum_bw_resc = 0;
> +       long avg_bw_imc = 0, avg_bw_resc = 0;
> +       long sum_bw_imc = 0, sum_bw_resc = 0;
>         int runs, ret, avg_diff_per;
>         float avg_diff = 0;
> 
> Should I resend the patch with that approach?

ok. That indeed makes the computations easier to understand. I assume
you intend to fix the snippet in mba_test.c also?

Reinette