Received-SPF: pass (google.com: domain of linux-kernel+bounces-172251-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <9ae11dcb-62e8-4361-9f78-971d4c6e6054@intel.com>
Date: Tue, 7 May 2024 15:30:51 -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>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240503234051.21217-1-jhubbard@nvidia.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VHV4VHBvZytGMDhvZFpzQVFZOVRBWGd0TTZxV0ZkMm5qSGV2ZXhEeDgvZkg5?=
 =?utf-8?B?VnF0MmFnVjcvYzNlcXBtRjYvUFZWQXkxSUsySkR6cC9SaWtYOWlITEVxem5W?=
 =?utf-8?B?VG1xZDRwM1NGTzl1cHl3VUcvMFJXUVRuUGxmL3Z3UVRoL2hsQ1FvRy94VXdy?=
 =?utf-8?B?ZTBrS0RobGZZQlFleVhCR2xQZGllQlFnYkgyWGJJWU5zQmN5cVlmUTZwaG5C?=
 =?utf-8?B?dzFqRjUwRkZ6UUZSODUrcFZxME5YNWdZQ0V6QVFIS01SbTBoc1ZGbmV0NVAz?=
 =?utf-8?B?QndBSnFhTHhYcjZab2pUWHo4Z3NmaUNMWVREODNXOVE5N05hL2dLdmV2Sm1D?=
 =?utf-8?B?MjFRM3A2MUZtTGFUUVN5bUtHUkpYUVlGcHcwZElQbVREY2RNQTVnOFZ4ZFRU?=
 =?utf-8?B?d0N4ZXhXUUVrQlIyL1l3OGxTajNIbWY3RWh4cGRvWjVYeDJCZ0dJM2tDRnJM?=
 =?utf-8?B?MXhrcHJHZkdSMC9VczNmTE1tTFhEMmpLWW5BVWE4N1RtRy9lYkkvbEhHWGRN?=
 =?utf-8?B?STAxTlh0Qmk5UFZVblpBNGhVdWN5SDg5OHVKOENwd1JmZU1GZTM5TmZudndP?=
 =?utf-8?B?cFFjUnBuelR2WEJUQ3JjYlQ4UkFTUmdxb0JYOWxiMEVJTTkyYnQxdXNzVnVk?=
 =?utf-8?B?dWtCMFYrNFMrN210RSsyQk1lZlZiSDZYR0wvZG5wS1YyT1dYbXp0SmF0SXNt?=
 =?utf-8?B?R3oxUDBYcXp5VW42R1FWekN6RHhoL2F3K2pSUnpmRzlCR0ltM0MxVWpwL3F4?=
 =?utf-8?B?L1BydkY3TlYydS9EQndTMmgrVkRjUTNzMmVDNEVqZEpDTEJ0REpnMm9teHBm?=
 =?utf-8?B?L1N1S2pUcWt6aGtSV1ZPaEQyUHVkZXNuRG9GWm5FUHVhbGJZU25VNHVHaVBW?=
 =?utf-8?B?dUZVWlZNUGJuTlNyc0prZGF2cE5MNkY5SFU4SkFrRlUzN1ZIN3hpcDZNbTRB?=
 =?utf-8?B?bWhSZlh4YnRNUTNWNzFpa3NBRUJCQ3FtcWFFZDBEQ2JpanJMK1E4eUZjY014?=
 =?utf-8?B?S1NibzFJQ2pBeGNFQlQrMHpNNmZ4dlJrV25SYitCdmhjdnhnc2ZsY0szL0Q3?=
 =?utf-8?B?K3Qzb09xUlVoREtyeFVOeXNpRDZIdlpnSmtDbkViNStrTTlqZnppSFRkMXA4?=
 =?utf-8?B?UVVNVVRUYWM5cXBGZFc5S1pDRWx0WHdzYW1UQWNoRzhuSFo3ZE9BVElFM2hK?=
 =?utf-8?B?UXVqTmlkSGtTQXFaMHVDWkJzL1pnRDNlME5SNk5BSHY3Q1A0Y2lLTTZXeDZw?=
 =?utf-8?B?N1R3cmNPYzZGcm9ZbitKbWMzN24rNVBXQ3RvU2x4M1J0WlprTG4rZEpFUWQx?=
 =?utf-8?B?alhEZjhVeUh5b0J4NFdORGM5RFBBd3ZaN0dNQmJKSGR3cDdwY0lqWWRnZ2xT?=
 =?utf-8?B?NkhCdWNpNjE5aHlHayswNWJQMkZLZFJ3OFdpa1BJY1pzdndxcDVQdmxkeGV1?=
 =?utf-8?B?QWlRdndDWXNlWlBpYnVMckdYTWZUQ0JhZnlPaXFQV0VHUXZoYkxVRFdtMGlD?=
 =?utf-8?B?QytNN0w1dHhaUmQrbEN6QXdKaVlJZ1hjcWlIUm5tZ3VQKzVIVmJndC9tSURj?=
 =?utf-8?B?citGMVRWU045Q3pENmZ1aHQrYXdNNy95SWxlSjBubDVpUlNTSW9oZTlzbnFK?=
 =?utf-8?B?ZlhlNWJtQVBGMmJJemR5cFhuYWxWdW1GVmZiVTRDUWZHT1I5Y3lnMXVRTmto?=
 =?utf-8?B?Q3N6VElzd3BIZnR2RkhHNzBvL0pPMzU1a1c2TlJXODdJWUhsSW80cmxCQko5?=
 =?utf-8?B?N1E0N1M1Q0daM3MzTTE1eTFFL25mSCtscjRYNWVUWm93b1RYSVdWZmlFZGVK?=
 =?utf-8?B?YjVjMzJLd29ySTNyVi9DMk0wNlZKTjVYRVNZcWFTMDZSeVlDaXpPaFhtNUtH?=
 =?utf-8?B?cGZib2JSeUdFakQzVmdHZThrVGo4aXh6Q2RMUVVTTHlpWWlkR0VyNmgwdkdC?=
 =?utf-8?B?bXhkR0lpSmI2MXgxZEV1eVB3ZytDOEg2ZDQ0YmQvNWRYNko5SGFPNGNnY3BK?=
 =?utf-8?B?UWFRQmZUZFowbmdBVFo2Vm0vWEc0MlJ3ejluTnFyQ2g4NmFucEtuaFptNlJh?=
 =?utf-8?B?MS9xbXFxSWk3TXRpNjltT0lweGo4UTI1MWZWWjdrSDFXdmJ6RTAvcmd3TklR?=
 =?utf-8?B?N3hTSDlya2N4UlA4VUkvcnAyWUUzTFVNTVA2aFlmcmR2QTM0SjhFTnNTdG1L?=
 =?utf-8?B?d0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4dcb8737-dfb3-425c-83ef-08dc6ee55b86
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 May 2024 22:30:56.5817
 (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: MZXc9wKelar3zMA10g6N7MLsF7GGjspcLjqxqYrEUiGtwwL7MXsGTEU5jwTTk3P7EnHqn0cH20Kv+QMpfufu80BXXhDzUddphs6cJ9CjPTg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB6314
X-OriginatorOrg: intel.com

Hi John,

On 5/3/2024 4:40 PM, John Hubbard wrote:
> First of all, in order to build with clang at all, one must first apply
> Valentin Obst's build fix for LLVM [1]. Furthermore, for this particular
> resctrl directory, my pending fix [2] must also be applied. Once those
> fixes are in place, then when building with clang, via:
> 
>     make LLVM=1 -C tools/testing/selftests
> 
> ...two types of warnings occur:
> 
>     warning: absolute value function 'abs' given an argument of type
>     'long' but has parameter of type 'int' which may cause truncation of
>     value
> 
>     warning: taking the absolute value of unsigned type 'unsigned long'
>     has no effect
> 
> Fix these by:
> 
> a) using labs() in place of abs(), when long integers are involved, and
> 
> b) Change to use signed integer data types, in places where subtraction
>    is used (and could end up with negative values).
> 
> [1] https://lore.kernel.org/all/20240329-selftests-libmk-llvm-rfc-v1-1-2f9ed7d1c49f@valentinobst.de/
> [2] https://lore.kernel.org/all/20240503021712.78601-1-jhubbard@nvidia.com/
> 
> Cc: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
> 
> Hi Reinette,
> 
> This v2 includes a fix for the bugs that you pointed out (thanks!) in v1.
> 
> I kept the changes to signed integers minimal: only what is required in
> order to get a clean clang build.
> 
> thanks,
> John Hubbard
> 
>  tools/testing/selftests/resctrl/cmt_test.c | 12 ++++++------
>  tools/testing/selftests/resctrl/mba_test.c |  4 ++--
>  tools/testing/selftests/resctrl/mbm_test.c |  4 ++--
>  3 files changed, 10 insertions(+), 10 deletions(-)
> 
> 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
to long (while taking labs() twice) unnecessarily.

> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
> index 7946e32e85c8..707b07687249 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -60,8 +60,8 @@ static bool show_mba_info(unsigned long *bw_imc, unsigned long *bw_resc)
>  	/* Memory bandwidth from 100% down to 10% */
>  	for (allocation = 0; allocation < ALLOCATION_MAX / ALLOCATION_STEP;
>  	     allocation++) {
> -		unsigned long avg_bw_imc, avg_bw_resc;
> -		unsigned long sum_bw_imc = 0, sum_bw_resc = 0;
> +		long avg_bw_imc, avg_bw_resc;
> +		long sum_bw_imc = 0, sum_bw_resc = 0;
>  		int avg_diff_per;
>  		float avg_diff;
>  

On second look it only seems necessary to change avg_bw_imc and avg_bw_resc. What do you think?


> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index d67ffa3ec63a..30af15020731 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;
>

Same here wrt the avg_ variables.

  
> 
> base-commit: ddb4c3f25b7b95df3d6932db0b379d768a6ebdf7
> prerequisite-patch-id: b901ece2a5b78503e2fb5480f20e304d36a0ea27
> prerequisite-patch-id: 8d96c4b8c3ed6d9ea2588ef7f594ae0f9f83c279

Reinette