Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1674471 > unrolled thread

Re: [PATCH v2] ktime: Simplify ktime_compare implementation

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] ktime: Simplify ktime_compare implementation Thierry Reding <thierry.reding@gmail.com> - 2017-06-26 09:00 +0200
    Re: [PATCH v2] ktime: Simplify ktime_compare implementation Thomas Gleixner <tglx@linutronix.de> - 2017-06-26 10:50 +0200

#1674471 — Re: [PATCH v2] ktime: Simplify ktime_compare implementation

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

#1674532

On Mon, 26 Jun 2017, Thierry Reding wrote:
> On Fri, May 26, 2017 at 03:00:47PM +0200, Mariusz Skamra wrote:
> > ktime_sub can be used here instread of two conditional checks.
> 
> 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:

You are right. And I should have seen it when merging that patch. Looking
deeper, the patch is pointless, because the compiler will optimize the
checks away and use the proper CMP instruction to figure it out.

It's reverted now.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web