[lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding

[Darrick J. Wong]

Create a helper macro to divide two numbers and round the result to the
nearest whole number. This is a helper macro for hwmon drivers that
want to convert incoming sysfs values per standard hwmon practice, though
the macro itself can be used by anyone.

Signed-off-by: Darrick J. Wong <djwong at us.ibm.com>
---

include/linux/kernel.h | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fba141d..fb02266 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
+#define DIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(divisor) __divisor = divisor; \
+ (((x) + ((__divisor) / 2)) / (__divisor)); \
+} \
+)

#define _RET_IP_ (unsigned long)__builtin_return_address(0)
#define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; })

[Darrick J. Wong]

Modify some hwmon drivers to use DIV_ROUND_CLOSEST instead of bloating
source with (naughty) macros.

Signed-off-by: Darrick J. Wong <djwong at us.ibm.com>
---

drivers/hwmon/adt7462.c | 14 ++++++--------
drivers/hwmon/adt7470.c | 8 +++-----
drivers/hwmon/adt7473.c | 10 ++++------
3 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/adt7462.c b/drivers/hwmon/adt7462.c
index 66107b4..1852f27 100644
--- a/drivers/hwmon/adt7462.c
+++ b/drivers/hwmon/adt7462.c
@@ -204,8 +204,6 @@ I2C_CLIENT_INSMOD_1(adt7462);
#define MASK_AND_SHIFT(value, prefix) \
(((value) & prefix##_MASK) >> prefix##_SHIFT)

-#define ROUND_DIV(x, divisor) (((x) + ((divisor) / 2)) / (divisor))
-
struct adt7462_data {
struct device *hwmon_dev;
struct attribute_group attrs;
@@ -840,7 +838,7 @@ static ssize_t set_temp_min(struct device *dev,
if (strict_strtol(buf, 10, &temp) || !temp_enabled(data, attr->index))
return -EINVAL;

- temp = ROUND_DIV(temp, 1000) + 64;
+ temp = DIV_ROUND_CLOSEST(temp, 1000) + 64;
temp = SENSORS_LIMIT(temp, 0, 255);

mutex_lock(&data->lock);
@@ -878,7 +876,7 @@ static ssize_t set_temp_max(struct device *dev,
if (strict_strtol(buf, 10, &temp) || !temp_enabled(data, attr->index))
return -EINVAL;

- temp = ROUND_DIV(temp, 1000) + 64;
+ temp = DIV_ROUND_CLOSEST(temp, 1000) + 64;
temp = SENSORS_LIMIT(temp, 0, 255);

mutex_lock(&data->lock);
@@ -943,7 +941,7 @@ static ssize_t set_volt_max(struct device *dev,
return -EINVAL;

temp *= 1000; /* convert mV to uV */
- temp = ROUND_DIV(temp, x);
+ temp = DIV_ROUND_CLOSEST(temp, x);
temp = SENSORS_LIMIT(temp, 0, 255);

mutex_lock(&data->lock);
@@ -985,7 +983,7 @@ static ssize_t set_volt_min(struct device *dev,
return -EINVAL;

temp *= 1000; /* convert mV to uV */
- temp = ROUND_DIV(temp, x);
+ temp = DIV_ROUND_CLOSEST(temp, x);
temp = SENSORS_LIMIT(temp, 0, 255);

mutex_lock(&data->lock);
@@ -1250,7 +1248,7 @@ static ssize_t set_pwm_hyst(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;

- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = SENSORS_LIMIT(temp, 0, 15);

/* package things up */
@@ -1337,7 +1335,7 @@ static ssize_t set_pwm_tmin(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;

- temp = ROUND_DIV(temp, 1000) + 64;
+ temp = DIV_ROUND_CLOSEST(temp, 1000) + 64;
temp = SENSORS_LIMIT(temp, 0, 255);

mutex_lock(&data->lock);
diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index 1311a59..da6c930 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -137,8 +137,6 @@ I2C_CLIENT_INSMOD_1(adt7470);
#define FAN_PERIOD_INVALID 65535
#define FAN_DATA_VALID(x) ((x) && (x) != FAN_PERIOD_INVALID)

-#define ROUND_DIV(x, divisor) (((x) + ((divisor) / 2)) / (divisor))
-
struct adt7470_data {
struct device *hwmon_dev;
struct attribute_group attrs;
@@ -360,7 +358,7 @@ static ssize_t set_temp_min(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;

- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = SENSORS_LIMIT(temp, 0, 255);

mutex_lock(&data->lock);
@@ -394,7 +392,7 @@ static ssize_t set_temp_max(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;

- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = SENSORS_LIMIT(temp, 0, 255);

mutex_lock(&data->lock);
@@ -671,7 +669,7 @@ static ssize_t set_pwm_tmin(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;

- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = SENSORS_LIMIT(temp, 0, 255);

mutex_lock(&data->lock);
diff --git a/drivers/hwmon/adt7473.c b/drivers/hwmon/adt7473.c
index 18aa308..0a6ce23 100644
--- a/drivers/hwmon/adt7473.c
+++ b/drivers/hwmon/adt7473.c
@@ -129,8 +129,6 @@ I2C_CLIENT_INSMOD_1(adt7473);
#define FAN_PERIOD_INVALID 65535
#define FAN_DATA_VALID(x) ((x) && (x) != FAN_PERIOD_INVALID)

-#define ROUND_DIV(x, divisor) (((x) + ((divisor) / 2)) / (divisor))
-
struct adt7473_data {
struct device *hwmon_dev;
struct attribute_group attrs;
@@ -459,7 +457,7 @@ static ssize_t set_temp_min(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;

- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = encode_temp(data->temp_twos_complement, temp);

mutex_lock(&data->lock);
@@ -495,7 +493,7 @@ static ssize_t set_temp_max(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;

- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = encode_temp(data->temp_twos_complement, temp);

mutex_lock(&data->lock);
@@ -720,7 +718,7 @@ static ssize_t set_temp_tmax(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;

- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = encode_temp(data->temp_twos_complement, temp);

mutex_lock(&data->lock);
@@ -756,7 +754,7 @@ static ssize_t set_temp_tmin(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;

- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = encode_temp(data->temp_twos_complement, temp);

mutex_lock(&data->lock);

[Jean Delvare]

Hi Darrick,

Post by Darrick J. Wong
Create a helper macro to divide two numbers and round the result to the
nearest whole number. This is a helper macro for hwmon drivers that
want to convert incoming sysfs values per standard hwmon practice, though
the macro itself can be used by anyone.
Signed-off-by: Darrick J. Wong <djwong at us.ibm.com>
---
include/linux/kernel.h | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fba141d..fb02266 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
+#define DIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(divisor) __divisor = divisor; \
+ (((x) + ((__divisor) / 2)) / (__divisor)); \
+} \
+)
#define _RET_IP_ (unsigned long)__builtin_return_address(0)
#define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; })

I don't get why you implement this as a macro rather than an inline
function? A function would look much better.

[Andrew Morton]

Post by Jean Delvare
Hi Darrick,

Post by Darrick J. Wong
Create a helper macro to divide two numbers and round the result to the
nearest whole number. This is a helper macro for hwmon drivers that
want to convert incoming sysfs values per standard hwmon practice, though
the macro itself can be used by anyone.
Signed-off-by: Darrick J. Wong <djwong at us.ibm.com>
---
include/linux/kernel.h | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fba141d..fb02266 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
+#define DIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(divisor) __divisor = divisor; \
+ (((x) + ((__divisor) / 2)) / (__divisor)); \
+} \
+)
#define _RET_IP_ (unsigned long)__builtin_return_address(0)
#define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; })

I don't get why you implement this as a macro rather than an inline
function? A function would look much better.

The idea is that DIV_ROUND_CLOSEST() can be used with arguments of any
size (char, short, ... long long) and will do all the suitable
promotion and will return a type of the appropriate width and
signedness.

It's not particularly pretty and can hide traps and pitfalls, but the
other way is tricky as well - it'd need a family of functions and
there's a risk that programmers will choose the wrong one.

[Jean Delvare]

Post by Andrew Morton

Post by Jean Delvare
Hi Darrick,

Post by Darrick J. Wong
Create a helper macro to divide two numbers and round the result to the
nearest whole number. This is a helper macro for hwmon drivers that
want to convert incoming sysfs values per standard hwmon practice, though
the macro itself can be used by anyone.
Signed-off-by: Darrick J. Wong <djwong at us.ibm.com>
---
include/linux/kernel.h | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fba141d..fb02266 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
+#define DIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(divisor) __divisor = divisor; \
+ (((x) + ((__divisor) / 2)) / (__divisor)); \
+} \
+)
#define _RET_IP_ (unsigned long)__builtin_return_address(0)
#define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; })

I don't get why you implement this as a macro rather than an inline
function? A function would look much better.

The idea is that DIV_ROUND_CLOSEST() can be used with arguments of any
size (char, short, ... long long) and will do all the suitable
promotion and will return a type of the appropriate width and
signedness.
It's not particularly pretty and can hide traps and pitfalls, but the
other way is tricky as well - it'd need a family of functions and
there's a risk that programmers will choose the wrong one.

OK, I get it now, thanks for the clarification.

[Joe Perches]

Post by Andrew Morton

Post by Jean Delvare

Post by Darrick J. Wong
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fba141d..fb02266 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
+#define DIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(divisor) __divisor = divisor; \
+ (((x) + ((__divisor) / 2)) / (__divisor)); \
+} \
+)

I don't get why you implement this as a macro rather than an inline
function? A function would look much better.

The idea is that DIV_ROUND_CLOSEST() can be used with arguments of any
size (char, short, ... long long) and will do all the suitable
promotion and will return a type of the appropriate width and
signedness.

Perhaps the macro should be placed directly after
DIV_ROUND_UP and should use the same argument naming.

Perhaps HALF_UP is more descriptive and fairly common.
http://java.sun.com/j2se/1.5.0/docs/api/java/math/RoundingMode.html

[Trent Piepho]

Post by Darrick J. Wong
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fba141d..fb02266 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
+#define DIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(divisor) __divisor = divisor; \
+ (((x) + ((__divisor) / 2)) / (__divisor)); \
+} \
+)

Maybe you can do away with the statement-expression extension? I've seen
cases where it cases gcc to generate worse code. It seems like it
shouldn't, but it does. I know DIV_ROUND_CLOSEST (maybe DIV_ROUND_NEAR?)
uses divisor twice, but all the also divide macros do that too, so why does
this one need to be different?

Note that if divisor is a signed variable, divisor/2 generates worse code
than divisor>>1.

[Andrew Morton]

On Tue, 11 Nov 2008 15:05:02 -0800 (PST)

Post by Trent Piepho

Post by Darrick J. Wong
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fba141d..fb02266 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
+#define DIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(divisor) __divisor = divisor; \
+ (((x) + ((__divisor) / 2)) / (__divisor)); \
+} \
+)

Maybe you can do away with the statement-expression extension? I've seen
cases where it cases gcc to generate worse code. It seems like it
shouldn't, but it does. I know DIV_ROUND_CLOSEST (maybe DIV_ROUND_NEAR?)
uses divisor twice, but all the also divide macros do that too, so why does
this one need to be different?

The others need fixing too.

Post by Trent Piepho
Note that if divisor is a signed variable, divisor/2 generates worse code
than divisor>>1.

yup. I wonder why the compiler doesn't do that for itself - is there a
case where it will generate a different result?

[Jochen Voß]

Hi,

Post by Andrew Morton
yup. I wonder why the compiler doesn't do that for itself - is there a
case where it will generate a different result?

The test program

#include <stdio.h>
int
main()
{
signed int x = -1;
printf("%d %d\n", x/2, x>>1);
return 0;
}

says

0 -1

so it seems to make a difference.

All the best,
Jochen

[Trent Piepho]

Post by Andrew Morton
On Tue, 11 Nov 2008 15:05:02 -0800 (PST)

Post by Trent Piepho

Post by Darrick J. Wong
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
+#define DIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(divisor) __divisor = divisor; \
+ (((x) + ((__divisor) / 2)) / (__divisor)); \
+} \
+)

Maybe you can do away with the statement-expression extension? I've seen
cases where it cases gcc to generate worse code. It seems like it
shouldn't, but it does. I know DIV_ROUND_CLOSEST (maybe DIV_ROUND_NEAR?)
uses divisor twice, but all the also divide macros do that too, so why does
this one need to be different?

The others need fixing too.

Is it worth generating worse code for these simple macros?

Post by Andrew Morton

Post by Trent Piepho
Note that if divisor is a signed variable, divisor/2 generates worse code
than divisor>>1.

yup. I wonder why the compiler doesn't do that for itself - is there a
case where it will generate a different result?

main()
{
int x = -5;
printf("%d %d\n", x>>1, x/2);
}
$ a.out
-3 -2

[Andrew Morton]

On Tue, 11 Nov 2008 15:42:09 -0800 (PST)

Post by Trent Piepho

Post by Andrew Morton
On Tue, 11 Nov 2008 15:05:02 -0800 (PST)

Post by Trent Piepho

Post by Darrick J. Wong
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
+#define DIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(divisor) __divisor = divisor; \
+ (((x) + ((__divisor) / 2)) / (__divisor)); \
+} \
+)

Maybe you can do away with the statement-expression extension? I've seen
cases where it cases gcc to generate worse code. It seems like it
shouldn't, but it does. I know DIV_ROUND_CLOSEST (maybe DIV_ROUND_NEAR?)
uses divisor twice, but all the also divide macros do that too, so why does
this one need to be different?

The others need fixing too.

Is it worth generating worse code for these simple macros?

Well that's an interesting question.

The risks with the current code are

a) It will introduce straightforward bugs, where pointers are
incremented twice, etc.

Hopefully these things will be apparent during testing and we'll
fix them up in the usual fashion.

b) It will introduce subtle slowdowns due to needlessly executing
code more than once, as in the hugepage case which I identified.
These problems will hang around for long periods.

