All of lore.kernel.org
 help / color / mirror / Atom feed
* + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree
@ 2015-09-17 21:36 akpm
  2015-09-23 13:44 ` Alexey Dobriyan
  0 siblings, 1 reply; 7+ messages in thread
From: akpm @ 2015-09-17 21:36 UTC (permalink / raw)
  To: mina86, a.p.zijlstra, john.stultz, masami.hiramatsu.pt, mingo,
	peterz, rostedt, torvalds, mm-commits


The patch titled
     Subject: kernel.h: make abs() work with 64-bit types
has been added to the -mm tree.  Its filename is
     kernelh-make-abs-work-with-64-bit-types.patch

This patch should soon appear at
    http://ozlabs.org/~akpm/mmots/broken-out/kernelh-make-abs-work-with-64-bit-types.patch
and later at
    http://ozlabs.org/~akpm/mmotm/broken-out/kernelh-make-abs-work-with-64-bit-types.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

The -mm tree is included into linux-next and is updated
there every 3-4 working days

------------------------------------------------------
From: Michal Nazarewicz <mina86@mina86.com>
Subject: kernel.h: make abs() work with 64-bit types

For 64-bit arguments, the abs macro casts it to an int which leads to lost
precision and may cause incorrect results.  To deal with 64-bit types
abs64 macro has been introduced but still there are places where abs macro
is used incorrectly.

To deal with the problem, expand abs macro such that it operates on s64
type when dealing with 64-bit types while still returning long when
dealing with smaller types.

This fixes one known bug (per John):

The internal clocksteering done for fine-grained error correction uses a
: logarithmic approximation, so any time adjtimex() adjusts the clock
: steering, timekeeping_freqadjust() quickly approximates the correct clock
: frequency over a series of ticks.
: 
: Unfortunately, the logic in timekeeping_freqadjust(), introduced in commit
: dc491596f639438 (Rework frequency adjustments to work better w/ nohz),
: used the abs() function with a s64 error value to calculate the size of
: the approximated adjustment to be made.
: 
: Per include/linux/kernel.h: "abs() should not be used for 64-bit types
: (s64, u64, long long) - use abs64()".
: 
: Thus on 32-bit platforms, this resulted in the clocksteering to take a
: quite dampended random walk trying to converge on the proper frequency,
: which caused the adjustments to be made much slower then intended (most
: easily observed when large adjustments are made).

Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
Reported-by: John Stultz <john.stultz@linaro.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/kernel.h |   43 ++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff -puN include/linux/kernel.h~kernelh-make-abs-work-with-64-bit-types include/linux/kernel.h
--- a/include/linux/kernel.h~kernelh-make-abs-work-with-64-bit-types
+++ a/include/linux/kernel.h
@@ -200,28 +200,31 @@ extern int _cond_resched(void);
 
 #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
 
-/*
- * abs() handles unsigned and signed longs, ints, shorts and chars.  For all
- * input types abs() returns a signed long.
- * abs() should not be used for 64-bit types (s64, u64, long long) - use abs64()
- * for those.
+/**
+ * abs - return absolute value of an argument
+ * @x: the value.  If it is unsigned type, it is converted to signed type first
+ *   (s64, long or int depending on its size).
+ *
+ * Return: an absolute value of x.  If x is 64-bit, macro's return type is s64,
+ *   otherwise it is signed long.
  */
-#define abs(x) ({						\
-		long ret;					\
-		if (sizeof(x) == sizeof(long)) {		\
-			long __x = (x);				\
-			ret = (__x < 0) ? -__x : __x;		\
-		} else {					\
-			int __x = (x);				\
-			ret = (__x < 0) ? -__x : __x;		\
-		}						\
-		ret;						\
-	})
+#define abs(x) __builtin_choose_expr(sizeof(x) == sizeof(s64), ({	\
+		s64 __x = (x);						\
+		(__x < 0) ? -__x : __x;					\
+	}), ({								\
+		long ret;						\
+		if (sizeof(x) == sizeof(long)) {			\
+			long __x = (x);					\
+			ret = (__x < 0) ? -__x : __x;			\
+		} else {						\
+			int __x = (x);					\
+			ret = (__x < 0) ? -__x : __x;			\
+		}							\
+		ret;							\
+	}))
 
-#define abs64(x) ({				\
-		s64 __x = (x);			\
-		(__x < 0) ? -__x : __x;		\
-	})
+/* Deprecated, use abs instead. */
+#define abs64(x) abs((s64)(x))
 
 /**
  * reciprocal_scale - "scale" a value into range [0, ep_ro)
_

Patches currently in -mm which might be from mina86@mina86.com are

kernelh-make-abs-work-with-64-bit-types.patch


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree
  2015-09-17 21:36 + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree akpm
@ 2015-09-23 13:44 ` Alexey Dobriyan
  2015-09-23 20:04   ` Andrew Morton
                     ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Alexey Dobriyan @ 2015-09-23 13:44 UTC (permalink / raw)
  To: Linux Kernel, Andrew Morton
  Cc: Michal Nazarewicz, Peter Zijlstra, john.stultz, Masami Hiramatsu,
	Ingo Molnar, Peter Zijlstra, Steven Rostedt, Linus Torvalds

On Fri, Sep 18, 2015 at 12:36 AM,  <akpm@linux-foundation.org> wrote:


> -#define abs(x) ({                                              \
> -               long ret;                                       \
> -               if (sizeof(x) == sizeof(long)) {                \
> -                       long __x = (x);                         \
> -                       ret = (__x < 0) ? -__x : __x;           \
> -               } else {                                        \
> -                       int __x = (x);                          \
> -                       ret = (__x < 0) ? -__x : __x;           \
> -               }                                               \
> -               ret;                                            \
> -       })
> +#define abs(x) __builtin_choose_expr(sizeof(x) == sizeof(s64), ({      \
> +               s64 __x = (x);                                          \
> +               (__x < 0) ? -__x : __x;                                 \
> +       }), ({                                                          \
> +               long ret;                                               \
> +               if (sizeof(x) == sizeof(long)) {                        \
> +                       long __x = (x);                                 \
> +                       ret = (__x < 0) ? -__x : __x;                   \
> +               } else {                                                \
> +                       int __x = (x);                                  \
> +                       ret = (__x < 0) ? -__x : __x;                   \
> +               }                                                       \
> +               ret;                                                    \
> +       }))

1. "char" should be banned, because it's signedness is not well defined.
2. there is unnecessary expansion to long.

I've sent kabs() before which didn't go in because it didn't work for
INT_MAX et al
(don't worry, this abs() doens't as well) but it is nicer that this
version in other aspects
(hopefully).

[PATCH v2] Add kabs()
http://marc.info/?l=linux-kernel&m=133518745522740&w=4

As for INT_MIN, it pretty impossible to make compiler to not generate a branch
despite generating correct result without a branch.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree
  2015-09-23 13:44 ` Alexey Dobriyan
