LinuxLists.cc - [PATCH] ubi: kill homegrown endian macros

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

Christoph,

On Thu, 2007-05-17 at 16:32 +0200, Christoph Hellwig wrote:
> Kill ubis homegrown endianess handling crap and replace it with
> the normal kernel endianess handling.

Err,__be32 and the company are just sparse things, while I have compiler
checks with my struct ubi32_t and friends. JFFS2 also uses the same
technique. Why do you force me to rely on sparse instead instead of
compiler?

Well, I see the good side of your change - no home-brewed media<->cpu
things. Fair enough and nice. But why don't you make __be32 a struct
(just like I do) so that compiler could complain then?

--
Best regards,
Artem Bityutskiy (Битюцкий Артём)

2007-05-17 14:57:44

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, May 17, 2007 at 05:50:43PM +0300, Artem Bityutskiy wrote:
> Christoph,
>
> On Thu, 2007-05-17 at 16:32 +0200, Christoph Hellwig wrote:
> > Kill ubis homegrown endianess handling crap and replace it with
> > the normal kernel endianess handling.
>
> Err,__be32 and the company are just sparse things, while I have compiler
> checks with my struct ubi32_t and friends. JFFS2 also uses the same
> technique. Why do you force me to rely on sparse instead instead of
> compiler?

Yes. Like all other code in the kernel aswell.

2007-05-17 15:10:44

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, 2007-05-17 at 16:56 +0200, Christoph Hellwig wrote:
> On Thu, May 17, 2007 at 05:50:43PM +0300, Artem Bityutskiy wrote:
> > Christoph,
> >
> > On Thu, 2007-05-17 at 16:32 +0200, Christoph Hellwig wrote:
> > > Kill ubis homegrown endianess handling crap and replace it with
> > > the normal kernel endianess handling.
> >
> > Err,__be32 and the company are just sparse things, while I have compiler
> > checks with my struct ubi32_t and friends. JFFS2 also uses the same
> > technique. Why do you force me to rely on sparse instead instead of
> > compiler?
>
> Yes. Like all other code in the kernel aswell.

Andrew, may I please have your ack that I absolutely have to use __be32
instead of my own types since Christoph tends to provide no explanation
to his requests.

--
Best regards,
Artem Bityutskiy (Битюцкий Артём)

2007-05-17 15:51:16

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, 2007-05-17 at 16:32 +0200, Christoph Hellwig wrote:
> Kill ubis homegrown endianess handling crap and replace it with
> the normal kernel endianess handling.

NAK. The 'normal' kernel stuff doesn't work in GCC; only in sparse.

If you want to go removing the ones which _do_ work in GCC, then fix the
'normal' one first. _Then_ go about converting others over to it.

--
dwmw2

2007-05-17 17:34:20

by Andrew Morton

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, 17 May 2007 18:09:50 +0300 Artem Bityutskiy <[email protected]> wrote:

> On Thu, 2007-05-17 at 16:56 +0200, Christoph Hellwig wrote:
> > On Thu, May 17, 2007 at 05:50:43PM +0300, Artem Bityutskiy wrote:
> > > Christoph,
> > >
> > > On Thu, 2007-05-17 at 16:32 +0200, Christoph Hellwig wrote:
> > > > Kill ubis homegrown endianess handling crap and replace it with
> > > > the normal kernel endianess handling.
> > >
> > > Err,__be32 and the company are just sparse things, while I have compiler
> > > checks with my struct ubi32_t and friends. JFFS2 also uses the same
> > > technique. Why do you force me to rely on sparse instead instead of
> > > compiler?
> >
> > Yes. Like all other code in the kernel aswell.
>
> Andrew, may I please have your ack that I absolutely have to use __be32
> instead of my own types since Christoph tends to provide no explanation
> to his requests.

umm.. I'd say what you've done in there is an improvement to the exiting
stuff: getting gcc to check it is better than having to use sparse.

I'd have expected gcc to generate poorer code with your approach but I'm
showing zero text size changes from Christoph's patch (gcc-4.1 and
gcc-3.4.5).