So they're good reasons to fix the macros. If these fixes cause the
compiler to generate worse code then we should quantify and understand
that. Perhaps it is only certain compiler versions. Perhaps we can
find a test case (should be easy?) and send it over to the gcc guys to
fix. Perhaps we can find some C-level construct which prevents the
compiler from going into stupid mode without reintroducing the existing
problems.

Post by Trent Piepho

Post by Andrew Morton

Post by Trent Piepho
Note that if divisor is a signed variable, divisor/2 generates worse code
than divisor>>1.

yup. I wonder why the compiler doesn't do that for itself - is there a
case where it will generate a different result?

main()
{
int x = -5;
printf("%d %d\n", x>>1, x/2);
}
$ a.out
-3 -2

ah, thanks.

[Trent Piepho]

Post by Andrew Morton

Post by Trent Piepho

Post by Andrew Morton

Post by Trent Piepho

Post by Darrick J. Wong
+#define DIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(divisor) __divisor = divisor; \
+ (((x) + ((__divisor) / 2)) / (__divisor)); \
+} \
+)

Maybe you can do away with the statement-expression extension? I've seen
cases where it cases gcc to generate worse code. It seems like it
shouldn't, but it does. I know DIV_ROUND_CLOSEST (maybe DIV_ROUND_NEAR?)
uses divisor twice, but all the also divide macros do that too, so why does
this one need to be different?

