Re: [PATCH 0/3] clean out Perl from build system.

Re: [PATCH 0/3] clean out Perl from build system.

[Michal Marek]

On 8.12.2009 10:17, Rob Landley wrote:
> Perl was introduced as a build dependency in 2.6.25.  My cross-compile 
> environment does not use perl, so ever since then I've patched it back out.
> 
> With these patches I've personally built kernels for x86, x86_64, arm, mips, 
> powerpc, sparc, sh4, and m68k.

Hi Rob,

you should CC linux-kbuild@ on patches like this (added now, the whole
series is at http://lkml.org/lkml/2009/12/8/93).

Michal

Re: [PATCH 0/3] clean out Perl from build system.

[Rob Landley]

On Tuesday 08 December 2009 04:23:06 Michal Marek wrote:
> On 8.12.2009 10:17, Rob Landley wrote:
> > Perl was introduced as a build dependency in 2.6.25.  My cross-compile
> > environment does not use perl, so ever since then I've patched it back
> > out.
> >
> > With these patches I've personally built kernels for x86, x86_64, arm,
> > mips, powerpc, sparc, sh4, and m68k.
>
> Hi Rob,
>
> you should CC linux-kbuild@ on patches like this (added now, the whole
> series is at http://lkml.org/lkml/2009/12/8/93).
>
> Michal

Thanks.  LWN said Sam Ravnborg was stepping down, and I haven't kept up with 
who's in charge now.

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

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

[Michal Marek]

[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:

with HZ=250:
--- 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        29
+#define HZ_TO_MSEC_SHR32       30
 #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)

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
 #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
 #define MSEC_TO_HZ_NUM          U64_C(1)
 #define MSEC_TO_HZ_DEN          U64_C(1)
 #define HZ_TO_USEC_MUL32        U64_C(0xfa000000)

$ bash --version
GNU bash, version 4.0.33(1)-release (x86_64-suse-linux-gnu)
...

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

#if HZ == 24
#define ...
...
#endif

#if HZ == 32
...
#endif
...
#ifndef HZ_TO_MSEC_MUL32
# error "Unknown HZ value, please update kernel/timeconst.h using
kernel/timeconst.pl"
#endif

plus some makefile automagic to run the script iff the HZ value isn't
precomputed. Then you would only need Perl for exotic HZ configurations.

> 
> Signed-off-by: Rob Landley <rob@landley.net>
> --
> 
> kernel/Makefile | 4
> kernel/timeconst.pl | 378 ------------------------------------------
> kernel/timeconst.sh | 91 ++++++++++
> 3 files changed, 93 insertions(+), 380 deletions(-)
> 
> diff -ruN linux-2.6.30/kernel/Makefile linux-2.6.30.new/kernel/Makefile
> --- linux-2.6.30/kernel/Makefile	2009-06-09 22:05:27.000000000 -0500
> +++ linux-2.6.30.new/kernel/Makefile	2009-06-22 14:29:16.000000000 -0500
> @@ -123,7 +123,7 @@
>  $(obj)/time.o: $(obj)/timeconst.h
>  
>  quiet_cmd_timeconst  = TIMEC   $@
> -      cmd_timeconst  = $(PERL) $< $(CONFIG_HZ) > $@
> +      cmd_timeconst  = $(CONFIG_SHELL) $< $(CONFIG_HZ) $@
>  targets += timeconst.h
> -$(obj)/timeconst.h: $(src)/timeconst.pl FORCE
> +$(obj)/timeconst.h: $(src)/timeconst.sh FORCE
>  	$(call if_changed,timeconst)
> diff -ruN linux-2.6.30/kernel/timeconst.pl linux-2.6.30.new/kernel/timeconst.pl
> --- linux-2.6.30/kernel/timeconst.pl	2009-06-09 22:05:27.000000000 -0500
> +++ linux-2.6.30.new/kernel/timeconst.pl	1969-12-31 18:00:00.000000000 -0600
> @@ -1,378 +0,0 @@
> -#!/usr/bin/perl
> -# -----------------------------------------------------------------------
> -#
> -#   Copyright 2007-2008 rPath, Inc. - All Rights Reserved
> -#
> -#   This file is part of the Linux kernel, and is made available under
> -#   the terms of the GNU General Public License version 2 or (at your
> -#   option) any later version; incorporated herein by reference.
> -#
> -# -----------------------------------------------------------------------
> -#
> -
> -#
> -# Usage: timeconst.pl HZ > timeconst.h
[snip]
> diff -ruN linux-2.6.30/kernel/timeconst.sh linux-2.6.30.new/kernel/timeconst.sh
> --- linux-2.6.30/kernel/timeconst.sh	1969-12-31 18:00:00.000000000 -0600
> +++ linux-2.6.30.new/kernel/timeconst.sh	2009-06-22 14:29:16.000000000 -0500
> @@ -0,0 +1,148 @@
> +#!/bin/sh
> +
> +if [ $# -ne 2 ]
> +then
> +	echo "Usage: timeconst.sh HZ FILENAME"
> +	echo
> +	echo "Generate a header file with constants for coverting between"
> +	echo "decimal HZ timer ticks and milisecond or microsecond delays."
> +	echo
> +	exit 1
> +fi
> +
> +HZ=$1
> +shift
> +FILENAME=$1
> +
> +# Sanity test: even the shell in Red Hat 9 (circa 2003) supported 64 bit math.
> +
> +if [ $((1 << 32)) -lt 0 ]
> +then
> +	echo "timeconst.sh needs a shell with 64 bit math, such as bash,"
> +	echo "busybox ash, or dash running on a 64 bit host."
> +	exit 1
> +fi
> +
> +# If this script exits for any reason before this trap is removed,
> +# delete the output file so a partial file won't confuse the build.
> +
> +trap "rm $FILENAME" EXIT
> +
> +# Output start of header file
> +
> +cat > $FILENAME << EOF || exit 1
> +/* Automatically generated by kernel/timeconst.sh */
> +/* Conversion constants for HZ == $HZ */
> +
> +#ifndef __KERNEL_TIMECONST_H
> +#define __KERNEL_TIMECONST_H
> +
> +#include <linux/param.h>
> +#include <linux/types.h>
> +
> +#if HZ != $HZ
> +#error "kernel/timeconst.h has the wrong HZ value!"
> +#endif
> +
> +EOF
> +
> +# For both Milliseconds and Microseconds
> +
> +cat << EOF |
> +MSEC 1000
> +USEC 1000000
> +EOF
> +while read NAME PERIOD
> +do
> +	# Find greatest common denominator (using Euclid's algorithm)
> +
> +	A=$HZ
> +	B=$PERIOD
> +
> +	while [ $B -ne 0 ]
> +	do
> +		C=$(( $A % $B ))
> +		A=$B
> +		B=$C
> +	done
> +
> +	GCD=$A
> +
> +	# Do this for each direction (HZ_TO_PERIOD and PERIOD_TO_HZ)
> +
> +	for DIRECTION in 0 1
> +	do
> +		if [ $DIRECTION -eq 0 ]
> +		then
> +			CONVERT="HZ_TO_${NAME}"
> +			FROM=$HZ
> +			TO=$PERIOD
> +		else
> +			CONVERT="${NAME}_TO_HZ"
> +			FROM=$PERIOD
> +			TO=$HZ
> +		fi
> +
> +		# Calculate 32 significant bits of MUL32 data.
> +
> +		SHIFT=0
> +		while true
> +		do
> +			# This can't overflow 64 bit math.  Pathological case
> +			# (TO=1, FROM=1000000) uses around 32+20=52 bits.
> +
> +			MUL32=$(( ( ( $TO << $SHIFT ) + $FROM - 1 ) / $FROM ))
> +
> +			# Keep increasing $SHIFT until we've got 32 bits.
> +
> +			[ $MUL32 -gt $(( 1 << 31 )) ] && break
> +			SHIFT=$(( $SHIFT + 1 ))
> +		done
> +		MUL32=$( printf %x $MUL32 )
> +
> +		# ADJ32 is just (((FROM/GCD)-1)<<SHIFT)/(FROM/GCD) but this
> +		# can overflow 64 bit math (examples, HZ=24 or HZ=122).
> +		# Pathological case could use 32+20+20=72 bits.  (And this is
> +		# the pathological case because a larger $HZ results in a
> +		# smaller $SHIFT, so even insane HZ>USEC cases should be ok.)
> +
> +		# To get around this, we chop the bottom 32 bits off the
> +		# calculation and then reassemble it to avoid overflow:
> +		# 32+64=96, which is > 72.
> +
> +		ADJ32=$(( $FROM / $GCD ))
> +		if [ $SHIFT -gt 32 ]
> +		then
> +			UPPER=$(( ( $ADJ32 - 1 ) << ( $SHIFT - 32 ) ))
> +			LOWER=$(( ( $UPPER % $ADJ32 ) << 32 ))
> +			ADJ32=$(( ( ( $UPPER / $ADJ32 ) << 32 ) + ( $LOWER / $ADJ32 )))
> +		else
> +			ADJ32=$(( ( ( $ADJ32 - 1 ) << $SHIFT) / $ADJ32 ))
> +		fi
> +		ADJ32=$( printf %x $ADJ32 )
> +
> +		NUM=$(( $TO / $GCD ))
> +		DEN=$(( $FROM / $GCD ))
> +
> +		# Output next chunk of header data to file
> +
> +		(
> +			echo "#define ${CONVERT}_MUL32	U64_C(0x$MUL32)" &&
> +			echo "#define ${CONVERT}_ADJ32	U64_C(0x$ADJ32)" &&
> +			echo "#define ${CONVERT}_SHR32	$SHIFT" &&
> +			echo "#define ${CONVERT}_NUM		U64_C($NUM)" &&
> +			echo "#define ${CONVERT}_DEN		U64_C($DEN)"
> +		) >> $FILENAME || exit 1
> +	done
> +done
> +
> +(
> +	echo
> +	echo "#endif /* __KERNEL_TIMECHONST_H */"
                                      ^

Should be "__KERNEL_TIMECONST_H".


> +) >> $FILENAME || exit 1
> +
> +# Don't rm $FILENAME on exit anymore.
> +
> +trap "" EXIT
> +
> +exit 0
> 

Michal

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

[Rob Landley]

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

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

[H. Peter Anvin]

On 12/09/2009 03:40 PM, Rob Landley wrote:
> 
> 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.)
> 

It's not "a random CPAN library" - it's a core module.

	-hpa

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

[Rob Landley]

On Wednesday 09 December 2009 17:45:21 H. Peter Anvin wrote:
> On 12/09/2009 03:40 PM, Rob Landley wrote:
> > 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.)
>
> It's not "a random CPAN library" - it's a core module.

Then why cache large quantities of its output and test for its existence?

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

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

[H. Peter Anvin]

On 12/09/2009 05:50 PM, Rob Landley wrote:
> On Wednesday 09 December 2009 17:45:21 H. Peter Anvin wrote:
>> On 12/09/2009 03:40 PM, Rob Landley wrote:
>>> 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.)
>>
>> It's not "a random CPAN library" - it's a core module.
> 
> Then why cache large quantities of its output and test for its existence?
> 

To support archaic Perl versions and Perl binaries without libraries.

	-hpa

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

[Michal Marek]

On 10.12.2009 00:40, Rob Landley wrote:
> On Wednesday 09 December 2009 09:45:37 Michal Marek wrote:
>> 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.)

