[PATCH] NIPQUAD macro broken on big endian
Andreas Schwab
number in network byte order.
--- linux/include/linux/kernel.h.~1~ Wed Oct 7 11:06:36 1998
+++ linux/include/linux/kernel.h Wed Oct 7 11:08:43 1998
@@ -69,10 +69,10 @@
*/
#define NIPQUAD(addr) \
- (int)(((addr) >> 0) & 0xff), \
- (int)(((addr) >> 8) & 0xff), \
- (int)(((addr) >> 16) & 0xff), \
- (int)(((addr) >> 24) & 0xff)
+ (int)((ntohl(addr) >> 24) & 0xff), \
+ (int)((ntohl(addr) >> 16) & 0xff), \
+ (int)((ntohl(addr) >> 8) & 0xff), \
+ (int)((ntohl(addr) >> 0) & 0xff)
#endif /* __KERNEL__ */
--
Andreas Schwab "And now for something
sch...@issan.cs.uni-dortmund.de completely different"
sch...@gnu.org
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/
David S. Miller
Date: 07 Oct 1998 11:09:08 +0200
The NIPQUAD makro is broken on big endian machines. The argument is a
number in network byte order.
Now it is slow as shit on little endian, what is your point with this
patch?
Linus isn't going to accept this, and I know why. Why not define it
conditionally based upon endianness, changing the shifts and masks to
match, instead of unnecessarily bloating things with ntohl()
etc. calls in one case for the sake of correctness on another?
Later,
David S. Miller
da...@dm.cobaltmicro.com
Andreas Schwab
|> From: Andreas Schwab <sch...@issan.informatik.uni-dortmund.de>
|> Date: 07 Oct 1998 11:09:08 +0200
|>
|> The NIPQUAD makro is broken on big endian machines. The argument is a
|> number in network byte order.
|>
|> Now it is slow as shit on little endian, what is your point with this
|> patch?
My point is to make it work, nothing more and nothing less. Of course, it
doesn't *have* to be slow on little endian if the ntohl macro is properly
written so that gcc can combine the shifts inside and outside of it.
|> Linus isn't going to accept this, and I know why. Why not define it
|> conditionally based upon endianness, changing the shifts and masks to
|> match, instead of unnecessarily bloating things with ntohl()
|> etc. calls in one case for the sake of correctness on another?
The use of ntohl makes things clear that NIPQUAD expects its argument in
network byteorder. The correct fix is of course to switch to a cpu with
the One True Byteorder. :-)) Ok, for those who cannot do that, here's an
improved patch:
--- linux/include/linux/kernel.h.~1~ Wed Oct 7 11:06:36 1998
+++ linux/include/linux/kernel.h Wed Oct 7 12:44:56 1998
@@ -10,6 +10,8 @@
#include <stdarg.h>
#include <linux/linkage.h>
+#include <asm/byteorder.h>
+
/* Optimization barrier */
#define barrier() __asm__("": : :"memory")
@@ -68,11 +70,19 @@
* Display an IP address in readable format.
*/
+#ifdef __LITTLE_ENDIAN
#define NIPQUAD(addr) \
(int)(((addr) >> 0) & 0xff), \
(int)(((addr) >> 8) & 0xff), \
(int)(((addr) >> 16) & 0xff), \
(int)(((addr) >> 24) & 0xff)
+#else
+#define NIPQUAD(addr) \
+ (int)(((addr) >> 24) & 0xff), \
+ (int)(((addr) >> 16) & 0xff), \
+ (int)(((addr) >> 8) & 0xff), \
+ (int)(((addr) >> 0) & 0xff)
+#endif
#endif /* __KERNEL__ */
--
Andreas Schwab "And now for something
sch...@issan.cs.uni-dortmund.de completely different"
sch...@gnu.org
-
David S. Miller
Date: 07 Oct 1998 12:48:00 +0200
"David S. Miller" <da...@dm.cobaltmicro.com> writes:
|> Now it is slow as shit on little endian, what is your point with this
|> patch?
My point is to make it work, nothing more and nothing less. Of course, it
doesn't *have* to be slow on little endian if the ntohl macro is properly
written so that gcc can combine the shifts inside and outside of it.
I know this, and I sent your original patch to Linus long ago, and I
am telling you his response and his reasoning. At least I gave you a
suggestion for an implementation which would have a greater chance at
accpetance :-)
Later,
David S. Miller
da...@dm.cobaltmicro.com
-
Geert Uytterhoeven
> From: Andreas Schwab <sch...@issan.informatik.uni-dortmund.de>
> Date: 07 Oct 1998 12:48:00 +0200
>
> "David S. Miller" <da...@dm.cobaltmicro.com> writes:
>
> |> Now it is slow as shit on little endian, what is your point with this
> |> patch?
>
> My point is to make it work, nothing more and nothing less. Of course, it
> doesn't *have* to be slow on little endian if the ntohl macro is properly
> written so that gcc can combine the shifts inside and outside of it.
>
> I know this, and I sent your original patch to Linus long ago, and I
> am telling you his response and his reasoning. At least I gave you a
> suggestion for an implementation which would have a greater chance at
> accpetance :-)
Do I read this correct?!? Is it more acceptable to have a bug[*], than a not
optimal fix?!?!?
Greetings,
Geert
[*] OK, only on `exotic' platforms ;-)
--
Geert Uytterhoeven Geert.Uyt...@cs.kuleuven.ac.be
Wavelets, Linux/{m68k~Amiga,PPC~CHRP} http://www.cs.kuleuven.ac.be/~geert/
Department of Computer Science -- Katholieke Universiteit Leuven -- Belgium
David S. Miller
From: Geert Uytterhoeven <Geert.Uyt...@cs.kuleuven.ac.be>
Do I read this correct?!? Is it more acceptable to have a bug[*],
than a not optimal fix?!?!?
s/optimal/stupid/
Later,
David S. Miller
da...@dm.cobaltmicro.com
-
Andreas Schwab
|> Date: Wed, 7 Oct 1998 13:15:07 +0200 (CEST)
|> From: Geert Uytterhoeven <Geert.Uyt...@cs.kuleuven.ac.be>
|>
|> Do I read this correct?!? Is it more acceptable to have a bug[*],
|> than a not optimal fix?!?!?
|>
|> s/optimal/stupid/
What's stupid is little endian. <g>
Andreas.
--
Andreas Schwab "And now for something
sch...@issan.cs.uni-dortmund.de completely different"
sch...@gnu.org
-
Matthew Kirkwood
> Date: Wed, 7 Oct 1998 13:15:07 +0200 (CEST)
> From: Geert Uytterhoeven <Geert.Uyt...@cs.kuleuven.ac.be>
>
> Do I read this correct?!? Is it more acceptable to have a bug[*],
> than a not optimal fix?!?!?
>
> s/optimal/stupid/
s/not stupid/stupid/
?
Matthew.
Linus Torvalds
On 7 Oct 1998, Andreas Schwab wrote:
>
> The NIPQUAD makro is broken on big endian machines. The argument is a
> number in network byte order.
Stop sending me this patch. I've rejected it before, for good reason.
Remember: I don't reject patches just because I drop them. I often reject
patches because they are obviously bad. This is one.
If you want to send me a patch to clean it up, make sure that it doesn't
generate ridiculously more code on the 99% of Linux machines that have a
sane byte order.
Solutions I can accept:
- move it into byteorder.h and make it dependent on the byte order
- use "cpu_to_le32()" instead of ntohl() so that the common case doesn't
have extra overhead.
Linus
Linus Torvalds
On 7 Oct 1998, Andreas Schwab wrote:
>
> My point is to make it work, nothing more and nothing less. Of course, it
> doesn't *have* to be slow on little endian if the ntohl macro is properly
> written so that gcc can combine the shifts inside and outside of it.
Umm:
- it's not so much slowness as code expansion (NIPQUAD is only used by
various printk's, so performance per se is secondary)
- a well-written htonl() macro will not do just the stupid expansion of
shifts, it will use inline assembly to generate a "bswap" instruction.
- gcc isn't that clever.
Linus Torvalds
On Wed, 7 Oct 1998, Geert Uytterhoeven wrote:
>
> Do I read this correct?!? Is it more acceptable to have a bug[*], than a not
> optimal fix?!?!?
Yes.
- the bug is purely aesthetic
- the bug only happens on 68k and sparc, ie <1% fo the installed base
Finally, even if the above isn't true, I often reject bug-fixes.
Bug-fixes are _often_ worse than the bug they fix, even with serious bugs.
Because a lot of bug-fixes are "band-aid" - not fixing the bug properly.
And band-aid is BAD. It's worse than even a crashing machine. Because
band-aid never goes away, and nobody cleans it up.
I prefer to have a known bug that will eventually get fixed than an ugly
solution that will hide it forever.
Geert, go away. You don't seem to understand what being a maintainer
means. It means saying no to crap.
Roderich Schupp
Linus Torvalds <torv...@transmeta.com> said:
> - it's not so much slowness as code expansion (NIPQUAD is only used
> by various printk's, so performance per se is secondary)
So why not teach the kernel's vsprintf a new format code and
get rid of this hack? (Wasn't such a format in the kernel at one time?)
Just my $0.02, Roderich
--
"We are ecologically minded. This package will self-destruct
in Mother Earth." -- label on a Japanese eraser
Roderich Schupp mailto:rs...@ExperTeam.de
ExperTeam GmbH http://www.experteam.de/
Munich, Germany linux:2.1.125
Jakub Jelinek
> > - it's not so much slowness as code expansion (NIPQUAD is only used
> > by various printk's, so performance per se is secondary)
>
> So why not teach the kernel's vsprintf a new format code and
> get rid of this hack? (Wasn't such a format in the kernel at one time?)
Because there is no way at the moment how to teach gcc about the new format
code, and as we use compiler warnings for formatting, you'd get a bunch of
warnings all the time.
Cheers,
Jakub
___________________________________________________________________
Jakub Jelinek | j...@sunsite.mff.cuni.cz | http://sunsite.mff.cuni.cz
Administrator of SunSITE Czech Republic, MFF, Charles University
___________________________________________________________________
Ultralinux - first 64bit OS to take full power of the UltraSparc
Linux version 2.1.125 on a sparc64 machine (498.80 BogoMips).
___________________________________________________________________
Linus Torvalds
Roderich Schupp <rs...@ExperTeam.de> wrote:
>Linus Torvalds <torv...@transmeta.com> said:
>> - it's not so much slowness as code expansion (NIPQUAD is only used
>> by various printk's, so performance per se is secondary)
>
>So why not teach the kernel's vsprintf a new format code and
>get rid of this hack? (Wasn't such a format in the kernel at one time?)
There was.
I got rid of it, because it's unusable. It means that you can't have gcc
warn about printf() misuses, and they are a lot more important to me
than adding a random format code.
Linus
Andreas Schwab
|> On 7 Oct 1998, Andreas Schwab wrote:
|> >
|> > The NIPQUAD makro is broken on big endian machines. The argument is a
|> > number in network byte order.
|>
|> Stop sending me this patch. I've rejected it before, for good reason.
This is the first time i have sent you this patch. How can you have
already rejected it???
Andreas.
--
Andreas Schwab "And now for something
sch...@issan.cs.uni-dortmund.de completely different"
sch...@gnu.org
-
David S. Miller
Date: 09 Oct 1998 10:47:37 +0200
Linus Torvalds <torv...@transmeta.com> writes:
|> Stop sending me this patch. I've rejected it before, for good reason.
This is the first time i have sent you this patch. How can you have
already rejected it???
I had sent it to him months ago silly.
Later,
David S. Miller
da...@dm.cobaltmicro.com
-
Linus Torvalds
On 9 Oct 1998, Andreas Schwab wrote:
> Linus Torvalds <torv...@transmeta.com> writes:
>
> |> On 7 Oct 1998, Andreas Schwab wrote:
> |> >
> |> > The NIPQUAD makro is broken on big endian machines. The argument is a
> |> > number in network byte order.
> |>
> |> Stop sending me this patch. I've rejected it before, for good reason.
>
> This is the first time i have sent you this patch. How can you have
> already rejected it???
Because of the f*cking vger braindamage.
People have sent me patches from vger, and when I tell them that they are
broken, nobody fixes vger.
Doesn't anybody see what's wrong here?
Linus
David S. Miller
From: Linus Torvalds <torv...@transmeta.com>
Because of the f*cking vger braindamage.
People have sent me patches from vger, and when I tell them that they are
broken, nobody fixes vger.
Doesn't anybody see what's wrong here?
Correction, I yanked that patch from vger precisely the moment you
told me you hated it. Andreas is just sending it again on his own,
independantly of vger, this patch does not exist in vger and hasn't
since you barfed at the sight of it.
Later,
David S. Miller
da...@dm.cobaltmicro.com
-
Jes Sorensen
David> Date: Fri, 9 Oct 1998 11:36:40 -0700 (PDT) From: Linus
David> Torvalds <torv...@transmeta.com>
> Because of the f*cking vger braindamage.
>
> People have sent me patches from vger, and when I tell them
> that they are broken, nobody fixes vger.
>
> Doesn't anybody see what's wrong here?
David> Correction, I yanked that patch from vger precisely the moment
David> you told me you hated it. Andreas is just sending it again on
David> his own, independantly of vger, this patch does not exist in
David> vger and hasn't since you barfed at the sight of it.
I believe the reason for all this is that _I_ kept the patch in the
m68k tree because it does solve a bug for us. I thought Andreas was
copied when mail was bounced around about this when you took it out
from the vger tree, seems that I was wrong.
I should have made a comment in the code saying something like 'don't
send this to Linus before someone comes up with a better patch'.
Jes
Linus Torvalds
On Fri, 9 Oct 1998, David S. Miller wrote:
>
> Correction, I yanked that patch from vger precisely the moment you
> told me you hated it. Andreas is just sending it again on his own,
> independantly of vger, this patch does not exist in vger and hasn't
> since you barfed at the sight of it.
Yes, I should have thought about it a bit more, and it was obvious that it
must have been yanked from vger itself. So apologies to vger.
Linus