The others need fixing too.

Is it worth generating worse code for these simple macros?

Well that's an interesting question.
The risks with the current code are
a) It will introduce straightforward bugs, where pointers are
incremented twice, etc.
Hopefully these things will be apparent during testing and we'll
fix them up in the usual fashion.
b) It will introduce subtle slowdowns due to needlessly executing
code more than once, as in the hugepage case which I identified.
These problems will hang around for long periods.
So they're good reasons to fix the macros. If these fixes cause the
compiler to generate worse code then we should quantify and understand
that. Perhaps it is only certain compiler versions. Perhaps we can
find a test case (should be easy?) and send it over to the gcc guys to
fix. Perhaps we can find some C-level construct which prevents the
compiler from going into stupid mode without reintroducing the existing
problems.

My question was more along the lines of is it worth it to even have macros for
something as simple rounding up when dividing?

For an example of statement expression problems, I noticed something with
swab16(), addressed in commit 8e2c20023f34b652605a5fb7c68bb843d2b100a8

#define ___swab16(x) \
({ \
__u16 __x = (x); \
((__u16)( \
(((__u16)(__x) & (__u16)0x00ffU) << 8) | \
(((__u16)(__x) & (__u16)0xff00U) >> 8) )); \
})

Produces this code:

movzwl %ax, %eax
movl %eax, %edx
shrl $8, %eax
sall $8, %edx
orl %eax, %edx

