In the overall kernel source there currently are
2544 msecs_to_jiffies
126 usecs_to_jiffies
and a few places that are using var * HZ / 1000 constructs
which are not always safe (no check of corner cases) and should
be switched to msecs_to_jiffies (roughly 25 left).
Allowing gcc to fold constants for these calls that in most
cases are passing in constants (roughly 95%) has some potential
to improve performance (and should save a few bytes).
size impact is marginal and for Powerpc it is
actually a slight increase.
As the changes to the top level Kbuild will impact every architecture
this is probably not enough - but I think suitable for a first review
Once this is clean a patch for usecs_to_jiffies will be provided as well
The patch set:
0001 moves timeconst.h from kernel/time/ to include/generated/ and makes
it available early enough so that the build can use the constants
for msecs_to_jiffies
0002 rename msecs_to_jiffies() to __msecs_to_jiffies()
move the common HZ range specific #ifdef'ed code into a inline helper
and #ifdef the variant to use. This allows to re-use the helpers in
both the const and non-const variant of msecs_to_jiffies()
modified msecs_to_jiffies() to checks for constant via
call to __builtin_constant_p() and call __msecs_to_jiffies() if it
can't determine that the argument is constant.
0003 documentation update and reformatting to kernel-doc format
for msecs_to_jiffies() and __msecs_to_jiffies() - for the helpers
its left as comments.
Verification:
kernel configs tested are defconfigs with e.g.
make x86_64_defconfig + CONFIG_TEST_LKM=m, GCONFIG_HZ_300=y. CONFIG_HZ=300
check kernel/softirq.c
#define MAX_SOFTIRQ_TIME msecs_to_jiffies(2)
used in __do_softirq() is folded into a single addq $1, %rax
conversely kernel/sched/core.c:sched_rr_handler() is not constant and
is a full call __msecs_to_jiffies
added the test-case proposed by Joe Perches <[email protected]>
into lib/test_module.c:test_module_init()
<snip>
unsigned int m;
for (m = 10; m < 200; m += 10)
pr_info("msecs_to_jiffies(%u) is %lu\n",
m, msecs_to_jiffies(m));
pr_info("msecs_to_jiffies(%u) is %lu\n",
10, msecs_to_jiffies(10));
pr_info("msecs_to_jiffies(%u) is %lu\n",
100, msecs_to_jiffies(100));
pr_info("msecs_to_jiffies(%u) is %lu\n",
1000, msecs_to_jiffies(1000));
<snip>
without the patch applied:
test_module_init:
pushq %rbp #
movq %rsp, %rbp #,
pushq %rbx #
movl $10, %ebx #, m
pushq %rcx #
.L2:
movl %ebx, %edi # m,
call msecs_to_jiffies #
movl %ebx, %esi # m,
movq %rax, %rdx # D.14503,
movq $.LC0, %rdi #,
xorl %eax, %eax #
addl $10, %ebx #, m
call printk #
cmpl $200, %ebx #, m
jne .L2 #,
movl $10, %edi #, <--- msecs_to_jiffies(10)
call msecs_to_jiffies # <--- runtime conversion
movl $10, %esi #,
movq %rax, %rdx # D.14504,
movq $.LC0, %rdi #,
xorl %eax, %eax #
call printk #
movl $100, %edi #,
call msecs_to_jiffies #
movl $100, %esi #,
movq %rax, %rdx # D.14505,
movq $.LC0, %rdi #,
xorl %eax, %eax #
call printk #
movl $1000, %edi #,
call msecs_to_jiffies #
movl $1000, %esi #,
movq %rax, %rdx # D.14506,
movq $.LC0, %rdi #,
xorl %eax, %eax #
call printk #
movq $.LC1, %rdi #,
xorl %eax, %eax #
call printk #
popq %rdx #
popq %rbx #
xorl %eax, %eax #
popq %rbp #
ret
with the patch applied:
test_module_init:
pushq %rbp #
movq %rsp, %rbp #,
pushq %rbx #
movl $10, %ebx #, m
pushq %rcx #
.L2:
movl %ebx, %edi # m,
call __msecs_to_jiffies #
movl %ebx, %esi # m,
movq %rax, %rdx # D.14545,
movq $.LC0, %rdi #,
xorl %eax, %eax #
addl $10, %ebx #, m
call printk #
cmpl $200, %ebx #, m
jne .L2 #,
movl $3, %edx #, <--- msecs_to_jiffies(10) == 3 jiffies
movl $10, %esi #, <--- const 10 passed to printk
movq $.LC0, %rdi #,
xorl %eax, %eax #
call printk #
movl $30, %edx #,
movl $100, %esi #,
movq $.LC0, %rdi #,
xorl %eax, %eax #
call printk #
movl $300, %edx #,
movl $1000, %esi #,
movq $.LC0, %rdi #,
xorl %eax, %eax #
call printk #
movq $.LC1, %rdi #,
xorl %eax, %eax #
call printk #
popq %rdx #
popq %rbx #
xorl %eax, %eax #
popq %rbp #
ret
kernel/time/timeconst.h is moved to include/generated/ and generated in
an early build stage by top level Kbuild. This allows using timeconst.h
in an earlier stage of the build.
Signed-off-by: Nicholas Mc Guire <[email protected]>
---
Thanks to Joe Perches <[email protected]> for suggesting this approach and
catching the unconditional rebuild (should be fixed here now properly) as
well as for his review comments on the first attempts as well as for the
macro version that the reformatting of V2 re-uses.
V2: no changes in this one from V1
Some questions that remain:
* Kbuild - is passing in arguments to .bc files via pipe and read();
rather than using multiple files acceptable or is there some reason
for the original multi-file solution that Im overlooking ?
* conditional rebuild of timeconst.h with $(call filechk,gentimeconst)
not really clear if this is going to do all rebuilds that could be
necessary and not clear how to verify this. currently only checked by
1) defconfig -> build -> rebuild -> CHK is executed timeconst.h unmodified
2) defconfig -> build -> change HZ -> rebuild -> UPD timeconst.h resulting
in a rebuild of most files.
Patch was compile tested for x86_64_defconfig,multi_v7_defconfig,
ppc64_defconfig, and boot/run
tested on x86_64. Further a test-case with an invalid HZ value to trigger the
error output in kernel/time/timeconst.bc was run.
Patch is against 4.0-rc7 (localversion-next is -next-20150410)
Kbuild | 30 +++++++++++++++++++++++++-----
kernel/time/Makefile | 17 +----------------
kernel/time/time.c | 2 +-
kernel/time/timeconst.bc | 3 ++-
4 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/Kbuild b/Kbuild
index 6f0d82a..1d1aef5 100644
--- a/Kbuild
+++ b/Kbuild
@@ -2,8 +2,9 @@
# Kbuild for top-level directory of the kernel
# This file takes care of the following:
# 1) Generate bounds.h
-# 2) Generate asm-offsets.h (may need bounds.h)
-# 3) Check for missing system calls
+# 2) Generate timeconst.h
+# 3) Generate asm-offsets.h (may need bounds.h)
+# 4) Check for missing system calls
# Default sed regexp - multiline due to syntax constraints
define sed-y
@@ -47,7 +48,26 @@ $(obj)/$(bounds-file): kernel/bounds.s FORCE
$(call filechk,offsets,__LINUX_BOUNDS_H__)
#####
-# 2) Generate asm-offsets.h
+# 2) Generate timeconst.h
+
+timeconst-file := include/generated/timeconst.h
+
+always += $(timeconst-file)
+targets += $(timeconst-file)
+
+quiet_cmd_gentimeconst = GEN $@
+define cmd_gentimeconst
+ (echo $(CONFIG_HZ) | bc -q $< ) > $@
+endef
+define filechk_gentimeconst
+ (echo $(CONFIG_HZ) | bc -q $< )
+endef
+
+$(obj)/$(timeconst-file): kernel/time/timeconst.bc FORCE
+ $(call filechk,gentimeconst)
+
+#####
+# 3) Generate asm-offsets.h
#
offsets-file := include/generated/asm-offsets.h
@@ -65,7 +85,7 @@ $(obj)/$(offsets-file): arch/$(SRCARCH)/kernel/asm-offsets.s FORCE
$(call filechk,offsets,__ASM_OFFSETS_H__)
#####
-# 3) Check for missing system calls
+# 4) Check for missing system calls
#
always += missing-syscalls
@@ -78,4 +98,4 @@ missing-syscalls: scripts/checksyscalls.sh $(offsets-file) FORCE
$(call cmd,syscalls)
# Keep these two files during make clean
-no-clean-files := $(bounds-file) $(offsets-file)
+no-clean-files := $(bounds-file) $(offsets-file) $(timeconst-file)
diff --git a/kernel/time/Makefile b/kernel/time/Makefile
index 01f0312..ffc4cc3 100644
--- a/kernel/time/Makefile
+++ b/kernel/time/Makefile
@@ -13,19 +13,4 @@ obj-$(CONFIG_TIMER_STATS) += timer_stats.o
obj-$(CONFIG_DEBUG_FS) += timekeeping_debug.o
obj-$(CONFIG_TEST_UDELAY) += test_udelay.o
-$(obj)/time.o: $(obj)/timeconst.h
-
-quiet_cmd_hzfile = HZFILE $@
- cmd_hzfile = echo "hz=$(CONFIG_HZ)" > $@
-
-targets += hz.bc
-$(obj)/hz.bc: $(objtree)/include/config/hz.h FORCE
- $(call if_changed,hzfile)
-
-quiet_cmd_bc = BC $@
- cmd_bc = bc -q $(filter-out FORCE,$^) > $@
-
-targets += timeconst.h
-$(obj)/timeconst.h: $(obj)/hz.bc $(src)/timeconst.bc FORCE
- $(call if_changed,bc)
-
+$(obj)/time.o: $(objtree)/include/config/
diff --git a/kernel/time/time.c b/kernel/time/time.c
index 2c85b77..4fa1d26 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -41,7 +41,7 @@
#include <asm/uaccess.h>
#include <asm/unistd.h>
-#include "timeconst.h"
+#include <generated/timeconst.h>
#include "timekeeping.h"
/*
diff --git a/kernel/time/timeconst.bc b/kernel/time/timeconst.bc
index 511bdf2..c7388de 100644
--- a/kernel/time/timeconst.bc
+++ b/kernel/time/timeconst.bc
@@ -50,7 +50,7 @@ define timeconst(hz) {
print "#include <linux/types.h>\n\n"
print "#if HZ != ", hz, "\n"
- print "#error \qkernel/timeconst.h has the wrong HZ value!\q\n"
+ print "#error \qinclude/generated/timeconst.h has the wrong HZ value!\q\n"
print "#endif\n\n"
if (hz < 2) {
@@ -105,4 +105,5 @@ define timeconst(hz) {
halt
}
+hz = read();
timeconst(hz)
--
1.7.10.4
The original msecs_to_jiffies is renamed to __msecs_to_jiffies and aside
from the removal of the check for negative values being moved out, is
unaltered.
Signed-off-by: Nicholas Mc Guire <[email protected]>
---
Thanks to Joe Perches <[email protected]> for suggesting this approach and
for his review comments on the first attempts and for the simple
test-case that helped after I confused my self with .lst files.
V2: remove duplicated code by moving it to helper functions
reformatted the #ifdef to a hopefully more readable code in
include/linux/jiffies.h and simplified __msecs_to_jiffies()
by re-using the helpers.
Question:
* nameing conventions _msecs_to_jiffies() suitable as helper name ?
* the suitability of the #ifdef/#elif/#else construct
Patch was compile and boot tested (x86_64) and a few msecs_to_jiffies()
locations verified by inspection of the .s files where constants are
reduced to a load or add instruction and the non-const. call
__msecs_to_jiffies().
Patch is against 4.0-rc7 (localversion-next is -next-20150410)
include/linux/jiffies.h | 35 ++++++++++++++++++++++++++++++++++-
kernel/time/time.c | 40 ++++++----------------------------------
2 files changed, 40 insertions(+), 35 deletions(-)
diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
index c367cbd..273b093 100644
--- a/include/linux/jiffies.h
+++ b/include/linux/jiffies.h
@@ -7,6 +7,7 @@
#include <linux/time.h>
#include <linux/timex.h>
#include <asm/param.h> /* for HZ */
+#include <generated/timeconst.h>
/*
* The following defines establish the engineering parameters of the PLL
@@ -288,7 +289,39 @@ static inline u64 jiffies_to_nsecs(const unsigned long j)
return (u64)jiffies_to_usecs(j) * NSEC_PER_USEC;
}
-extern unsigned long msecs_to_jiffies(const unsigned int m);
+extern unsigned long __msecs_to_jiffies(const unsigned int m);
+#if HZ <= MSEC_PER_SEC && !(MSEC_PER_SEC % HZ)
+static inline unsigned long _msecs_to_jiffies(const unsigned int m)
+{
+ return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);
+}
+#elif HZ > MSEC_PER_SEC && !(HZ % MSEC_PER_SEC)
+static inline unsigned long _msecs_to_jiffies(const unsigned int m)
+{
+ if (m > jiffies_to_msecs(MAX_JIFFY_OFFSET))
+ return MAX_JIFFY_OFFSET;
+ return m * (HZ / MSEC_PER_SEC);
+}
+#else
+static inline unsigned long _msecs_to_jiffies(const unsigned int m)
+{
+ if (HZ > MSEC_PER_SEC && m > jiffies_to_msecs(MAX_JIFFY_OFFSET))
+ return MAX_JIFFY_OFFSET;
+
+ return (MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32)
+ >> MSEC_TO_HZ_SHR32;
+}
+#endif
+static inline unsigned long msecs_to_jiffies(const unsigned int m)
+{
+ if (__builtin_constant_p(m)) {
+ if ((int)m < 0)
+ return MAX_JIFFY_OFFSET;
+ return _msecs_to_jiffies(m);
+ } else
+ return __msecs_to_jiffies(m);
+}
+
extern unsigned long usecs_to_jiffies(const unsigned int u);
extern unsigned long timespec_to_jiffies(const struct timespec *value);
extern void jiffies_to_timespec(const unsigned long jiffies,
diff --git a/kernel/time/time.c b/kernel/time/time.c
index 4fa1d26..25bee56 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -488,6 +488,8 @@ EXPORT_SYMBOL(ns_to_timespec64);
* the following way:
*
* - negative values mean 'infinite timeout' (MAX_JIFFY_OFFSET)
+ * negative values are handled in msecs_to_jiffies in
+ * include/linux/jiffies.h
*
* - 'too large' values [that would result in larger than
* MAX_JIFFY_OFFSET values] mean 'infinite timeout' too.
@@ -496,48 +498,18 @@ EXPORT_SYMBOL(ns_to_timespec64);
* the input value by a factor or dividing it with a factor
*
* We must also be careful about 32-bit overflows.
+ *
*/
-unsigned long msecs_to_jiffies(const unsigned int m)
+unsigned long __msecs_to_jiffies(const unsigned int m)
{
/*
* Negative value, means infinite timeout:
*/
if ((int)m < 0)
return MAX_JIFFY_OFFSET;
-
-#if HZ <= MSEC_PER_SEC && !(MSEC_PER_SEC % HZ)
- /*
- * HZ is equal to or smaller than 1000, and 1000 is a nice
- * round multiple of HZ, divide with the factor between them,
- * but round upwards:
- */
- return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);
-#elif HZ > MSEC_PER_SEC && !(HZ % MSEC_PER_SEC)
- /*
- * HZ is larger than 1000, and HZ is a nice round multiple of
- * 1000 - simply multiply with the factor between them.
- *
- * But first make sure the multiplication result cannot
- * overflow:
- */
- if (m > jiffies_to_msecs(MAX_JIFFY_OFFSET))
- return MAX_JIFFY_OFFSET;
-
- return m * (HZ / MSEC_PER_SEC);
-#else
- /*
- * Generic case - multiply, round and divide. But first
- * check that if we are doing a net multiplication, that
- * we wouldn't overflow:
- */
- if (HZ > MSEC_PER_SEC && m > jiffies_to_msecs(MAX_JIFFY_OFFSET))
- return MAX_JIFFY_OFFSET;
-
- return (MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32)
- >> MSEC_TO_HZ_SHR32;
-#endif
+ return _msecs_to_jiffies(m);
}
-EXPORT_SYMBOL(msecs_to_jiffies);
+EXPORT_SYMBOL(__msecs_to_jiffies);
unsigned long usecs_to_jiffies(const unsigned int u)
{
--
1.7.10.4
update the documentation of msecs_to_jiffies and move to kernel-doc format
Signed-off-by: Nicholas Mc Guire <[email protected]>
---
V2: reformatting of documentation for the Hz range dependent
_msecs_to_jiffies() helpers
Patch is against 4.0-rc7 (localversion-next is -next-20150410)
include/linux/jiffies.h | 39 +++++++++++++++++++++++++++++++++++++++
kernel/time/time.c | 25 ++++++++++++++++---------
2 files changed, 55 insertions(+), 9 deletions(-)
diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
index 273b093..bb8d0d7 100644
--- a/include/linux/jiffies.h
+++ b/include/linux/jiffies.h
@@ -291,11 +291,22 @@ static inline u64 jiffies_to_nsecs(const unsigned long j)
extern unsigned long __msecs_to_jiffies(const unsigned int m);
#if HZ <= MSEC_PER_SEC && !(MSEC_PER_SEC % HZ)
+/*
+ * HZ is equal to or smaller than 1000, and 1000 is a nice round
+ * multiple of HZ, divide with the factor between them, but round
+ * upwards:
+ */
static inline unsigned long _msecs_to_jiffies(const unsigned int m)
{
return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);
}
#elif HZ > MSEC_PER_SEC && !(HZ % MSEC_PER_SEC)
+/*
+ * HZ is larger than 1000, and HZ is a nice round multiple of 1000 -
+ * simply multiply with the factor between them.
+ *
+ * But first make sure the multiplication result cannot overflow:
+ */
static inline unsigned long _msecs_to_jiffies(const unsigned int m)
{
if (m > jiffies_to_msecs(MAX_JIFFY_OFFSET))
@@ -303,6 +314,10 @@ static inline unsigned long _msecs_to_jiffies(const unsigned int m)
return m * (HZ / MSEC_PER_SEC);
}
#else
+/*
+ * Generic case - multiply, round and divide. But first check that if
+ * we are doing a net multiplication, that we wouldn't overflow:
+ */
static inline unsigned long _msecs_to_jiffies(const unsigned int m)
{
if (HZ > MSEC_PER_SEC && m > jiffies_to_msecs(MAX_JIFFY_OFFSET))
@@ -312,6 +327,30 @@ static inline unsigned long _msecs_to_jiffies(const unsigned int m)
>> MSEC_TO_HZ_SHR32;
}
#endif
+/**
+ * msecs_to_jiffies: - convert milliseconds to jiffies
+ * @m: time in milliseconds
+ *
+ * conversion is done as follows:
+ *
+ * - negative values mean 'infinite timeout' (MAX_JIFFY_OFFSET)
+ *
+ * - 'too large' values [that would result in larger than
+ * MAX_JIFFY_OFFSET values] mean 'infinite timeout' too.
+ *
+ * - all other values are converted to jiffies by either multiplying
+ * the input value by a factor or dividing it with a factor and
+ * handling any 32-bit overflows.
+ * for the details see __msecs_to_jiffies()
+ *
+ * msecs_to_jiffies() checks for the passed in value being a constant
+ * via __builtin_constant_p() allowing gcc to eliminate most of the
+ * code, __msecs_to_jiffies() is called if the value passed does not
+ * allow constant folding and the actual conversion must be done at
+ * runtime.
+ * the HZ range specific helpers _msecs_to_jiffies() are used for both
+ * cases
+ */
static inline unsigned long msecs_to_jiffies(const unsigned int m)
{
if (__builtin_constant_p(m)) {
diff --git a/kernel/time/time.c b/kernel/time/time.c
index f5196aa..884bb8e 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -483,22 +483,29 @@ struct timespec64 ns_to_timespec64(const s64 nsec)
}
EXPORT_SYMBOL(ns_to_timespec64);
#endif
-/*
- * When we convert to jiffies then we interpret incoming values
- * the following way:
+/**
+ * msecs_to_jiffies: - convert milliseconds to jiffies
+ * @m: time in milliseconds
+ *
+ * conversion is done as follows:
*
* - negative values mean 'infinite timeout' (MAX_JIFFY_OFFSET)
- * negative values are handled in msecs_to_jiffies in
- * include/linux/jiffies.h
*
* - 'too large' values [that would result in larger than
* MAX_JIFFY_OFFSET values] mean 'infinite timeout' too.
*
* - all other values are converted to jiffies by either multiplying
- * the input value by a factor or dividing it with a factor
- *
- * We must also be careful about 32-bit overflows.
- *
+ * the input value by a factor or dividing it with a factor and
+ * handling any 32-bit overflows.
+ * for the details see __msecs_to_jiffies()
+ *
+ * msecs_to_jiffies() checks for the passed in value being a constant
+ * via __builtin_constant_p() allowing gcc to eliminate most of the
+ * code, __msecs_to_jiffies() is called if the value passed does not
+ * allow constant folding and the actual conversion must be done at
+ * runtime.
+ * the _msecs_to_jiffies helpers are the HZ dependent conversion
+ * routines found in include/linux/jiffies.h
*/
unsigned long __msecs_to_jiffies(const unsigned int m)
{
--
1.7.10.4
On Sun, Apr 12, 2015 at 7:13 AM, Nicholas Mc Guire <[email protected]> wrote:
> kernel/time/timeconst.h is moved to include/generated/ and generated in
> an early build stage by top level Kbuild. This allows using timeconst.h
> in an earlier stage of the build.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
Sigh, I'll adjust my patch:
http://landley.net/hg/aboriginal/file/1698/sources/patches/linux-noperl-timeconst.patch
Backstory: Peter Anvin added perl to the kernel build in 2.6.25 and
something like the 9th time I submitted patches to remove it, several
years later when it looked like they'd finally go in, he submitted a
competing patch to one of my "just do this in C and shell" patch
series to instead add a dependency on the 'bc" tool, which is not in
busybox, wasn't in the linux from scratch or buildroot or openembedded
builds (everybody had to add it after his patch went in to keep
building the kernel), and which is actually hard to implement in a
posix compliant way for an embedded environment because it's defined
as requiring arbitrary precision math (all timeconst calculation needs
is 64 bit math, I.E. long long) and it's defined as being capable of
doing things like fractional exponentiation at arbitrary precision
(fixed point, I think).
I don't know why Peter is on a crusade to stamp out simple build
environments. He added perl as a build dependency to every tool he
maintained at the same time (not just the kernel but also klibc and
his bootloader), and then when he couldn't defend perl he added a new
dependency to break existing simple/audited build environments.
When I objected to him about the perl he said I was engaged in an
uninteresting "academic" exercise.
(https://lkml.org/lkml/2008/2/15/548) Oh yes, a simple auditable build
environment totally has no real world consequences:
http://www.kith.org/journals/jed/2015/03/15/15043.html
Yup, none at all. Snowden proved that, clearly. (And he was totally a
one-off, it's not like there was Manning or Daniel Ellsberg or Mark
Felt before him...)
Sigh. Don't mind me, I'll update my local patch, once again.
Rob
On Sun, 12 Apr 2015, Nicholas Mc Guire wrote:
> +extern unsigned long __msecs_to_jiffies(const unsigned int m);
> +#if HZ <= MSEC_PER_SEC && !(MSEC_PER_SEC % HZ)
> +static inline unsigned long _msecs_to_jiffies(const unsigned int m)
> +{
This should move the comments explaining the logic for each variant as
well.
> + return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);
> +}
> +#elif HZ > MSEC_PER_SEC && !(HZ % MSEC_PER_SEC)
> +static inline unsigned long _msecs_to_jiffies(const unsigned int m)
> +{
> + if (m > jiffies_to_msecs(MAX_JIFFY_OFFSET))
> + return MAX_JIFFY_OFFSET;
> + return m * (HZ / MSEC_PER_SEC);
> +}
> +#else
> +static inline unsigned long _msecs_to_jiffies(const unsigned int m)
> +{
> + if (HZ > MSEC_PER_SEC && m > jiffies_to_msecs(MAX_JIFFY_OFFSET))
> + return MAX_JIFFY_OFFSET;
> +
> + return (MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32)
> + >> MSEC_TO_HZ_SHR32;
> +}
> +#endif
> +static inline unsigned long msecs_to_jiffies(const unsigned int m)
> +{
> + if (__builtin_constant_p(m)) {
> + if ((int)m < 0)
> + return MAX_JIFFY_OFFSET;
> + return _msecs_to_jiffies(m);
> + } else
> + return __msecs_to_jiffies(m);
It'd be nice to have this as two patches:
1) Factor out the code into inline helpers w/o adding anything
2) Add the __builtin_constant_p() check
Thanks,
tglx
On Wed, 2015-04-22 at 14:00 +0200, Thomas Gleixner wrote:
> On Sun, 12 Apr 2015, Nicholas Mc Guire wrote:
> > +extern unsigned long __msecs_to_jiffies(const unsigned int m);
> > +#if HZ <= MSEC_PER_SEC && !(MSEC_PER_SEC % HZ)
> > +static inline unsigned long _msecs_to_jiffies(const unsigned int m)
> > +{
>
> This should move the comments explaining the logic for each variant as
> well.
[]
> It'd be nice to have this as two patches:
>
> 1) Factor out the code into inline helpers w/o adding anything
It also might be nice to use a single static inline
with #ifdef blocks inside that inline rather than
have 3 separate static inline msecs_to_jiffies.
> 2) Add the __builtin_constant_p() check
And please add the usecs_to_jiffies variants in a
similar patch set.
On Wed, 22 Apr 2015, Thomas Gleixner wrote:
> On Sun, 12 Apr 2015, Nicholas Mc Guire wrote:
> > +extern unsigned long __msecs_to_jiffies(const unsigned int m);
> > +#if HZ <= MSEC_PER_SEC && !(MSEC_PER_SEC % HZ)
> > +static inline unsigned long _msecs_to_jiffies(const unsigned int m)
> > +{
>
> This should move the comments explaining the logic for each variant as
> well.
should be covered by [PATCH 3/3 V2] time: update msecs_to_jiffies doc and move to kernel-doc
>
> > + return (m + (MSEC_PER_SEC / HZ) - 1) / (MSEC_PER_SEC / HZ);
> > +}
> > +#elif HZ > MSEC_PER_SEC && !(HZ % MSEC_PER_SEC)
> > +static inline unsigned long _msecs_to_jiffies(const unsigned int m)
> > +{
> > + if (m > jiffies_to_msecs(MAX_JIFFY_OFFSET))
> > + return MAX_JIFFY_OFFSET;
> > + return m * (HZ / MSEC_PER_SEC);
> > +}
> > +#else
> > +static inline unsigned long _msecs_to_jiffies(const unsigned int m)
> > +{
> > + if (HZ > MSEC_PER_SEC && m > jiffies_to_msecs(MAX_JIFFY_OFFSET))
> > + return MAX_JIFFY_OFFSET;
> > +
> > + return (MSEC_TO_HZ_MUL32 * m + MSEC_TO_HZ_ADJ32)
> > + >> MSEC_TO_HZ_SHR32;
> > +}
> > +#endif
> > +static inline unsigned long msecs_to_jiffies(const unsigned int m)
> > +{
> > + if (__builtin_constant_p(m)) {
> > + if ((int)m < 0)
> > + return MAX_JIFFY_OFFSET;
> > + return _msecs_to_jiffies(m);
> > + } else
> > + return __msecs_to_jiffies(m);
>
> It'd be nice to have this as two patches:
>
> 1) Factor out the code into inline helpers w/o adding anything
>
> 2) Add the __builtin_constant_p() check
>
so basically 1) is refactoring only and 2) is the actual
change keept at a minimum or what is the intent of this split ?
will cleanup and resubmit - just waiting for some feedback on
the Kbuild change.
thx!
hofrat
On Wed, 22 Apr 2015, Nicholas Mc Guire wrote:
> On Wed, 22 Apr 2015, Thomas Gleixner wrote:
>
> > On Sun, 12 Apr 2015, Nicholas Mc Guire wrote:
> > > +extern unsigned long __msecs_to_jiffies(const unsigned int m);
> > > +#if HZ <= MSEC_PER_SEC && !(MSEC_PER_SEC % HZ)
> > > +static inline unsigned long _msecs_to_jiffies(const unsigned int m)
> > > +{
> >
> > This should move the comments explaining the logic for each variant as
> > well.
>
> should be covered by [PATCH 3/3 V2] time: update msecs_to_jiffies doc and move to kernel-doc
Well, it's covered but it makes no sense patch wise.
> > 1) Factor out the code into inline helpers w/o adding anything
> >
> > 2) Add the __builtin_constant_p() check
> >
>
> so basically 1) is refactoring only and 2) is the actual
> change keept at a minimum or what is the intent of this split ?
Right.
Thanks,
tglx
On Wed, 22 Apr 2015, Joe Perches wrote:
> On Wed, 2015-04-22 at 14:00 +0200, Thomas Gleixner wrote:
> > On Sun, 12 Apr 2015, Nicholas Mc Guire wrote:
> > > +extern unsigned long __msecs_to_jiffies(const unsigned int m);
> > > +#if HZ <= MSEC_PER_SEC && !(MSEC_PER_SEC % HZ)
> > > +static inline unsigned long _msecs_to_jiffies(const unsigned int m)
> > > +{
> >
> > This should move the comments explaining the logic for each variant as
> > well.
> []
> > It'd be nice to have this as two patches:
> >
> > 1) Factor out the code into inline helpers w/o adding anything
>
> It also might be nice to use a single static inline
> with #ifdef blocks inside that inline rather than
> have 3 separate static inline msecs_to_jiffies.
That was actually intentional to make it more readable - atleast
I thought its more readable this way than it
was before.
>
> > 2) Add the __builtin_constant_p() check
>
> And please add the usecs_to_jiffies variants in a
> similar patch set.
>
yup - as soon as its clean for msecs_to_jiffies applying
the same refactoring for usecs_to_jiffies is streight forward
so basically just waiting for the feedback on the
first one before pushing the usecs_to_jiffies case out.
thx!
hofrat