* [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do @ 2008-11-11 1:01 ` Darrick J. Wong 0 siblings, 0 replies; 32+ messages in thread From: Darrick J. Wong @ 2008-11-11 1:01 UTC (permalink / raw) To: Darrick J. Wong, Jean Delvare; +Cc: Andrew Morton, linux-kernel, lm-sensors 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@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; }) _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2008-11-11 1:01 ` Darrick J. Wong 0 siblings, 0 replies; 32+ messages in thread From: Darrick J. Wong @ 2008-11-11 1:01 UTC (permalink / raw) To: Darrick J. Wong, Jean Delvare; +Cc: Andrew Morton, linux-kernel, lm-sensors 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@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; }) ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [lm-sensors] [PATCH 2/2] adt74{62, 70, 2008-11-11 1:01 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Darrick J. Wong @ 2008-11-11 1:01 ` Darrick J. Wong -1 siblings, 0 replies; 32+ messages in thread From: Darrick J. Wong @ 2008-11-11 1:01 UTC (permalink / raw) To: Darrick J. Wong, Jean Delvare; +Cc: Andrew Morton, linux-kernel, lm-sensors Modify some hwmon drivers to use DIV_ROUND_CLOSEST instead of bloating source with (naughty) macros. Signed-off-by: Darrick J. Wong <djwong@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); _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/2] adt74{62, 70, 73}: Use DIV_ROUND_CLOSEST for rounded division @ 2008-11-11 1:01 ` Darrick J. Wong 0 siblings, 0 replies; 32+ messages in thread From: Darrick J. Wong @ 2008-11-11 1:01 UTC (permalink / raw) To: Darrick J. Wong, Jean Delvare; +Cc: Andrew Morton, linux-kernel, lm-sensors Modify some hwmon drivers to use DIV_ROUND_CLOSEST instead of bloating source with (naughty) macros. Signed-off-by: Darrick J. Wong <djwong@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); ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2008-11-11 1:01 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Darrick J. Wong @ 2008-11-11 10:04 ` Jean Delvare -1 siblings, 0 replies; 32+ messages in thread From: Jean Delvare @ 2008-11-11 10:04 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Andrew Morton, linux-kernel, lm-sensors Hi Darrick, On Mon, 10 Nov 2008 17:01:32 -0800, Darrick J. Wong wrote: > > 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@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. -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2008-11-11 10:04 ` Jean Delvare 0 siblings, 0 replies; 32+ messages in thread From: Jean Delvare @ 2008-11-11 10:04 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Andrew Morton, linux-kernel, lm-sensors Hi Darrick, On Mon, 10 Nov 2008 17:01:32 -0800, Darrick J. Wong wrote: > > 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@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. -- Jean Delvare ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2008-11-11 10:04 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Jean Delvare @ 2008-11-11 17:07 ` Andrew Morton -1 siblings, 0 replies; 32+ messages in thread From: Andrew Morton @ 2008-11-11 17:07 UTC (permalink / raw) To: Jean Delvare; +Cc: Darrick J. Wong, linux-kernel, lm-sensors On Tue, 11 Nov 2008 11:04:54 +0100 Jean Delvare <khali@linux-fr.org> wrote: > Hi Darrick, > > On Mon, 10 Nov 2008 17:01:32 -0800, Darrick J. Wong wrote: > > > > 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@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. _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2008-11-11 17:07 ` Andrew Morton 0 siblings, 0 replies; 32+ messages in thread From: Andrew Morton @ 2008-11-11 17:07 UTC (permalink / raw) To: Jean Delvare; +Cc: Darrick J. Wong, linux-kernel, lm-sensors On Tue, 11 Nov 2008 11:04:54 +0100 Jean Delvare <khali@linux-fr.org> wrote: > Hi Darrick, > > On Mon, 10 Nov 2008 17:01:32 -0800, Darrick J. Wong wrote: > > > > 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@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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2008-11-11 17:07 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Andrew Morton @ 2008-11-11 17:11 ` Jean Delvare -1 siblings, 0 replies; 32+ messages in thread From: Jean Delvare @ 2008-11-11 17:11 UTC (permalink / raw) To: Andrew Morton; +Cc: Darrick J. Wong, linux-kernel, lm-sensors On Tue, 11 Nov 2008 09:07:02 -0800, Andrew Morton wrote: > On Tue, 11 Nov 2008 11:04:54 +0100 Jean Delvare <khali@linux-fr.org> wrote: > > > Hi Darrick, > > > > On Mon, 10 Nov 2008 17:01:32 -0800, Darrick J. Wong wrote: > > > > > > 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@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. -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2008-11-11 17:11 ` Jean Delvare 0 siblings, 0 replies; 32+ messages in thread From: Jean Delvare @ 2008-11-11 17:11 UTC (permalink / raw) To: Andrew Morton; +Cc: Darrick J. Wong, linux-kernel, lm-sensors On Tue, 11 Nov 2008 09:07:02 -0800, Andrew Morton wrote: > On Tue, 11 Nov 2008 11:04:54 +0100 Jean Delvare <khali@linux-fr.org> wrote: > > > Hi Darrick, > > > > On Mon, 10 Nov 2008 17:01:32 -0800, Darrick J. Wong wrote: > > > > > > 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@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. -- Jean Delvare ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2008-11-11 17:07 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Andrew Morton @ 2008-11-11 18:51 ` Joe Perches -1 siblings, 0 replies; 32+ messages in thread From: Joe Perches @ 2008-11-11 18:51 UTC (permalink / raw) To: Andrew Morton; +Cc: Jean Delvare, Darrick J. Wong, linux-kernel, lm-sensors On Tue, 2008-11-11 at 09:07 -0800, Andrew Morton wrote: > On Tue, 11 Nov 2008 11:04:54 +0100 Jean Delvare <khali@linux-fr.org> wrote: > > On Mon, 10 Nov 2008 17:01:32 -0800, Darrick J. Wong wrote: > > > 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 _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2008-11-11 18:51 ` Joe Perches 0 siblings, 0 replies; 32+ messages in thread From: Joe Perches @ 2008-11-11 18:51 UTC (permalink / raw) To: Andrew Morton; +Cc: Jean Delvare, Darrick J. Wong, linux-kernel, lm-sensors On Tue, 2008-11-11 at 09:07 -0800, Andrew Morton wrote: > On Tue, 11 Nov 2008 11:04:54 +0100 Jean Delvare <khali@linux-fr.org> wrote: > > On Mon, 10 Nov 2008 17:01:32 -0800, Darrick J. Wong wrote: > > > 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2008-11-11 1:01 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Darrick J. Wong @ 2008-11-11 23:05 ` Trent Piepho -1 siblings, 0 replies; 32+ messages in thread From: Trent Piepho @ 2008-11-11 23:05 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Jean Delvare, Andrew Morton, linux-kernel, lm-sensors On Mon, 10 Nov 2008, Darrick J. Wong wrote: > 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. _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2008-11-11 23:05 ` Trent Piepho 0 siblings, 0 replies; 32+ messages in thread From: Trent Piepho @ 2008-11-11 23:05 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Jean Delvare, Andrew Morton, linux-kernel, lm-sensors On Mon, 10 Nov 2008, Darrick J. Wong wrote: > 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2008-11-11 23:05 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Trent Piepho @ 2008-11-11 23:20 ` Andrew Morton -1 siblings, 0 replies; 32+ messages in thread From: Andrew Morton @ 2008-11-11 23:20 UTC (permalink / raw) To: Trent Piepho; +Cc: djwong, khali, linux-kernel, lm-sensors On Tue, 11 Nov 2008 15:05:02 -0800 (PST) Trent Piepho <tpiepho@freescale.com> wrote: > On Mon, 10 Nov 2008, Darrick J. Wong wrote: > > 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. > 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? _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2008-11-11 23:20 ` Andrew Morton 0 siblings, 0 replies; 32+ messages in thread From: Andrew Morton @ 2008-11-11 23:20 UTC (permalink / raw) To: Trent Piepho; +Cc: djwong, khali, linux-kernel, lm-sensors On Tue, 11 Nov 2008 15:05:02 -0800 (PST) Trent Piepho <tpiepho@freescale.com> wrote: > On Mon, 10 Nov 2008, Darrick J. Wong wrote: > > 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. > 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? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2008-11-11 23:20 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Andrew Morton @ 2008-11-11 23:42 ` Trent Piepho -1 siblings, 0 replies; 32+ messages in thread From: Trent Piepho @ 2008-11-11 23:42 UTC (permalink / raw) To: Andrew Morton; +Cc: djwong, Jean Delvare, linux-kernel, lm-sensors On Tue, 11 Nov 2008, Andrew Morton wrote: > On Tue, 11 Nov 2008 15:05:02 -0800 (PST) > Trent Piepho <tpiepho@freescale.com> wrote: >> On Mon, 10 Nov 2008, Darrick J. Wong wrote: >>> #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? >> 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 _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2008-11-11 23:42 ` Trent Piepho 0 siblings, 0 replies; 32+ messages in thread From: Trent Piepho @ 2008-11-11 23:42 UTC (permalink / raw) To: Andrew Morton; +Cc: djwong, Jean Delvare, linux-kernel, lm-sensors On Tue, 11 Nov 2008, Andrew Morton wrote: > On Tue, 11 Nov 2008 15:05:02 -0800 (PST) > Trent Piepho <tpiepho@freescale.com> wrote: >> On Mon, 10 Nov 2008, Darrick J. Wong wrote: >>> #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? >> 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2008-11-11 23:42 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Trent Piepho @ 2008-11-12 0:08 ` Andrew Morton -1 siblings, 0 replies; 32+ messages in thread From: Andrew Morton @ 2008-11-12 0:08 UTC (permalink / raw) To: Trent Piepho; +Cc: djwong, khali, linux-kernel, lm-sensors On Tue, 11 Nov 2008 15:42:09 -0800 (PST) Trent Piepho <tpiepho@freescale.com> wrote: > On Tue, 11 Nov 2008, Andrew Morton wrote: > > On Tue, 11 Nov 2008 15:05:02 -0800 (PST) > > Trent Piepho <tpiepho@freescale.com> wrote: > >> On Mon, 10 Nov 2008, Darrick J. Wong wrote: > >>> #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. > >> 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. _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2008-11-12 0:08 ` Andrew Morton 0 siblings, 0 replies; 32+ messages in thread From: Andrew Morton @ 2008-11-12 0:08 UTC (permalink / raw) To: Trent Piepho; +Cc: djwong, khali, linux-kernel, lm-sensors On Tue, 11 Nov 2008 15:42:09 -0800 (PST) Trent Piepho <tpiepho@freescale.com> wrote: > On Tue, 11 Nov 2008, Andrew Morton wrote: > > On Tue, 11 Nov 2008 15:05:02 -0800 (PST) > > Trent Piepho <tpiepho@freescale.com> wrote: > >> On Mon, 10 Nov 2008, Darrick J. Wong wrote: > >>> #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. > >> 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2008-11-12 0:08 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Andrew Morton @ 2008-11-14 21:46 ` Trent Piepho -1 siblings, 0 replies; 32+ messages in thread From: Trent Piepho @ 2008-11-14 21:46 UTC (permalink / raw) To: Andrew Morton; +Cc: Trent Piepho, djwong, khali, linux-kernel, lm-sensors On Tue, 11 Nov 2008, Andrew Morton wrote: >>>>> +#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 _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2008-11-14 21:46 ` Trent Piepho 0 siblings, 0 replies; 32+ messages in thread From: Trent Piepho @ 2008-11-14 21:46 UTC (permalink / raw) To: Andrew Morton; +Cc: Trent Piepho, djwong, khali, linux-kernel, lm-sensors On Tue, 11 Nov 2008, Andrew Morton wrote: >>>>> +#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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2008-11-14 21:46 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Trent Piepho @ 2008-11-14 22:24 ` Andrew Morton -1 siblings, 0 replies; 32+ messages in thread From: Andrew Morton @ 2008-11-14 22:24 UTC (permalink / raw) To: Trent Piepho; +Cc: tpiepho, djwong, khali, linux-kernel, lm-sensors On Fri, 14 Nov 2008 13:46:42 -0800 (PST) Trent Piepho <tpiepho@freescale.com> wrote: > On Tue, 11 Nov 2008, Andrew Morton wrote: > >>>>> +#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 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. _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2008-11-14 22:24 ` Andrew Morton 0 siblings, 0 replies; 32+ messages in thread From: Andrew Morton @ 2008-11-14 22:24 UTC (permalink / raw) To: Trent Piepho; +Cc: tpiepho, djwong, khali, linux-kernel, lm-sensors On Fri, 14 Nov 2008 13:46:42 -0800 (PST) Trent Piepho <tpiepho@freescale.com> wrote: > On Tue, 11 Nov 2008, Andrew Morton wrote: > >>>>> +#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 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2008-11-11 23:20 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Andrew Morton @ 2008-11-11 23:50 ` Jochen Voß -1 siblings, 0 replies; 32+ messages in thread From: Jochen Voß @ 2008-11-11 23:50 UTC (permalink / raw) To: Andrew Morton; +Cc: Trent Piepho, djwong, khali, linux-kernel, lm-sensors Hi, 2008/11/11 Andrew Morton <akpm@linux-foundation.org>: > 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 -- http://seehuhn.de/ _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2008-11-11 23:50 ` Jochen Voß 0 siblings, 0 replies; 32+ messages in thread From: Jochen Voß @ 2008-11-11 23:50 UTC (permalink / raw) To: Andrew Morton; +Cc: Trent Piepho, djwong, khali, linux-kernel, lm-sensors Hi, 2008/11/11 Andrew Morton <akpm@linux-foundation.org>: > 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 -- http://seehuhn.de/ ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2008-11-11 1:01 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Darrick J. Wong @ 2009-08-03 11:57 ` Peter Zijlstra -1 siblings, 0 replies; 32+ messages in thread From: Peter Zijlstra @ 2009-08-03 11:57 UTC (permalink / raw) To: Darrick J. Wong Cc: Jean Delvare, Andrew Morton, linux-kernel, lm-sensors, Julia Lawall On Mon, 2008-11-10 at 17:01 -0800, Darrick J. Wong wrote: > 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). > Signed-off-by: Darrick J. Wong <djwong@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; }) _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2009-08-03 11:57 ` Peter Zijlstra 0 siblings, 0 replies; 32+ messages in thread From: Peter Zijlstra @ 2009-08-03 11:57 UTC (permalink / raw) To: Darrick J. Wong Cc: Jean Delvare, Andrew Morton, linux-kernel, lm-sensors, Julia Lawall On Mon, 2008-11-10 at 17:01 -0800, Darrick J. Wong wrote: > 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). > Signed-off-by: Darrick J. Wong <djwong@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; }) ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2009-08-03 11:57 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Peter Zijlstra @ 2009-08-03 12:21 ` Jean Delvare -1 siblings, 0 replies; 32+ messages in thread From: Jean Delvare @ 2009-08-03 12:21 UTC (permalink / raw) To: Peter Zijlstra Cc: Darrick J. Wong, Andrew Morton, linux-kernel, lm-sensors, Julia Lawall Hi Peter, On Mon, 03 Aug 2009 13:57:52 +0200, Peter Zijlstra wrote: > On Mon, 2008-11-10 at 17:01 -0800, Darrick J. Wong wrote: > > 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.) > > 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. > > 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 :( > > 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. > 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... > > 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. -- Jean Delvare _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2009-08-03 12:21 ` Jean Delvare 0 siblings, 0 replies; 32+ messages in thread From: Jean Delvare @ 2009-08-03 12:21 UTC (permalink / raw) To: Peter Zijlstra Cc: Darrick J. Wong, Andrew Morton, linux-kernel, lm-sensors, Julia Lawall Hi Peter, On Mon, 03 Aug 2009 13:57:52 +0200, Peter Zijlstra wrote: > On Mon, 2008-11-10 at 17:01 -0800, Darrick J. Wong wrote: > > 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.) > > 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. > > 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 :( > > 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. > 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... > > 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. -- Jean Delvare ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do 2009-08-03 12:21 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Jean Delvare @ 2009-08-03 12:27 ` Peter Zijlstra -1 siblings, 0 replies; 32+ messages in thread From: Peter Zijlstra @ 2009-08-03 12:27 UTC (permalink / raw) To: Jean Delvare Cc: Darrick J. Wong, Andrew Morton, linux-kernel, lm-sensors, Julia Lawall On Mon, 2009-08-03 at 14:21 +0200, Jean Delvare wrote: > > 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. _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding @ 2009-08-03 12:27 ` Peter Zijlstra 0 siblings, 0 replies; 32+ messages in thread From: Peter Zijlstra @ 2009-08-03 12:27 UTC (permalink / raw) To: Jean Delvare Cc: Darrick J. Wong, Andrew Morton, linux-kernel, lm-sensors, Julia Lawall On Mon, 2009-08-03 at 14:21 +0200, Jean Delvare wrote: > > 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2009-08-03 12:27 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-11-11 1:01 [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Darrick J. Wong
2008-11-11 1:01 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Darrick J. Wong
2008-11-11 1:01 ` [lm-sensors] [PATCH 2/2] adt74{62, 70, Darrick J. Wong
2008-11-11 1:01 ` [PATCH 2/2] adt74{62, 70, 73}: Use DIV_ROUND_CLOSEST for rounded division Darrick J. Wong
2008-11-11 10:04 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Jean Delvare
2008-11-11 10:04 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Jean Delvare
2008-11-11 17:07 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Andrew Morton
2008-11-11 17:07 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Andrew Morton
2008-11-11 17:11 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Jean Delvare
2008-11-11 17:11 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Jean Delvare
2008-11-11 18:51 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Joe Perches
2008-11-11 18:51 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Joe Perches
2008-11-11 23:05 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Trent Piepho
2008-11-11 23:05 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Trent Piepho
2008-11-11 23:20 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Andrew Morton
2008-11-11 23:20 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Andrew Morton
2008-11-11 23:42 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Trent Piepho
2008-11-11 23:42 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Trent Piepho
2008-11-12 0:08 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Andrew Morton
2008-11-12 0:08 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Andrew Morton
2008-11-14 21:46 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Trent Piepho
2008-11-14 21:46 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Trent Piepho
2008-11-14 22:24 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Andrew Morton
2008-11-14 22:24 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Andrew Morton
2008-11-11 23:50 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Jochen Voß
2008-11-11 23:50 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Jochen Voß
2009-08-03 11:57 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Peter Zijlstra
2009-08-03 11:57 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Peter Zijlstra
2009-08-03 12:21 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Jean Delvare
2009-08-03 12:21 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Jean Delvare
2009-08-03 12:27 ` [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do Peter Zijlstra
2009-08-03 12:27 ` [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding Peter Zijlstra
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.