While this:

static __inline__ __attribute_const__ __u16 ___swab16(__u16 x)
{
return x<<8 | x>>8;
}

Produces this code:

rolw $8, %ax

[Andrew Morton]

On Fri, 14 Nov 2008 13:46:42 -0800 (PST)

Post by Trent Piepho

Post by Andrew Morton

Post by Trent Piepho

Post by Andrew Morton

Post by Trent Piepho

Post by Darrick J. Wong
+#define DIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(divisor) __divisor = divisor; \
+ (((x) + ((__divisor) / 2)) / (__divisor)); \
+} \
+)

Maybe you can do away with the statement-expression extension? I've seen
cases where it cases gcc to generate worse code. It seems like it
shouldn't, but it does. I know DIV_ROUND_CLOSEST (maybe DIV_ROUND_NEAR?)
uses divisor twice, but all the also divide macros do that too, so why does
this one need to be different?

The others need fixing too.

Is it worth generating worse code for these simple macros?

Well that's an interesting question.
The risks with the current code are
a) It will introduce straightforward bugs, where pointers are
incremented twice, etc.
Hopefully these things will be apparent during testing and we'll
fix them up in the usual fashion.
b) It will introduce subtle slowdowns due to needlessly executing
code more than once, as in the hugepage case which I identified.
These problems will hang around for long periods.
So they're good reasons to fix the macros. If these fixes cause the
compiler to generate worse code then we should quantify and understand
that. Perhaps it is only certain compiler versions. Perhaps we can
find a test case (should be easy?) and send it over to the gcc guys to
fix. Perhaps we can find some C-level construct which prevents the
compiler from going into stupid mode without reintroducing the existing
problems.

