ubi: kill homegrown endian macros

Kill ubis homegrown endianess handling crap and replace it with the normal kernel endianess handling.

Christoph,

Quoted 2 lines · Christoph Hellwig

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?

Quoted 6 lines · Artem Bityutskiy

Christoph,

Quoted 2 lines · Christoph Hellwig

Kill ubis homegrown endianess handling crap and replace it with the normal kernel endianess handling.

Quoted 4 lines

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.

Quoted 12 lines · Christoph Hellwig

Quoted 10 lines · Artem Bityutskiy

Christoph,

Quoted 2 lines · Christoph Hellwig

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?

Quoted 1 line

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.

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.

Quoted 7 lines · Andrew Morton

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).

Quoted 6 lines

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.

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).

Quoted 12 lines · Andrew Morton

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).

Quoted 14 lines · Matthieu CASTET

Quoted 12 lines · Andrew Morton

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).

Quoted 6 lines

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,

Quoted 2 lines · John Anthony Kazos Jr.

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.

Quoted 3 lines

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.

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

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 3 lines · Matthieu CASTET

Quoted 1 line · Andrew Morton

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.

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

Quoted 2 lines

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

Which version of gcc did you try ?

Quoted 1 line · Matthieu CASTET

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?

      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}

Quoted 13 lines · On Thu, 17 May 2007 18:09:50 +0300 Artem Bityutskiy <[email protected]>…

Quoted 12 lines · Christoph Hellwig

Quoted 10 lines · Artem Bityutskiy

Christoph,

Quoted 2 lines · Christoph Hellwig

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?

Quoted 5 lines

Quoted 1 line

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?

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.

Quoted 6 lines · Al Viro

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?

Ahem... So what does

       x |= y;

turns into with that approach?

Quoted 4 lines · Al Viro

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.

Quoted 4 lines

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.

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

Quoted 6 lines · Al Viro

Quoted 4 lines

Ahem... So what does

       x |= y;

turns into with that approach?

Quoted 2 lines

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.

Ahem... So what does

	x |= y;

turns into with that approach?

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?

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

Quoted 3 lines

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

Kill ubis homegrown endianess handling crap and replace it with the normal kernel endianess handling.

Quoted 2 lines · Christoph Hellwig

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.

Quoted 4 lines · David Woodhouse

Quoted 2 lines · Christoph Hellwig

Kill ubis homegrown endianess handling crap and replace it with the normal kernel endianess handling.

Quoted 4 lines

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.

Quoted 10 lines · Christoph Hellwig

Quoted 8 lines · David Woodhouse

Quoted 2 lines · Christoph Hellwig

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.

Quoted 3 lines

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 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)

Kill ubis homegrown endianess handling crap and replace it with the normal kernel endianess handling.