Re: [PATCH 1/3] Replace kernel/timeconst.pl with kernel/timeconst.sh

[Rob Landley <rob@landley.net>]

On Wednesday 09 December 2009 09:45:37 Michal Marek wrote:
> [CC hpa who wrote the timeconst.pl script]
>
> On 8.12.2009 10:19, Rob Landley wrote:
> > From: Rob Landley <rob@landley.net>
> >
> > Replace kernel/timeconst.pl with kernel/timeconst.sh. The new shell
> > script is much simpler, about 1/4 the size, and runs on Red Hat 9 from
> > 2003.
>
> I tried the shell script with the precomputed values in timeconst.pl and
> it gave me different results than the perl version for 250 and 1000:

You're right.

They're functionally equivalent (due to the relationship between MUL32 and 
SHR32), which is why this code has worked for me and other people for a year 
now.  The difference is the possibility of an integer overflow if you try to 
convert a time period of "0xffffffff".

Trivial fix:

--- a/sources/patches/linux-2.6.25-rc1-noperl.patch	Tue Dec 08 20:22:53 2009 
-0600
+++ b/sources/patches/linux-2.6.25-rc1-noperl.patch	Wed Dec 09 17:28:26 2009 
-0600
@@ -507,7 +507,7 @@
 +
 +			# Keep increasing $SHIFT until we've got 32 bits.
 +
-+			[ $MUL32 -gt $(( 1 << 31 )) ] && break
++			[ $MUL32 -ge $(( 1 << 31 )) ] && break
 +			SHIFT=$(( $SHIFT + 1 ))
 +		done
 +		MUL32=$( printf %x $MUL32 )

As long as MUL32 fits in 32 bits than you can multiply it by another 32 bit 
number without overflow, and that's probably all the kernel's enforcing.

>  #define HZ_TO_MSEC_NUM          U64_C(4)
>  #define HZ_TO_MSEC_DEN          U64_C(1)
> -#define MSEC_TO_HZ_MUL32        U64_C(0x80000000)
> -#define MSEC_TO_HZ_ADJ32        U64_C(0x180000000)
> -#define MSEC_TO_HZ_SHR32        33
> +#define MSEC_TO_HZ_MUL32       U64_C(0x100000000)
> +#define MSEC_TO_HZ_ADJ32       U64_C(0x300000000)
> +#define MSEC_TO_HZ_SHR32       34
>  #define MSEC_TO_HZ_NUM          U64_C(1)
>  #define MSEC_TO_HZ_DEN          U64_C(4)
>  #define HZ_TO_USEC_MUL32        U64_C(0xfa000000)

The same.

> and with HZ=1000:
> --- perl
> +++ bash
> @@ -8,14 +8,14 @@
>  #error "kernel/timeconst.h has the wrong HZ value!"
>  #endif
>
> -#define HZ_TO_MSEC_MUL32        U64_C(0x80000000)
> +#define HZ_TO_MSEC_MUL32       U64_C(0x100000000)
>  #define HZ_TO_MSEC_ADJ32        U64_C(0x0)
> -#define HZ_TO_MSEC_SHR32        31
> +#define HZ_TO_MSEC_SHR32       32

And again.

>  #define HZ_TO_MSEC_NUM          U64_C(1)
>  #define HZ_TO_MSEC_DEN          U64_C(1)
> -#define MSEC_TO_HZ_MUL32        U64_C(0x80000000)
> +#define MSEC_TO_HZ_MUL32       U64_C(0x100000000)
>  #define MSEC_TO_HZ_ADJ32        U64_C(0x0)
> -#define MSEC_TO_HZ_SHR32        31
> +#define MSEC_TO_HZ_SHR32       32

Same thing.

> You're trying to avoid the build dependency on Perl. What about adding a
> timeconst.h_shipped with the precomputed values from timeconst.pl:

Been there, done that.  My first patch (way back for 2.6.25) took that 
approach:

http://landley.net/hg/hgwebdir.cgi/firmware/file/a791ca629d9c/sources/patches/linux-2.6.25-
rc1-noperl.patch

But it turns out various non-x86 targets (such as ARM OMAP) allow HZ to be 
specified by an entry field in the config file, into which the user can type a 
range of numbers.  See this post from last year for details:

http://lists.impactlinux.com/pipermail/firmware-impactlinux.com/2008-
December/000022.html

This is why reducing the perl version to just the precomputed constants 
wouldn't work either.  (They're there so that you only need to install a 
random cpan library when surprised by a build break on non-x86 machines.)

> > +	echo
> > +	echo "#endif /* __KERNEL_TIMECHONST_H */"
>
>                                       ^
>
> Should be "__KERNEL_TIMECONST_H".

Yeah, Joe Perches pointed that out to me off-list.  It's just a comment so I 
didn't resubmit the patch for that, but I've fixed my local version already.

Rob
-- 
Latency is more important than throughput. It's that simple. - Linus Torvalds