So I wouldn't be averse to creating a new, generic, kernel-wide alternative
to the existing __be32/__le32/etc code. It is an improvement.

But I don't think we want driver-private implementations.

We could conceivably simply switch the existing stuff to use structs, but
quite a lot of code assumes that cpu_to_foo32(0) == 0 and just does
open-coded assigments of zero. They'd need fixups.

2007-05-17 17:54:05

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, 2007-05-17 at 10:29 -0700, Andrew Morton wrote:
> umm.. I'd say what you've done in there is an improvement to the exiting
> stuff: getting gcc to check it is better than having to use sparse.
>
> I'd have expected gcc to generate poorer code with your approach but I'm
> showing zero text size changes from Christoph's patch (gcc-4.1 and
> gcc-3.4.5).
>
> So I wouldn't be averse to creating a new, generic, kernel-wide alternative
> to the existing __be32/__le32/etc code. It is an improvement.
>
> We could conceivably simply switch the existing stuff to use structs, but
> quite a lot of code assumes that cpu_to_foo32(0) == 0 and just does
> open-coded assigments of zero. They'd need fixups.

Andrew,

thanks for answer. I personally do not think this should be applied
before we have better __be32 and friends. Thus, if I can do so, I will
just sit and wait for your decision - whether you include this patch to
-mm or not :-) .

--
Best regards,
Artem Bityutskiy (Битюцкий Артём)

2007-05-17 18:13:20

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, May 17, 2007 at 11:50:54PM +0800, David Woodhouse wrote:
> On Thu, 2007-05-17 at 16:32 +0200, Christoph Hellwig wrote:
> > Kill ubis homegrown endianess handling crap and replace it with
> > the normal kernel endianess handling.
>
> NAK. The 'normal' kernel stuff doesn't work in GCC; only in sparse.
>
> If you want to go removing the ones which _do_ work in GCC, then fix the
> 'normal' one first. _Then_ go about converting others over to it.

Nope. gcc checking is crap and we need to run sparse anyway. It's what
everyone in the kernel use so it's perfect for some random code almost
no one uses aswell.

2007-05-17 18:20:50

by Andrew Morton

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, 17 May 2007 20:46:46 +0300
Artem Bityutskiy <[email protected]> wrote:

> On Thu, 2007-05-17 at 10:29 -0700, Andrew Morton wrote:
> > umm.. I'd say what you've done in there is an improvement to the exiting
> > stuff: getting gcc to check it is better than having to use sparse.
> >
> > I'd have expected gcc to generate poorer code with your approach but I'm
> > showing zero text size changes from Christoph's patch (gcc-4.1 and
> > gcc-3.4.5).
> >
> > So I wouldn't be averse to creating a new, generic, kernel-wide alternative
> > to the existing __be32/__le32/etc code. It is an improvement.
> >
> > We could conceivably simply switch the existing stuff to use structs, but
> > quite a lot of code assumes that cpu_to_foo32(0) == 0 and just does
> > open-coded assigments of zero. They'd need fixups.
>
> Andrew,
>
> thanks for answer. I personally do not think this should be applied
> before we have better __be32 and friends. Thus, if I can do so, I will
> just sit and wait for your decision - whether you include this patch to
> -mm or not :-) .
>

Drat, and here was I hoping I'd lured you into implementing the generic
code.

2007-05-17 18:22:25

by Sam Ravnborg

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, May 17, 2007 at 08:12:23PM +0200, Christoph Hellwig wrote:
> On Thu, May 17, 2007 at 11:50:54PM +0800, David Woodhouse wrote:
> > On Thu, 2007-05-17 at 16:32 +0200, Christoph Hellwig wrote:
> > > Kill ubis homegrown endianess handling crap and replace it with
> > > the normal kernel endianess handling.
> >
> > NAK. The 'normal' kernel stuff doesn't work in GCC; only in sparse.
> >
> > If you want to go removing the ones which _do_ work in GCC, then fix the
> > 'normal' one first. _Then_ go about converting others over to it.
>
> Nope. gcc checking is crap and we need to run sparse anyway. It's what
> everyone in the kernel use so it's perfect for some random code almost
> no one uses aswell.