@ 2015-09-23 20:04   ` Andrew Morton
  2015-09-24 13:32   ` Michal Nazarewicz
  2015-09-24 18:03   ` Linus Torvalds
  2 siblings, 0 replies; 7+ messages in thread
From: Andrew Morton @ 2015-09-23 20:04 UTC (permalink / raw)
  To: Alexey Dobriyan
  Cc: Linux Kernel, Michal Nazarewicz, Peter Zijlstra, john.stultz,
	Masami Hiramatsu, Ingo Molnar, Peter Zijlstra, Steven Rostedt,
	Linus Torvalds

On Wed, 23 Sep 2015 16:44:29 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Fri, Sep 18, 2015 at 12:36 AM,  <akpm@linux-foundation.org> wrote:
> 
> 
> > -#define abs(x) ({                                              \
> > -               long ret;                                       \
> > -               if (sizeof(x) == sizeof(long)) {                \
> > -                       long __x = (x);                         \
> > -                       ret = (__x < 0) ? -__x : __x;           \
> > -               } else {                                        \
> > -                       int __x = (x);                          \
> > -                       ret = (__x < 0) ? -__x : __x;           \
> > -               }                                               \
> > -               ret;                                            \
> > -       })
> > +#define abs(x) __builtin_choose_expr(sizeof(x) == sizeof(s64), ({      \
> > +               s64 __x = (x);                                          \
> > +               (__x < 0) ? -__x : __x;                                 \
> > +       }), ({                                                          \
> > +               long ret;                                               \
> > +               if (sizeof(x) == sizeof(long)) {                        \
> > +                       long __x = (x);                                 \
> > +                       ret = (__x < 0) ? -__x : __x;                   \
> > +               } else {                                                \
> > +                       int __x = (x);                                  \
> > +                       ret = (__x < 0) ? -__x : __x;                   \
> > +               }                                                       \
> > +               ret;                                                    \
> > +       }))
> 
> 1. "char" should be banned, because it's signedness is not well defined.
> 2. there is unnecessary expansion to long.
> 
> I've sent kabs() before which didn't go in because it didn't work for
> INT_MAX et al
> (don't worry, this abs() doens't as well) but it is nicer that this
> version in other aspects
> (hopefully).
> 
> [PATCH v2] Add kabs()
> http://marc.info/?l=linux-kernel&m=133518745522740&w=4

Looks nice.  Care to send along a patch which updates the abs() in
linux-next?

> As for INT_MIN, it pretty impossible to make compiler to not generate a branch
> despite generating correct result without a branch.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree
  2015-09-23 13:44 ` Alexey Dobriyan
  2015-09-23 20:04   ` Andrew Morton
@ 2015-09-24 13:32   ` Michal Nazarewicz
  2015-09-24 14:26     ` Alexey Dobriyan
  2015-09-24 18:03   ` Linus Torvalds
  2 siblings, 1 reply; 7+ messages in thread