That's why I wrote

>> plus some makefile automagic to run the script iff the HZ value isn't
>> precomputed. Then you would only need Perl for exotic HZ configurations.

E.g. make it
...
#elif HZ == 1200
...
#else
#include "timeconst_custom.h"
#endif

and the makefile would run timeconst.pl to generate timeconst_custom.h
iff HZ is set to something arbitrary. I don't have a patch for that, but
I don't see a fundamental problem with such approach.

Michal

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

[Rob Landley]

On Thursday 10 December 2009 07:52:44 Michal Marek wrote:
> On 10.12.2009 00:40, Rob Landley wrote:
> > On Wednesday 09 December 2009 09:45:37 Michal Marek wrote:
> >> 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/pat
> >ches/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.)
>
> That's why I wrote
>
> >> plus some makefile automagic to run the script iff the HZ value isn't
> >> precomputed. Then you would only need Perl for exotic HZ configurations.
>
> E.g. make it
> ...
> #elif HZ == 1200
> ...
> #else
> #include "timeconst_custom.h"
> #endif
>
> and the makefile would run timeconst.pl to generate timeconst_custom.h
> iff HZ is set to something arbitrary. I don't have a patch for that, but
> I don't see a fundamental problem with such approach.

I looked at that when I was attempting to update (rather than replace) my 
original patch.  There's a reason I didn't go there.  You're suggesting there 
be two code paths, one maintained by hand and the other just about never 
tested against bit-rot.  You want to add logic to the makefile so it knows when 
to run the perl script and when not to, meaning the makefile needs its own list 
of the cached HZ values, which needs to be kept in sync with the header of 
cached values, and thus the same information has to live in two places.