Care to explain what is wrong with the gcc checks as used by UBI?
Would be nice to know before someone start to migrate current
stuff so gcc can check for correct endian.

Sam

2007-05-17 18:36:37

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, May 17, 2007 at 08:23:14PM +0200, Sam Ravnborg wrote:
> On Thu, May 17, 2007 at 08:12:23PM +0200, Christoph Hellwig wrote:
> > On Thu, May 17, 2007 at 11:50:54PM +0800, David Woodhouse wrote:
> > > On Thu, 2007-05-17 at 16:32 +0200, Christoph Hellwig wrote:
> > > > Kill ubis homegrown endianess handling crap and replace it with
> > > > the normal kernel endianess handling.
> > >
> > > NAK. The 'normal' kernel stuff doesn't work in GCC; only in sparse.
> > >
> > > If you want to go removing the ones which _do_ work in GCC, then fix the
> > > 'normal' one first. _Then_ go about converting others over to it.
> >
> > Nope. gcc checking is crap and we need to run sparse anyway. It's what
> > everyone in the kernel use so it's perfect for some random code almost
> > no one uses aswell.
>
> Care to explain what is wrong with the gcc checks as used by UBI?
> Would be nice to know before someone start to migrate current
> stuff so gcc can check for correct endian.

The major wrong thing is that it makes a little subsysttem artifically
different from the rest of the kernel.

At a technical level it overdoing things. The attribute syntax is perfecltly
find to address endianess issues without introducing wrapper structs the
lead to horrible code generation in some situations (e.g. trying to return
such a value)

2007-05-17 20:19:03

by Jeremy Fitzhardinge

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

Christoph Hellwig wrote:
> The attribute syntax is perfecltly
> find to address endianess issues without introducing wrapper structs the
> lead to horrible code generation in some situations (e.g. trying to return
> such a value)

Small structs are returned in register; it is indistinguishable from
returning a scalar.

J

2007-05-17 20:27:45

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, 17 May 2007 16:32:01 +0200, Christoph Hellwig wrote:

> Kill ubis homegrown endianess handling crap and replace it with the
> normal kernel endianess handling.
>
Hum, you should check about alignment stuff.
Jffs2 use a similar mechanism and the packed struct also take care of
some unaligned data (present in summary node for example).

Matthieu

2007-05-17 20:32:36

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, 17 May 2007, Andrew Morton wrote:
> Drat, and here was I hoping I'd lured you into implementing the generic
> code.

Well, I am really happy to contribute, but I am not a generic janitor like
Cristoph, who (amaizingly) knows many differet kernel subsystems. I am
a specific developer and I would rather concentrate on the Flash file
system I am writing, and this will be my later contribution.

Artem.

2007-05-17 20:35:25

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, 17 May 2007 10:29:31 -0700, Andrew Morton wrote:

> On Thu, 17 May 2007 18:09:50 +0300 Artem Bityutskiy
> <[email protected]> wrote:
>
> umm.. I'd say what you've done in there is an improvement to the
> exiting stuff: getting gcc to check it is better than having to use
> sparse.
>
> I'd have expected gcc to generate poorer code with your approach but I'm
> showing zero text size changes from Christoph's patch (gcc-4.1 and
> gcc-3.4.5).
>
>
On which arch did you try ?
X86 where unaligned access are ok ?

On arch that don't support aligned access, packed struct access will be
done byte per byte (but it could be the expected behavior if there
unaligned access).

Matthieu