From: Michal Nazarewicz @ 2015-09-24 13:32 UTC (permalink / raw)
  To: Alexey Dobriyan, Linux Kernel, Andrew Morton
  Cc: Peter Zijlstra, john.stultz, Masami Hiramatsu, Ingo Molnar,
	Peter Zijlstra, Steven Rostedt, Linus Torvalds

On Wed, Sep 23 2015, Alexey Dobriyan wrote:
> I've sent kabs() before which didn't go in because it didn't work for
> INT_MAX et al
> (don't worry, this abs() doens't as well) but it is nicer that this
> version in other aspects
> (hopefully).
>
> [PATCH v2] Add kabs()
> http://marc.info/?l=linux-kernel&m=133518745522740&w=4

Perhaps:

+	(void)(_x)))))));						\

instead of:

+	_x))))));							\

at the end.  Since kabs makes no sense for unsigned types it’s best to
fail with compile-time error than to let user think that the call is
actually doing something.

Also, you don’t need ({ … }) around the ‘_x < 0 ? -_x : _x’ expression,
do you?

-- 
Best regards,                                            _     _
.o. | Liege of Serenely Enlightened Majesty of         o' \,=./ `o
..o | Computer Science,  ミハウ “mina86” ナザレヴイツ  (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>-----ooO--(_)--Ooo--

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree
  2015-09-24 13:32   ` Michal Nazarewicz
@ 2015-09-24 14:26     ` Alexey Dobriyan
  0 siblings, 0 replies; 7+ messages in thread
From: Alexey Dobriyan @ 2015-09-24 14:26 UTC (permalink / raw)
  To: Michal Nazarewicz
  Cc: Linux Kernel, Andrew Morton, Peter Zijlstra, John Stultz,
	Masami Hiramatsu, Ingo Molnar, Peter Zijlstra, Steven Rostedt,
	Linus Torvalds

On Thu, Sep 24, 2015 at 4:32 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
> On Wed, Sep 23 2015, Alexey Dobriyan wrote:
>> I've sent kabs() before which didn't go in because it didn't work for
>> INT_MAX et al
>> (don't worry, this abs() doens't as well) but it is nicer that this
>> version in other aspects
>> (hopefully).
>>
>> [PATCH v2] Add kabs()
>> http://marc.info/?l=linux-kernel&m=133518745522740&w=4
>
> Perhaps:
>
> +       (void)(_x)))))));                                               \
>
> instead of:
>
> +       _x))))));                                                       \
>
> at the end.  Since kabs makes no sense for unsigned types it’s best to
> fail with compile-time error than to let user think that the call is
> actually doing something.

I thought so, but the amount of uses like

  unsigned int = abs(unsigned int - unsigned int)

is non trivial. With a few exceptions it is about 170 on
x86_64 allmodconfig. Most of them are correct because abs()
casts to signed first. So if you want to expose such uses,
then you are forced to write

  unsigned int = kabs((int)(unsigned int - unsigned int));

Don't know if it's a good thing.

> Also, you don’t need ({ … }) around the ‘_x < 0 ? -_x : _x’ expression,
> do you?

