Re: [PATCH 1/2] Make cramfs little endian only

[Jeff Garzik <jeff@garzik.org>]

Linus Torvalds wrote:
> 
> On Tue, 4 Dec 2007, Andi Drebes wrote:
>> Perhaps I'm missing somehting, but I think for cramfs, unfortunately,
>> there has to be this statement. The bitfields in the cramfs_inode structure
>> cause some problems.
> 
> I agree that bitfields can be painful, but they should likely be just 
> rewritten to be accesses using actual masks and shifts. The thing is, 
> bitfields aren't actually endianness safe *anyway*, in that a compiler may 
> end up using a *different* bit order than the byte order.  So you cannot 
> really use bitfields reliably on things like that (although Linux has a 
> notion of a "__[BIG|LITTLE]_ENDIAN_BITFIELD", if you really want to).

Bitfields also generate lower-quality assembly than masks&shifts 
(typically more instructions using additional temporaries to accomplish 
the same thing), based on my own informal gcc testing.

You would think gcc would be advanced enough to turn bitfield use into 
masks and shifts under the hood, but for whatever reason that often is 
not the case in kernel code.

Due to the way they're used, bitfields make more difficult the common 
code pattern of setting several flags at once:

	(assuming 'foo', 'bar' and 'baz' are bitfields in a struct)
	pdev->foo = 1;
	pdev->bar = 0;
	pdev->baz = 1;

     versus

	flag_foo = (1 << 0);
	flag_bar = (1 << 1);
	flag_baz = (1 << 2);
	...
	pdev->flags = flag_foo | flag_bar;

</pet peeve>

And getting back on topic, I think "pdev->flags = 
cpu_to_le32(flag1|flag2)" is nicer than dealing with bitfields, when 
your data structures are fixed-endian.

	Jeff