2007-05-17 20:42:51

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, May 17, 2007 at 10:29:31AM -0700, Andrew Morton wrote:
> On Thu, 17 May 2007 18:09:50 +0300 Artem Bityutskiy <[email protected]> wrote:
>
> > On Thu, 2007-05-17 at 16:56 +0200, Christoph Hellwig wrote:
> > > On Thu, May 17, 2007 at 05:50:43PM +0300, Artem Bityutskiy wrote:
> > > > Christoph,
> > > >
> > > > On Thu, 2007-05-17 at 16:32 +0200, Christoph Hellwig wrote:
> > > > > Kill ubis homegrown endianess handling crap and replace it with
> > > > > the normal kernel endianess handling.
> > > >
> > > > Err,__be32 and the company are just sparse things, while I have compiler
> > > > checks with my struct ubi32_t and friends. JFFS2 also uses the same
> > > > technique. Why do you force me to rely on sparse instead instead of
> > > > compiler?
> > >
> > > Yes. Like all other code in the kernel aswell.
> >
> > Andrew, may I please have your ack that I absolutely have to use __be32
> > instead of my own types since Christoph tends to provide no explanation
> > to his requests.
>
> umm.. I'd say what you've done in there is an improvement to the exiting
> stuff: getting gcc to check it is better than having to use sparse.

Ahem... So what does
x |= y;
turns into with that approach?

2007-05-17 20:53:51

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

> Ahem... So what does
> x |= y;
> turns into with that approach?

BTW, you can simply typedef __be16 ubi16_t; etc. and define conversion
functions as cpu_to_ubi16(x) being (__force ubi16_t)cpu_to_be16(x), etc.

sparse will do all checks just fine, you still have bitwise operations
(might or might be not relevant in your case) and for gcc it simply
becomes __be16, etc - i.e. an integer type.

2007-05-17 20:56:31

by David Miller

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

From: Artem Bityutskiy <[email protected]>
Date: Thu, 17 May 2007 17:50:43 +0300

> Well, I see the good side of your change - no home-brewed media<->cpu
> things. Fair enough and nice. But why don't you make __be32 a struct
> (just like I do) so that compiler could complain then?

structs get passed on the stack instead of via registers, regardless
of size, when passed as arguments on some architectures, so there is a
terrible performance cost of doing things that way

2007-05-17 21:03:31

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, May 17, 2007 at 01:56:24PM -0700, David Miller wrote:
> From: Artem Bityutskiy <[email protected]>
> Date: Thu, 17 May 2007 17:50:43 +0300
>
> > Well, I see the good side of your change - no home-brewed media<->cpu
> > things. Fair enough and nice. But why don't you make __be32 a struct
> > (just like I do) so that compiler could complain then?
>
> structs get passed on the stack instead of via registers, regardless
> of size, when passed as arguments on some architectures, so there is a
> terrible performance cost of doing things that way

BTW, the lack of home-grown coversions is not a benefit - it's _nice_ to
have protections against mixing be32 and ubi32, etc. Avoiding the mess
with struct, OTOH, *is* a benefit.

So I'd rather go with independent bitwise types and conversions done by
use of be... ones + force-cast. The rest of ubi code remain unchanged.

2007-05-17 21:14:37

by Dmitry Torokhov

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On 5/17/07, Al Viro <[email protected]> wrote:
>
> Ahem... So what does
> x |= y;
> turns into with that approach?

Do we want to do such kind of operations on endian-annotated data? I'd
imagine you want to convert ot host-endianess first anyway.

--
Dmitry

2007-05-17 21:26:56

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, May 17, 2007 at 09:42:34PM +0100, Al Viro wrote:
> Ahem... So what does
> x |= y;
> turns into with that approach?

Another case is

#define FLAG1 cpu_to_be32(2)
#define FLAG2 cpu_to_be32(0x400)

if (packet_header->field & (FLAG1 | FLAG2))
....