My question was more along the lines of is it worth it to even have macros for
something as simple rounding up when dividing?
For an example of statement expression problems, I noticed something with
swab16(), addressed in commit 8e2c20023f34b652605a5fb7c68bb843d2b100a8
#define ___swab16(x) \
({ \
__u16 __x = (x); \
((__u16)( \
(((__u16)(__x) & (__u16)0x00ffU) << 8) | \
(((__u16)(__x) & (__u16)0xff00U) >> 8) )); \
})
movzwl %ax, %eax
movl %eax, %edx
shrl $8, %eax
sall $8, %edx
orl %eax, %edx
static __inline__ __attribute_const__ __u16 ___swab16(__u16 x)
{
return x<<8 | x>>8;
}
rolw $8, %ax

stupid gcc.

I wonder if we could do something along these lines:

static inline u8 __div_round_up_u8(u8 n, u8 d)
{
...
}

static inline u16 __div_round_up_u16(u16 n, u16 d)
{
...
}

<etc>

#define DIV_ROUND_UP(n, d)
(sizeof(n) == 8 ? __div_round_up_u8(n, d) :
(sizeof(n) == 16 ? __div_round_up_u16(n, d) :
(sizeof(n) == 32 ? __div_round_up_u32(n, d) :
(sizeof(n) == 64 ? __div_round_up_u64(n, d) :
__panic_i_am_confused()))))