Meanwhile the shell script exists, works, and takes a fraction of a second to 
run.  Using it every build provides a single consistent code path which works 
the same for everybody.  I'm not seeing any advantage in cacheing values that 
are easy to calculate at compile time.  It's just as easy to completely 
eliminate the perl dependency as to merely mitigate it.

> Michal

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

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

[Michal Marek]

Rob Landley wrote:
> On Thursday 10 December 2009 07:52:44 Michal Marek wrote:
>> and the makefile would run timeconst.pl to generate timeconst_custom.h
>> iff HZ is set to something arbitrary. I don't have a patch for that, but
>> I don't see a fundamental problem with such approach.
> 
> I looked at that when I was attempting to update (rather than replace) my 
> original patch.  There's a reason I didn't go there.  You're suggesting there 
> be two code paths, one maintained by hand and the other just about never 
> tested against bit-rot.

OK, that's valid point, indeed. Peter, would you ack Rob's patch with
the oneline fix added (http://lkml.org/lkml/2009/12/8/94 plus
http://lkml.org/lkml/2009/12/9/435)?

Michal

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

[H. Peter Anvin]

On 12/11/2009 07:31 AM, Michal Marek wrote:
> 
> OK, that's valid point, indeed. Peter, would you ack Rob's patch with
> the oneline fix added (http://lkml.org/lkml/2009/12/8/94 plus
> http://lkml.org/lkml/2009/12/9/435)?
> 

I strongly dislike his patch, as he open-codes specific multiprecision
arithmetic.  This makes it hard for other people to maintain, and makes
it prone to errors -- as evidenced by the fact that it didn't even
replicate the known-good results.

I have made my position clear on this and other patches several times
before: I consider it a fool's errand, and a result of a completely
pointless crusade to make a particular science fair-type project a wee
bit easier.  We have already seen real damage caused by it, since people
have used awk instead, and have gotten bitten by incompatibilities
between awk implementations.

As such, no, I will not ack this patch, and will consider myself
released of any obligation to maintain the code if this goes in anyway.
 I would consider acking a C program which does proper multiprecision
arithmetic, but I'm also not going to spend my time on it.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

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

[Rob Landley]

On Friday 11 December 2009 13:13:39 H. Peter Anvin wrote:
> On 12/11/2009 07:31 AM, Michal Marek wrote:
> > OK, that's valid point, indeed. Peter, would you ack Rob's patch with
> > the oneline fix added (http://lkml.org/lkml/2009/12/8/94 plus
> > http://lkml.org/lkml/2009/12/9/435)?
>
> I strongly dislike his patch, as he open-codes specific multiprecision
> arithmetic.  This makes it hard for other people to maintain, and makes
> it prone to errors

It's always easier to understand code that you wrote, doubly so with perl.  
The perl code this is replacing has comments, function names like fmul() and 
fmuls() with no way to distinguish what they do except by reading their 
implementation, and half of it is devoting to cacheing the output of the other 
half for reasons which are never explained in the code.

The few comments the perl code does have are sometimes actually misleading.  
For example, bignum_hex() has the comment "generate a hex value if the result 
fits in 64 bits; otherwise skip".  What would skpping mean in this context?  
Would it silently emit a broken header, or would it break the build?  (My first 
guess is that "undef" without an expr would be a build break, but maybe this 
is perl's way of saying "None"?  I do know that whatever happened the result 
wouldn't be a usable kernel, so "skip" is disingenous.)

That said, I read through and managed to understand the perl code.  But it 
took me more than a day, and I was motivated by the desire to replace it.

> -- as evidenced by the fact that it didn't even
> replicate the known-good results.

Actually I had noticed the difference back at the time I wrote the script, but 
the value plus shift were functionally equivalent, and at the time (a year 
ago) I vaguely recall working through the math (and looking at the kernel code 
using the output) convinced me that that integer overflow couldn't actually 
happen for real inputs.  But I don't remember why (perhaps the value to be 
converted is range checked somewhere so ffffffff couldn't actually make it to this 
code, I don't remember) and thus my version would give more precision to the 
result (which seemed to be the whole point of the exercise).

No point in trying to reconstruct my reasoning from a year ago when it's a one 
line change to make it non-controversial, and the "value always fits in 32 bits 
so you can safely multiply by an unsigned int due to LP64" is easier to 
explain/document anyway.  (On the other end of things, I do remember that the 
admittedly insane HZ of ffffffff would have a GCD of 5 with 1000 and thus not be 
saved as that.  That came up because I was worried an ffffffff value times ffffffff 
might _round_ up to overflow when adj32 was added.  I did attempt to document 
some of the edge cases in the shell script comments...)

> I have made my position clear on this and other patches several times
> before: I consider it a fool's errand,

I.E. your fundamental objection is to removing perl form the build.

You are the one who added this perl to the build system.  You're also the one 
who added perl to all the other projects you maintain (klibc and syslinux I 
noticed) at approximately the same time, after years of those not requiring 
perl.  I don't know why you suddenly decided perl needed to be everywhere, but 
the Linux kernel is not the only project you simultaneously applied this 
philosophy to.

I strongly suspect the other objections you've raised in the past two years 
are a proxy for the objection to removing perl.  However, your central 
objection to removing perl hasn't convinced people like Alan Cox, who called 
Perl "a bad candidate for our toolchain dependencies" here:

  http://lkml.indiana.edu/hypermail/linux/kernel/0901.1/02108.html

Thus you raise other issues, which seem to be proxies for that fundamental 
objection.  I've tried to address the ones that look like they were actually 
intended to be addressed, but I note that your current objection is to the 
something like the seventh submission of this patch, and I believe this is the 
first time you've raised this particular issue.  I believe I first submitted an 
ancestor of this patch on February 15, 2008.  Just this year I've submitted 
previous versions of this patch (more or less in its current form) on January 
2, 2009 (with a resubmit on January 3 in response to feedback), on June 22, 
and on September 18.  And again now.  The patch hasn't changed much, your 
objections to it have.

I don't even remember you previously complaining about the output differing.  
(At a guess, you never actually tried my patch?)  But it has been a while, I 
could easily be forgetting details by now.

I'm posting these here because they're useful to other people, not because I 
expect you to ever stop objecting to them no matter what changes I make.  
You've made it clear you object to the _goal_ of the patch.  The 
implementation seems to be a side issue.

> and a result of a completely
> pointless crusade to make a particular science fair-type project a wee
> bit easier.

Embedded developers' desire to restrict their build environment to something 
controllable seems to strike you as weird, yes.  Would it help to explain that 
it's the same general theory behind Linux From Scratch making a temporary 
system, chrooting into it, and then building in there in Chapter 5?

Leaking random crap from the host is bad, often in subtle ways you don't 
immediately catch.  Some programs (perl, libtool, syscfg) are champions at 
leaking context, and to be avoided if at all possible.  (I suppose you could 
always suggest setting up a different build environment for the kernel as for 
userspace, the way Red Hat used to do with kgcc.  Doesn't strike me as a good 
idea, though.)

> We have already seen real damage caused by it, since people
> have used awk instead, and have gotten bitten by incompatibilities
> between awk implementations.

I'm not using AWK, and I'm sticking with POSIX shell constructs that were 
there in susv3, let alone susv4.

Since I wasn't the one proposing any patches extensively using awk (I need the 
man page open to make awk do anything more complicated than '{print $2}'), and 
this patch isn't using awk at all, I guess you're once again objecting to the 
removal of perl in general, not to the patch in front of you.  And in doing 
so, you're providing evidence that I'm not the only one motivated to submit 
perl removal patches, which might undermine your "particular science fair 
project" argument a bit.

I am motivated to follow up on this.  I've been maintaining perl removal 
patches for a couple years now because I personally need them, but I'm not the 
only one using these patches, and I've been thanked for tackling this problem 
by a dozen embedded developers.  I wasn't the one who first noticed the perl 
dependency when it went in back in .25, David Anders notified me of the issue 
before I'd tested the -rc that introduced it.  The most recent guy to say, and 
I quote, "Thanks for eliminating the perl dependency." to me in private email 
was Joe Perches (on Tuesday).

Embedded developers tend to do stuff off-list.  This is because every new kernel 
breaks random stuff on non-x86 targets.  For example, I finally got powerpc's 
pmac_zilog fixed a few days ago (allowing me to use the serial console on 
qemu's mac99 target), after a thread stretching back two months (it broke 
after 2.6.28).  The "works for me" fix is here:

  http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-December/078700.html

The most recent arm breakage turned out to be simple (a new CONFIG_MMU symbol 
showed up between 2.6.31 and 2.6.32 and needed to be switched on), but 
diagnosing "it doesn't boot, no console messages" is always a pain.

The most recent mips breakage was fixed by commit 
d71789b6fa37c21ce5eb588d279f57904a62e7e2 which I asked to be backported to 
2.6.31 in the stable series and Greg KH acknowledged on November 5th.  (I'd 
link to my submission to stable@kernel.org but there's no entry for that in 
vger lists and googling doesn't seem to come up with a public archive.)

That's just in the past month or so.  Trying to git bisect each of those 
problems found huge ranges of "it doesn't even compile" in the repository, 
because that's the _norm_ for non-x86 targets during development.

Most embedded developers don't want to deal with this, so they stay a year or 
three behind the bleeding edge and only upgrade after crazy people like me 
have gotten it working.  Since even the ones who _don't_ start their messages 
by apologizing about their English know they'll be flamed or ignored if they 
raise an issue about an 18 month old kernel here, they develop a fear of 
posting on linux-kernel at all because it's an "unfriendly" place.  (I try to 
talk them out of it, but it's a cultural thing at this point.  The new kernel 
embedded list hasn't really helped more than 10% of this problem.)

I regularly tackle these issues anyway, am generally friendly to newbies on 
the lists I follow, and apparently have no shame, so people sometimes forward 
issues to me and hope I'll push them upstream so they don't have to.  (And 
then go away again once they've managed to upgrade to whatever snapshot 
they'll be using for the next 3 years, leaving me to follow up.)

*shrug*  I'm used to it.  You feed stray issues patches and they wind up 
adopting you...

> As such, no, I will not ack this patch, and will consider myself
> released of any obligation to maintain the code if this goes in anyway.

I've been maintaining this patch series for 2 years already.  I would be happy 
to maintain my changed version in future if it gets in.  I expected that.

>  I would consider acking a C program which does proper multiprecision
> arithmetic, but I'm also not going to spend my time on it.

Hmmm, "would consider acking" (not "would ack", not even "could ack"), with 
the additional caveat that the multiprecision arithmetic must be an unspecified 
"proper", whatever that means.  The "proxy objections" thesis stated above 
would label this an obvious delaying tactic, but if I promise to call your 
bluff, then the current patch gets dropped on the floor for another development 
cycle...

Well played, sir.  I must plead the 5th for now.

> 	-hpa

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