which is a bloody common idiom in net/* and it happens on some pretty hot
paths. Approaches that would require converting it to something like
if (be32_to_cpu(packet_header->field) & (FLAG1 | FLAG2))
would be not just obnoxious, they would actually generate worse code.

2007-05-17 21:29:28

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, May 17, 2007 at 05:14:26PM -0400, Dmitry Torokhov wrote:
> On 5/17/07, Al Viro <[email protected]> wrote:
> >
> >Ahem... So what does
> > x |= y;
> >turns into with that approach?
>
> Do we want to do such kind of operations on endian-annotated data? I'd
> imagine you want to convert ot host-endianess first anyway.

Why? When both x and y are of the same type, it's a perfectly sane and
safe operation. And yes, sparse checks handle that.

2007-05-17 21:33:40

by David Miller

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

From: "Dmitry Torokhov" <[email protected]>
Date: Thu, 17 May 2007 17:14:26 -0400

> On 5/17/07, Al Viro <[email protected]> wrote:
> >
> > Ahem... So what does
> > x |= y;
> > turns into with that approach?
>
> Do we want to do such kind of operations on endian-annotated data? I'd
> imagine you want to convert ot host-endianess first anyway.

Generally you don't, if 'x' and 'y' are both in the needed
endinaness already, there is no reason to convert anything.

2007-05-17 21:47:33

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, May 17, 2007 at 02:33:34PM -0700, David Miller wrote:
> From: "Dmitry Torokhov" <[email protected]>
> Date: Thu, 17 May 2007 17:14:26 -0400
>
> > On 5/17/07, Al Viro <[email protected]> wrote:
> > >
> > > Ahem... So what does
> > > x |= y;
> > > turns into with that approach?
> >
> > Do we want to do such kind of operations on endian-annotated data? I'd
> > imagine you want to convert ot host-endianess first anyway.
>
> Generally you don't, if 'x' and 'y' are both in the needed
> endinaness already, there is no reason to convert anything.

BTW, if you have two independently defined bitwise types, sparse
will complain about mixing them, so you still have protection against
mixing unrelated types that happen to have the same endianness.

Folks, just doing annotations of net/* had been large enough and it
mostly had been about annotating declarations. If we have to rewrite
every sodding place where we happen to do bitwise operations on those...
Forget about it. BTW, another fun kind of places is comparing for
equality - also adds a fsckload of lines to rewrite^Wobscure.

2007-05-18 02:39:31

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, 2007-05-17 at 20:30 +0000, Matthieu CASTET wrote:
> On Thu, 17 May 2007 10:29:31 -0700, Andrew Morton wrote:
>
> > On Thu, 17 May 2007 18:09:50 +0300 Artem Bityutskiy
> > <[email protected]> wrote:
> >
> > umm.. I'd say what you've done in there is an improvement to the
> > exiting stuff: getting gcc to check it is better than having to use
> > sparse.
> >
> > I'd have expected gcc to generate poorer code with your approach but I'm
> > showing zero text size changes from Christoph's patch (gcc-4.1 and
> > gcc-3.4.5).
> >
> >
> On which arch did you try ?
> X86 where unaligned access are ok ?
>
> On arch that don't support aligned access, packed struct access will be
> done byte per byte (but it could be the expected behavior if there
> unaligned access).

When I tested this on ARM, the output for je32_to_cpu et al was fine.
For _other_ structures where I'd used __attribute__((packed)) to be
safe, gcc would emit code to handle unaligned loads. But not in the
simple case where the struct has only one member.

Are you suggesting that this has changed since I did my testing?

--
dwmw2

2007-05-18 02:58:10

by John Anthony Kazos Jr.

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Fri, 18 May 2007, David Woodhouse wrote:
> On Thu, 2007-05-17 at 20:30 +0000, Matthieu CASTET wrote:
> > On Thu, 17 May 2007 10:29:31 -0700, Andrew Morton wrote:
> >
> > > On Thu, 17 May 2007 18:09:50 +0300 Artem Bityutskiy
> > > <[email protected]> wrote:
> > >
> > > umm.. I'd say what you've done in there is an improvement to the
> > > exiting stuff: getting gcc to check it is better than having to use
> > > sparse.
> > >
> > > I'd have expected gcc to generate poorer code with your approach but I'm
> > > showing zero text size changes from Christoph's patch (gcc-4.1 and
> > > gcc-3.4.5).
> > >
> > >
> > On which arch did you try ?
> > X86 where unaligned access are ok ?
> >
> > On arch that don't support aligned access, packed struct access will be
> > done byte per byte (but it could be the expected behavior if there
> > unaligned access).
>
> When I tested this on ARM, the output for je32_to_cpu et al was fine.
> For _other_ structures where I'd used __attribute__((packed)) to be
> safe, gcc would emit code to handle unaligned loads. But not in the
> simple case where the struct has only one member.
>
> Are you suggesting that this has changed since I did my testing?

Wouldn't the appropriate test be to demonstrate that the same program text
opcodes are generated in both cases for all architectures? If that's not
the case, even if the generation isn't -worse-, it shows that the compiler
is doing different things with each, which means different versions of the
compiler could do different things with it, and all of a sudden another
nice programming construct which helps compile-time issues ends up
increasing the number of cases where compiler differences generate faster
or slower kernels with the same code, whereas in the test cases it was "no
better, no worse" so deemed fine.

2007-05-18 03:18:16

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Thu, 2007-05-17 at 22:57 -0400, John Anthony Kazos Jr. wrote:
> Wouldn't the appropriate test be to demonstrate that the same program text
> opcodes are generated in both cases for all architectures?

No, empirical testing with the compiler is never the _correct_ thing to
do. It's just expedient.

> If that's not the case, even if the generation isn't -worse-, it shows
> that the compiler is doing different things with each, which means
> different versions of the compiler could do different things with it,

Well yes, but even it _is_ generating precisely the same output today,
there's no reason why the compiler shouldn't behave differently under a
different phase of the moon.

The _correct_ thing to do is act upon my mutterings at the time I
removed the '__attribute__((packed))' from various JFFS2 structures to
improve the generated code on ARM -- actually implement an attribute for
GCC which has the same "don't insert any padding" meaning, but without
the unwanted "assume arbitrary alignment" implications.

It'd actually be nice if GCC knew about endianness too. I don't want to
have to do:

*x = le32_to_cpu(cpu_to_le32(*x) + 5);

I just want

uint32_t __attribute__((littleendian)) *x;

*x += 5;

I know we can hack around it for masks, with '*x |= cpu_to_le32(X_BAR);'
and such like, and we can load it into local native-endian variables and
then copy it back again later -- but it's better just to let the
compiler know what's going on and do its own optimisation. Especially on
architectures which have 'load-and-swap' or 'store-and-swap'
instructions.

--
dwmw2

2007-05-18 07:00:06

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

Al Viro wrote:
> BTW, you can simply typedef __be16 ubi16_t; etc. and define conversion
> functions as cpu_to_ubi16(x) being (__force ubi16_t)cpu_to_be16(x), etc.
>
> sparse will do all checks just fine, you still have bitwise operations
> (might or might be not relevant in your case) and for gcc it simply
> becomes __be16, etc - i.e. an integer type.

Err, what is the benefit of it? If we relied on sparce, why not would we be
just using __be16 directly?

--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

2007-05-18 08:39:26

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Fri, May 18, 2007 at 09:58:25AM +0300, Artem Bityutskiy wrote:
> Al Viro wrote:
> >BTW, you can simply typedef __be16 ubi16_t; etc. and define conversion
> >functions as cpu_to_ubi16(x) being (__force ubi16_t)cpu_to_be16(x), etc.
> >
> >sparse will do all checks just fine, you still have bitwise operations
> >(might or might be not relevant in your case) and for gcc it simply
> >becomes __be16, etc - i.e. an integer type.
>
> Err, what is the benefit of it? If we relied on sparce, why not would we be
> just using __be16 directly?

Because you might legitimately want your type to be *not* mixed with whatever
other big-endian 16bit types you have out there?

2007-05-18 11:53:16

by John Anthony Kazos Jr.

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Fri, 18 May 2007, David Woodhouse wrote:
> On Thu, 2007-05-17 at 22:57 -0400, John Anthony Kazos Jr. wrote:
> > Wouldn't the appropriate test be to demonstrate that the same program text
> > opcodes are generated in both cases for all architectures?
>
> No, empirical testing with the compiler is never the _correct_ thing to
> do. It's just expedient.
>
> > If that's not the case, even if the generation isn't -worse-, it shows
> > that the compiler is doing different things with each, which means
> > different versions of the compiler could do different things with it,
>
> Well yes, but even it _is_ generating precisely the same output today,
> there's no reason why the compiler shouldn't behave differently under a
> different phase of the moon.
>
> The _correct_ thing to do is act upon my mutterings at the time I
> removed the '__attribute__((packed))' from various JFFS2 structures to
> improve the generated code on ARM -- actually implement an attribute for
> GCC which has the same "don't insert any padding" meaning, but without
> the unwanted "assume arbitrary alignment" implications.

Out of curiosity, why would a compiler ever insert padding in a structure
that has all its elements properly-aligned?

> It'd actually be nice if GCC knew about endianness too. I don't want to
> have to do:
>
> *x = le32_to_cpu(cpu_to_le32(*x) + 5);
>
> I just want
>
> uint32_t __attribute__((littleendian)) *x;
>
> *x += 5;
>
> I know we can hack around it for masks, with '*x |= cpu_to_le32(X_BAR);'
> and such like, and we can load it into local native-endian variables and
> then copy it back again later -- but it's better just to let the
> compiler know what's going on and do its own optimisation. Especially on
> architectures which have 'load-and-swap' or 'store-and-swap'
> instructions.

2007-05-18 14:52:49

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Fri, 2007-05-18 at 07:52 -0400, John Anthony Kazos Jr. wrote:
> Out of curiosity, why would a compiler ever insert padding in a structure
> that has all its elements properly-aligned?

Well, it might decide it would be nicer if some elements were aligned to
64 bits. Or to a cache line. Or something. I don't care about _why_ --
the point is that it's _allowed_ to. Hence the original use of
__attribute__((packed)).

In practice, there's no real reason why it would do such a thing, which
is why I removed the packed attribute and replaced it with a runtime
check on the size of the structures in question.

--
dwmw2

2007-05-18 19:55:21

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

David Woodhouse wrote:
> On Thu, 2007-05-17 at 20:30 +0000, Matthieu CASTET wrote:
>> On Thu, 17 May 2007 10:29:31 -0700, Andrew Morton wrote:
>>
>>> On Thu, 17 May 2007 18:09:50 +0300 Artem Bityutskiy
> When I tested this on ARM, the output for je32_to_cpu et al was fine.
> For _other_ structures where I'd used __attribute__((packed)) to be
> safe, gcc would emit code to handle unaligned loads. But not in the
> simple case where the struct has only one member.
I don't think it is true.

When porting some summary stuff to bootloader, I hit some unaligned
access problem :
in summary.c:405 there is a
switch (je16_to_cpu(((struct jffs2_sum_unknown_flash *)sp)->nodetype)) {
and
struct jffs2_sum_unknown_flash
{
jint16_t nodetype; /* node type */
};