which might work but is arguably too stupid to live. And whcih still cannot
be used for compile-time array-sizing.

[Peter Zijlstra]

Post by Darrick J. Wong
Create a helper macro to divide two numbers and round the result to the
nearest whole number. This is a helper macro for hwmon drivers that
want to convert incoming sysfs values per standard hwmon practice, though
the macro itself can be used by anyone.

Its too late to rename this now isn't it :-/

DIV_ROUND_{UP,CEIL}
DIV_ROUND_{DOWN,FLOOR}

I get.

The proposed thing is simply DIV_ROUND, but this DIV_ROUND_CLOSEST name
is just wonky.

Also, shouldn't it be something like ?

DIV_ROUND(x, y) (((x) + (((y)+1)/2)) / (y))

Also, do we want the default rounding to be symmetric or asymmetric?

I think round to half even or something would be nice, but would add an
extra conditional, not sure its worth it (round away from zero only
works for signed numbers and it would also cost one conditional).

Post by Darrick J. Wong
Signed-off-by: Darrick J. Wong <djwong at us.ibm.com>
---
include/linux/kernel.h | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fba141d..fb02266 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
+#define DIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(divisor) __divisor = divisor; \
+ (((x) + ((__divisor) / 2)) / (__divisor)); \
+} \
+)
#define _RET_IP_ (unsigned long)__builtin_return_address(0)
#define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; })

[Jean Delvare]

Hi Peter,

Post by Peter Zijlstra

Post by Darrick J. Wong
Create a helper macro to divide two numbers and round the result to the
nearest whole number. This is a helper macro for hwmon drivers that
want to convert incoming sysfs values per standard hwmon practice, though
the macro itself can be used by anyone.

Its too late to rename this now isn't it :-/

We can always rename things, but it is costly, so we only do that if we
have a good reason (e.g. the original name caused confusion.)

Post by Peter Zijlstra
DIV_ROUND_{UP,CEIL}
DIV_ROUND_{DOWN,FLOOR}

The latter is the simple integer division, so there is no point in
defining a macro for it. The former we already have.

Post by Peter Zijlstra
I get.
The proposed thing is simply DIV_ROUND, but this DIV_ROUND_CLOSEST name
is just wonky.

I can't disagree. I even think I argued about it back then, but then
finally gave up. You should have participated in the debate when it was
hot rather than 8 months later :(

Post by Peter Zijlstra
Also, shouldn't it be something like ?
DIV_ROUND(x, y) (((x) + (((y)+1)/2)) / (y))

No, that wouldn't work. Simple example, 10/3 is 3.33. (10+3/2)/3 is 3
which is the correct rounded value. (10+(3+1)/2)/3 is 4 which is not
the correct rounded value.

Post by Peter Zijlstra
Also, do we want the default rounding to be symmetric or asymmetric?

I don't think we care at all. This macro will rarely be called on
negative numbers, and even then...

Post by Peter Zijlstra
I think round to half even or something would be nice, but would add an
extra conditional, not sure its worth it (round away from zero only
works for signed numbers and it would also cost one conditional).

Definitely not worth the extra cost. We do the rounding because it is
not too expensive and adds value. Fine-tuning it will cost more than is
worth, plus would lead to endless debates about what is the best
rounding strategy.

If anyone really needs additional control on rounding, they better do
not rely on a general helper, but instead implement their own rounding
function.

[Peter Zijlstra]

Post by Jean Delvare

Post by Peter Zijlstra
The proposed thing is simply DIV_ROUND, but this DIV_ROUND_CLOSEST name
is just wonky.

I can't disagree. I even think I argued about it back then, but then
finally gave up. You should have participated in the debate when it was
hot rather than 8 months later :(

Yeah, sometimes its hard to keep up with all on lkml, I only noticed it
because I ran into some code that used it.

Agreed on the other points.