Re: [PATCH v2] ktime: Simplify ktime_compare implementation

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2620 bytes --]

On Fri, May 26, 2017 at 03:00:47PM +0200, Mariusz Skamra wrote:
> ktime_sub can be used here instread of two conditional checks.
> 
> Signed-off-by: Mariusz Skamra <mariuszx.skamra@intel.com>
> Acked-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@intel.com>
> ---
>  include/linux/ktime.h | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/include/linux/ktime.h b/include/linux/ktime.h
> index 0c8bd45..04817b1 100644
> --- a/include/linux/ktime.h
> +++ b/include/linux/ktime.h
> @@ -108,11 +108,7 @@ static inline ktime_t timeval_to_ktime(struct timeval tv)
>   */
>  static inline int ktime_compare(const ktime_t cmp1, const ktime_t cmp2)
>  {
> -	if (cmp1 < cmp2)
> -		return -1;
> -	if (cmp1 > cmp2)
> -		return 1;
> -	return 0;
> +	return ktime_sub(cmp1, cmp2);
>  }
>  
>  /**
> -- 
> 2.1.4

Boot is broken on Tegra186 on next-20170623. Bisection points at this
commit and I've confirmed that reverting it also fixes the problem.

Another fix is to change the return value of ktime_compare() to ktime_t,
though I'm not sure that'd be a good alternative because it breaks with
the traditional return type of int (-1, 0, +1) for comparison functions.

It looks like the issue is with the truncation from s64 to int, as
demonstrated by this minimal test case:

--- >8 ---
#include <stdint.h>
#include <stdio.h>

#define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))

typedef int64_t s64;
typedef s64 ktime_t;

#define ktime_sub(lhs, rhs) ((lhs) - (rhs))

static inline int ktime_compare(const ktime_t cmp1, const ktime_t cmp2)
{
	return ktime_sub(cmp1, cmp2);
}

int main(int argc, char *argv[])
{
	static const ktime_t values[][2] = {
		{ 0, 1 },
		{ 1, 0 },
		{ 0, INT64_MAX },
		{ INT64_MAX, 0 },
	};
	unsigned int i;

	for (i = 0; i < ARRAY_SIZE(values); i++)
		printf("%ld - %ld = %ld (%d)\n", values[i][0], values[i][1],
		       ktime_sub(values[i][0], values[i][1]),
		       ktime_compare(values[i][0], values[i][1]));

	return 0;
}
--- >8 ---

and the following results on x86_64:

	$ gcc -O2 -g -Wall -Werror -o test test.c && ./test
	0 - 1 = -1 (-1)
	1 - 0 = 1 (1)
	0 - 9223372036854775807 = -9223372036854775807 (1)
	9223372036854775807 - 0 = 9223372036854775807 (-1)

and AArch64:

	$ aarch64-unknown-linux-gnu-gcc -O2 -g -Wall -Werror -static -o test test.c && qemu-aarch64 ./test
	0 - 1 = -1 (-1)
	1 - 0 = 1 (1)
	0 - 9223372036854775807 = -9223372036854775807 (1)
	9223372036854775807 - 0 = 9223372036854775807 (-1)

Perhaps it would be best to drop this patch?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]