note that sp isn't aligned to a 32 bits boundary (it can be anything and
depend for example of the size of the dirent name).

if gcc doesn't emit code to handle unaligned loads in this case, there
is unaligned access.

When compiling my code for arm, I use a gcc-4.1.1 with a bug and it
break. With another compiler (last gcc from codesourcery) it worked
(because gcc emit load it bit per bit).

>
> Are you suggesting that this has changed since I did my testing?
>
Which version of gcc did you try ?

Matthieu

2007-05-18 20:30:33

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

Hi,

David Woodhouse wrote:
> On Thu, 2007-05-17 at 20:30 +0000, Matthieu CASTET wrote:
>> On arch that don't support aligned access, packed struct access will be
>> done byte per byte (but it could be the expected behavior if there
>> unaligned access).
>
> When I tested this on ARM, the output for je32_to_cpu et al was fine.
> For _other_ structures where I'd used __attribute__((packed)) to be
> safe, gcc would emit code to handle unaligned loads. But not in the
> simple case where the struct has only one member.
>
> Are you suggesting that this has changed since I did my testing?
>
here a small example I made and try with a sourcery 2006q3-27

/tmp/arm-2006q3/bin/arm-none-linux-gnueabi-gcc -S test.c -O3 -o
test.s.packed
/tmp/arm-2006q3/bin/arm-none-linux-gnueabi-gcc -S test.c -DNOP -O3 -o test.s

