[PATCH 0/3 V2] time: use __builtin_constant_p() in msecs_to_jiffies

[PATCH 0/3 V2] time: use __builtin_constant_p() in msecs_to_jiffies

[Nicholas Mc Guire]

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 <joe@perches.com>
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

[PATCH 1/3 V2] time: move timeconst.h into include/generated

[Nicholas Mc Guire]

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 <hofrat@osadl.org>
---

Thanks to Joe Perches <joe@perches.com> 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

Re: [PATCH 1/3 V2] time: move timeconst.h into include/generated

[Rob Landley]

On Sun, Apr 12, 2015 at 7:13 AM, Nicholas Mc Guire <hofrat@osadl.org> 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 <hofrat@osadl.org>

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

[PATCH 2/3 V2] time: allow gcc to fold constants when using

[Nicholas Mc Guire]

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 <hofrat@osadl.org>
---

Thanks to Joe Perches <joe@perches.com> 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

Re: [PATCH 2/3 V2] time: allow gcc to fold constants when using

[Thomas Gleixner]

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

Re: [PATCH 2/3 V2] time: allow gcc to fold constants when using

[Joe Perches]

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.

Re: [PATCH 2/3 V2] time: allow gcc to fold constants when using

[Nicholas Mc Guire]

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 

Re: [PATCH 2/3 V2] time: allow gcc to fold constants when using

[Nicholas Mc Guire]

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 

Re: [PATCH 2/3 V2] time: allow gcc to fold constants when using

[Thomas Gleixner]

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

[PATCH 3/3 V2] time: update msecs_to_jiffies doc and move to kernel-doc

[Nicholas Mc Guire]

update the documentation of msecs_to_jiffies and move to kernel-doc format

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

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