Seems so, thank you for noticing.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree
  2015-09-23 13:44 ` Alexey Dobriyan
  2015-09-23 20:04   ` Andrew Morton
  2015-09-24 13:32   ` Michal Nazarewicz
@ 2015-09-24 18:03   ` Linus Torvalds
  2015-09-25 16:36     ` Michal Nazarewicz
  2 siblings, 1 reply; 7+ messages in thread
From: Linus Torvalds @ 2015-09-24 18:03 UTC (permalink / raw)
  To: Alexey Dobriyan
  Cc: Linux Kernel, Andrew Morton, Michal Nazarewicz, Peter Zijlstra,
	John Stultz, Masami Hiramatsu, Ingo Molnar, Peter Zijlstra,
	Steven Rostedt

On Wed, Sep 23, 2015 at 6:44 AM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> I've sent kabs() before which didn't go in because it didn't work for
> INT_MAX et al
>
> [PATCH v2] Add kabs()
> http://marc.info/?l=linux-kernel&m=133518745522740&w=4

Yeah, no, that's bad.

Testing against char/short is pointless. They should get upgraded to "int".

And guys, stop the idiotic "shouldn't work on unsigned values" crap.
It damn well should work on unsigned values, and the semantics we want
is that we treat it as a signed value.

Why? That's how "abs()" works. Really. Stop fighting it. If you were
to use a real "abs()", it takes an "int" argument, and if you pass it
an unsigned value, it gets converted to "int".

End of story. Stop with the broken "but but but unsigned" crap already.

So the end result is that

 (a) we should look at the *size* of the argument type, not the
signedness, because the signedness is immaterial

 (b) we should *not* cast the thing to unsigned, because we
traditionally haven't.

 (c) we should definitely not use smaller than "int" as a minimum
size, no crazy games with char/short. And considering our legacy, I
think we should probably skip "int" and stay with "long" as the
minimum size.

So I *much* prefer Michal's "abs()" definition that doesn't radically
change the meaning (it keeps the old behavior *except* if you pass in
a bigger size than "long", in which case it will auto-widen to "s64").
That's the maximally compatible model given the "we handle bigger
types automatically" extension.

Because "maximally compatible" is a strong argument. The signedness
stuff and the "let's try to use the exact same type" is just bogus.

One thing that *is* interesting is "what if 'long' and 's64' are the
same size?" In particular, it means that right now Michal's patch
*always* returns "long" on a 64-bit architecture, but will return
"long" or "s64" on a 32-bit one. The reason that is somewhat
interesting is that while the sizes and values are the same, and the
resulting C type expansions are "equivalent" types, i people *print*
things, you have to use different modifiers for the two cases. So you
might get warnings on 32-bit architectures and not get them on 64-bit,
or vice versa.

However, I don't see a good solution for that. And assuming we don't
use "abs()" in an expression to printk(), I guess it doesn't much
matter either.

              Linus

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree
  2015-09-24 18:03   ` Linus Torvalds
@ 2015-09-25 16:36     ` Michal Nazarewicz
  0 siblings, 0 replies; 7+ messages in thread
From: Michal Nazarewicz @ 2015-09-25 16:36 UTC (permalink / raw)
  To: Linus Torvalds, Alexey Dobriyan
  Cc: Linux Kernel, Andrew Morton, Peter Zijlstra, John Stultz,
	Masami Hiramatsu, Ingo Molnar, Peter Zijlstra, Steven Rostedt

On Thu, Sep 24 2015, Linus Torvalds wrote:
> One thing that *is* interesting is "what if 'long' and 's64' are the
> same size?" In particular, it means that right now Michal's patch
> *always* returns "long" on a 64-bit architecture, but will return
> "long" or "s64" on a 32-bit one.

That’s not accurate.  s64 is defined as long long so:
- on 64-bit architectures, the macro will return s64 (i.e. long long)
  for long arguments (because sizeof(long) == 8 == sizeof(s64) and the
  first path is taken), but
- on 32-bit architectures, it will return long for long arguments (since
  sizeof(long) == 4 != 8 == sizeof(long long) and the second path is
  taken).
But yes, the point remains, depending on architecture, the macro returns
different type for long arguments.

> The reason that is somewhat interesting is that while the sizes and
> values are the same, and the resulting C type expansions are
> "equivalent" types, i people *print* things, you have to use different
> modifiers for the two cases. So you might get warnings on 32-bit
> architectures and not get them on 64-bit, or vice versa.
>
> However, I don't see a good solution for that. And assuming we don't
> use "abs()" in an expression to printk(), I guess it doesn't much
> matter either.

This should do the trick:

#define abs(x) __builtin_choose_expr(				\
	__builtin_types_compatible_p(typeof(x), s64) ||		\
	__builtin_types_compatible_p(typeof(x), u64),		\
	({ s64 __x = (x); __x < 0 ? -__x : __x; }),		\
	__builtin_choose_expr(sizeof(x) <= sizeof(long), ({	\
		long ret;					\
		if (sizeof(x) == sizeof(long)) {		\
			long __x = (x);				\
			ret = (__x < 0) ? -__x : __x;		\
		} else {					\
			int __x = (x);				\
			ret = (__x < 0) ? -__x : __x;		\
		}						\
		ret;						\
	}), (void)(x)))

It’ll return s64 for s64 and u64 (i.e. long long and unsigned long long)
types, long for anything whose sizeof <= sizeof(long) and will bail out
with compile time error if used for any other type (if return value is
used).

I dunno whether added complexity is worth solving the problem though.

-- 
Best regards,                                            _     _
.o. | Liege of Serenely Enlightened Majesty of         o' \,=./ `o
..o | Computer Science,  ミハウ “mina86” ナザレヴイツ  (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>-----ooO--(_)--Ooo--

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-09-25 16:36 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-17 21:36 + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree akpm
2015-09-23 13:44 ` Alexey Dobriyan
2015-09-23 20:04   ` Andrew Morton
2015-09-24 13:32   ` Michal Nazarewicz
2015-09-24 14:26     ` Alexey Dobriyan
2015-09-24 18:03   ` Linus Torvalds
2015-09-25 16:36     ` Michal Nazarewicz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.