$diff test.s test.s.packed 19,23c19,34
< @ link register save eliminated.
< ldrh r2, [r1, #0]
< ldrh r1, [r0, #2]
< ldr r0, [r0, #4]
< @ lr needed for prologue
---
> stmfd sp!, {r4, r5, r6, lr}
> ldrb r2, [r0, #5] @ zero_extendqisi2
> ldrb r3, [r0, #4] @ zero_extendqisi2
> mov lr, r1
> ldrb r4, [r0, #6] @ zero_extendqisi2
> ldrb r5, [r0, #3] @ zero_extendqisi2
> ldrb r6, [r1, #1] @ zero_extendqisi2
> orr r3, r3, r2, asl #8
> ldrb r1, [r0, #2] @ zero_extendqisi2
> ldrb ip, [r0, #7] @ zero_extendqisi2
> ldrb r2, [lr, #0] @ zero_extendqisi2
> orr r3, r3, r4, asl #16
> orr r0, r3, ip, asl #24
> orr r1, r1, r5, asl #8
> orr r2, r2, r6, asl #8
> ldmfd sp!, {r4, r5, r6, lr}

Matthieu

Attachments:

test.c (820.00 B)

2007-05-18 22:01:10

by Segher Boessenkool

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

>> Out of curiosity, why would a compiler ever insert padding in a
>> structure
>> that has all its elements properly-aligned?
>
> Well, it might decide it would be nicer if some elements were aligned
> to
> 64 bits. Or to a cache line. Or something. I don't care about _why_ --
> the point is that it's _allowed_ to. Hence the original use of
> __attribute__((packed)).

It's not the compiler who decides -- struct layout is
dictated by the ABI you're compiling for.

Segher

2007-05-19 01:59:27

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Fri, 2007-05-18 at 21:55 +0200, matthieu castet wrote:
> > Are you suggesting that this has changed since I did my testing?
> >
> Which version of gcc did you try ?

It was a while ago -- probably 3.2 or something like that. I think it
might even predate the summary support.

When I get home I'll take another look.

--
dwmw2

2007-05-19 02:07:56

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

On Sat, 2007-05-19 at 00:00 +0200, Segher Boessenkool wrote:
> It's not the compiler who decides -- struct layout is
> dictated by the ABI you're compiling for.

This is true in the case of externally-visible stuff. I think the
compiler is permitted to violate the ABI for purely unit-internal things
if it makes sense though, isn't it?

Besides, in the case of the Linux kernel the ABI in question could be
one of many. It could even be a new one which was added a couple of
weeks ago, and which I had no _chance_ of considering.

The rule stands -- empirical testing of what the compiler will do isn't
usually the right answer.

--
dwmw2

2007-05-19 12:24:47

by Segher Boessenkool

[permalink] [raw]

Subject: Re: [PATCH] ubi: kill homegrown endian macros

>> It's not the compiler who decides -- struct layout is
>> dictated by the ABI you're compiling for.
>
> This is true in the case of externally-visible stuff. I think the
> compiler is permitted to violate the ABI for purely unit-internal
> things
> if it makes sense though, isn't it?

Sure. It isn't "violating the ABI" in that case though,
to be perfectly clear.

> Besides, in the case of the Linux kernel the ABI in question could be
> one of many. It could even be a new one which was added a couple of
> weeks ago, and which I had no _chance_ of considering.

Of course.

> The rule stands -- empirical testing of what the compiler will do isn't
> usually the right answer.

It is *never* the right answer. You should always write
your code so that it will do the right thing no matter
what the compiler decides to do to it.

Segher

2007-05-20 11:29:48