| Message ID | 1506954181.985.9.camel@infinera.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | ext4 build errors | expand |
On Mon, Oct 02, 2017 at 02:23:02PM +0000, Joakim Tjernlund wrote: > Hi ext4 devs > > Adding the patch last in this mail cause lots of build errors in ext4, here is a few: Why did you need this patch to fix problems in VirtualBox? Cleaning this up is going to be a little tricky, because one of the implications the void * declaration in the __set_bit_le() declaration is that there isn't any particular alignment requirement with the __le functions. But the long * declaration implies that the bitmaps have to be aligned to sizeof(long). For the ext4 bitmap, we use it on bh->b_data, for which we can safely assume is long-aligned. But the mballoc buddy bitmaps use mb_set_bit() in ways that are _not_ guaranteed to be long aligned. So fixing this is going to be a bit painful, and will likely result in a performance regression for ext4. We can make our own version that open codes it as C functions --- but then we lose all of the architecture optimized bitop functions. I believe the reason why the standard bitop functions are made long * aligned is that on some BE architectures --- I suspect it was PowerPC but I'm not 100% sure about that --- the native bitop functions required a long * alignment. Fortunately all of the little endian architectures didn't have these alignment restrictions, so we could keep the __set_bit_le functions to not have any long alignment restrictions. The fact that bitop and the bitop_le functions are not the same is... inelegant, but if it represents a practical optimization that is possible on LE systems but not on BE systems (where bitop_le gets open coded in C, in an inefficient way, but oh, well, BE systems aren't for the cool kids anyway :-), I have to ask whether it's really worth it to do the cleanup. Cheers, - Ted
On Mon, 2017-10-02 at 10:55 -0400, Theodore Ts'o wrote: > On Mon, Oct 02, 2017 at 02:23:02PM +0000, Joakim Tjernlund wrote: > > Hi ext4 devs > > > > Adding the patch last in this mail cause lots of build errors in ext4, here is a few: > > Why did you need this patch to fix problems in VirtualBox? Earlier VirtualBox had some c++ code that included kernel header and g++ is much pickier about these things so I had to do some cleanups to build some of the VirtualBox modules. I think it is better now but I haven't really checked. > > Cleaning this up is going to be a little tricky, because one of the > implications the void * declaration in the __set_bit_le() declaration > is that there isn't any particular alignment requirement with the __le > functions. But the long * declaration implies that the bitmaps have > to be aligned to sizeof(long). > > For the ext4 bitmap, we use it on bh->b_data, for which we can safely > assume is long-aligned. But the mballoc buddy bitmaps use > mb_set_bit() in ways that are _not_ guaranteed to be long aligned. > > So fixing this is going to be a bit painful, and will likely result in > a performance regression for ext4. We can make our own version that > open codes it as C functions --- but then we lose all of the > architecture optimized bitop functions. > > I believe the reason why the standard bitop functions are made long * > aligned is that on some BE architectures --- I suspect it was PowerPC > but I'm not 100% sure about that --- the native bitop functions > required a long * alignment. Fortunately all of the little endian > architectures didn't have these alignment restrictions, so we could > keep the __set_bit_le functions to not have any long alignment > restrictions. If this is a special case for ext4, can you not just do an explicit type cast in ext4 code? > > The fact that bitop and the bitop_le functions are not the same > is... inelegant, but if it represents a practical optimization that is > possible on LE systems but not on BE systems (where bitop_le gets open > coded in C, in an inefficient way, but oh, well, BE systems aren't for > the cool kids anyway :-), I have to ask whether it's really worth it > to do the cleanup. I see, but by using void * you also loose type checking w.r.t size so if you by mistake use an u32, you will not notice. Jocke
On Mon, Oct 02, 2017 at 03:15:33PM +0000, Joakim Tjernlund wrote: > > I believe the reason why the standard bitop functions are made long * > > aligned is that on some BE architectures --- I suspect it was PowerPC > > but I'm not 100% sure about that --- the native bitop functions > > required a long * alignment. Fortunately all of the little endian > > architectures didn't have these alignment restrictions, so we could > > keep the __set_bit_le functions to not have any long alignment > > restrictions. > > If this is a special case for ext4, can you not just do an explicit > type cast in ext4 code? Sure, it would be safe *today*, but then in the future someone might change an implementation of the bitop_le* functions for some architecture which would not tolerate unaligned pointers (since using a long * would imply this is allowed), and then things would break. > > The fact that bitop and the bitop_le functions are not the same > > is... inelegant, but if it represents a practical optimization that is > > possible on LE systems but not on BE systems (where bitop_le gets open > > coded in C, in an inefficient way, but oh, well, BE systems aren't for > > the cool kids anyway :-), I have to ask whether it's really worth it > > to do the cleanup. > > I see, but by using void * you also loose type checking w.r.t size so > if you by mistake use an u32, you will not notice. Um, we're never using a u32. We're using a pointer into a bit array which is often far larger than 32 or 64 bits. For example, when we use a 4k block size, then bh->b_data is a bit array which is 4096*8 == 32,768 bits. This is why void * is the right thing --- it's not a u32 or a long. It's a bit array. And in the case of the mb buddy bitmap, it's not necessarily going to start on a a byte boundary which is a multiple of 4 or 8. - Ted
On Mon, 2017-10-02 at 12:54 -0400, Theodore Ts'o wrote: > On Mon, Oct 02, 2017 at 03:15:33PM +0000, Joakim Tjernlund wrote: > > > I believe the reason why the standard bitop functions are made long * > > > aligned is that on some BE architectures --- I suspect it was PowerPC > > > but I'm not 100% sure about that --- the native bitop functions > > > required a long * alignment. Fortunately all of the little endian > > > architectures didn't have these alignment restrictions, so we could > > > keep the __set_bit_le functions to not have any long alignment > > > restrictions. > > > > If this is a special case for ext4, can you not just do an explicit > > type cast in ext4 code? > > Sure, it would be safe *today*, but then in the future someone might > change an implementation of the bitop_le* functions for some > architecture which would not tolerate unaligned pointers (since using > a long * would imply this is allowed), and then things would break. I guess I am missing something, the __set_bit_le calls __set_bit(nr ^ BITOP_LE_SWIZZLE, addr): static inline void __set_bit_le(int nr, void *addr) { __set_bit(nr ^ BITOP_LE_SWIZZLE, addr); } and __set_bit assumes it is working on a long, why is is safe in __set_bit_le to cast the void * into a long * but not in ext4 code? > > > > The fact that bitop and the bitop_le functions are not the same > > > is... inelegant, but if it represents a practical optimization that is > > > possible on LE systems but not on BE systems (where bitop_le gets open > > > coded in C, in an inefficient way, but oh, well, BE systems aren't for > > > the cool kids anyway :-), I have to ask whether it's really worth it > > > to do the cleanup. > > > > I see, but by using void * you also loose type checking w.r.t size so > > if you by mistake use an u32, you will not notice. > > Um, we're never using a u32. We're using a pointer into a bit array > which is often far larger than 32 or 64 bits. For example, when we > use a 4k block size, then bh->b_data is a bit array which is 4096*8 == > 32,768 bits. > > This is why void * is the right thing --- it's not a u32 or a long. > It's a bit array. And in the case of the mb buddy bitmap, it's not > necessarily going to start on a a byte boundary which is a multiple of > 4 or 8. For ext4 it might be right but I was using "you" in a wider scope, the rest of kernel src. Jocke
On Mon, Oct 02, 2017 at 05:27:12PM +0000, Joakim Tjernlund wrote: > > This is why void * is the right thing --- it's not a u32 or a long. > > It's a bit array. And in the case of the mb buddy bitmap, it's not > > necessarily going to start on a a byte boundary which is a multiple of > > 4 or 8. > > For ext4 it might be right but I was using "you" in a wider scope, > the rest of kernel src. So let's take this up to a high level of the problem. For the bitops functions, there are two sorts of users. Those that want to operate on an integer type (either a u32 or a long), and those that operate on bitarrays. Most of the integer users are using the bitops for in-memory state flags. Most of the bitarray use cases are for things like ext4's allocation bitmaps, where the on-disk format needs to be portable across architectures --- and that's where bitops_le tends to be used. Taking a quick look at the output of "git grep set_bit_le", most of the users are just like ext4, where it's being used for a bitarray. So my argument I think *does* apply much more widely. - Ted
On Mon, 2017-10-02 at 14:40 -0400, Theodore Ts'o wrote: > On Mon, Oct 02, 2017 at 05:27:12PM +0000, Joakim Tjernlund wrote: > > > This is why void * is the right thing --- it's not a u32 or a long. > > > It's a bit array. And in the case of the mb buddy bitmap, it's not > > > necessarily going to start on a a byte boundary which is a multiple of > > > 4 or 8. > > > > For ext4 it might be right but I was using "you" in a wider scope, > > the rest of kernel src. > > So let's take this up to a high level of the problem. For the bitops > functions, there are two sorts of users. Those that want to operate > on an integer type (either a u32 or a long), and those that operate on > bitarrays. Most of the integer users are using the bitops for > in-memory state flags. Most of the bitarray use cases are for things > like ext4's allocation bitmaps, where the on-disk format needs to be > portable across architectures --- and that's where bitops_le tends to > be used. > > Taking a quick look at the output of "git grep set_bit_le", most of > the users are just like ext4, where it's being used for a bitarray. > So my argument I think *does* apply much more widely. OK, I can see that. It is just that __set_bit_le is impl. using __set_bit(long nr, volatile unsigned long *addr) and that implies, to me, that the memory should be long aligned. Jocke
diff --git a/include/asm-generic/bitops/le.h b/include/asm-generic/bitops/le.h index 6173154..92a3afa 100644 --- a/include/asm-generic/bitops/le.h +++ b/include/asm-generic/bitops/le.h @@ -8,20 +8,22 @@ #define BITOP_LE_SWIZZLE 0 -static inline unsigned long find_next_zero_bit_le(const void *addr, - unsigned long size, unsigned long offset) +static inline unsigned long find_next_zero_bit_le(const unsigned long *addr, + unsigned long size, + unsigned long offset) { return find_next_zero_bit(addr, size, offset); } -static inline unsigned long find_next_bit_le(const void *addr, - unsigned long size, unsigned long offset) +static inline unsigned long find_next_bit_le(const unsigned long *addr, + unsigned long size, + unsigned long offset) { return find_next_bit(addr, size, offset); } -static inline unsigned long find_first_zero_bit_le(const void *addr, - unsigned long size) +static inline unsigned long find_first_zero_bit_le(const unsigned long *addr, + unsigned long size) { return find_first_zero_bit(addr, size); } @@ -31,13 +33,15 @@ static inline unsigned long find_first_zero_bit_le(const void *addr, #define BITOP_LE_SWIZZLE ((BITS_PER_LONG-1) & ~0x7) #ifndef find_next_zero_bit_le -extern unsigned long find_next_zero_bit_le(const void *addr, - unsigned long size, unsigned long offset); +extern unsigned long find_next_zero_bit_le(const unsigned long *addr, + unsigned long size, + unsigned long offset); #endif #ifndef find_next_bit_le -extern unsigned long find_next_bit_le(const void *addr, - unsigned long size, unsigned long offset); +extern unsigned long find_next_bit_le(const unsigned long *addr, + unsigned long size, + unsigned long offset); #endif #ifndef find_first_zero_bit_le @@ -49,47 +53,47 @@ extern unsigned long find_next_bit_le(const void *addr, #error "Please fix <asm/byteorder.h>" #endif -static inline int test_bit_le(int nr, const void *addr) +static inline int test_bit_le(int nr, const unsigned long *addr) { return test_bit(nr ^ BITOP_LE_SWIZZLE, addr); } -static inline void set_bit_le(int nr, void *addr) +static inline void set_bit_le(int nr, unsigned long *addr) { set_bit(nr ^ BITOP_LE_SWIZZLE, addr); } -static inline void clear_bit_le(int nr, void *addr) +static inline void clear_bit_le(int nr, unsigned long *addr) { clear_bit(nr ^ BITOP_LE_SWIZZLE, addr); } -static inline void __set_bit_le(int nr, void *addr) +static inline void __set_bit_le(int nr, unsigned long *addr) { __set_bit(nr ^ BITOP_LE_SWIZZLE, addr); } -static inline void __clear_bit_le(int nr, void *addr) +static inline void __clear_bit_le(int nr, unsigned long *addr) { __clear_bit(nr ^ BITOP_LE_SWIZZLE, addr); } -static inline int test_and_set_bit_le(int nr, void *addr) +static inline int test_and_set_bit_le(int nr, unsigned long *addr) { return test_and_set_bit(nr ^ BITOP_LE_SWIZZLE, addr); } -static inline int test_and_clear_bit_le(int nr, void *addr) +static inline int test_and_clear_bit_le(int nr, unsigned long *addr) { return test_and_clear_bit(nr ^ BITOP_LE_SWIZZLE, addr); } -static inline int __test_and_set_bit_le(int nr, void *addr) +static inline int __test_and_set_bit_le(int nr, unsigned long *addr) { return __test_and_set_bit(nr ^ BITOP_LE_SWIZZLE, addr); } -static inline int __test_and_clear_bit_le(int nr, void *addr) +static inline int __test_and_clear_bit_le(int nr, unsigned long *addr) { return __test_and_clear_bit(nr ^ BITOP_LE_SWIZZLE, addr); }
Hi ext4 devs Adding the patch last in this mail cause lots of build errors in ext4, here is a few: fs/ext4/balloc.c: In function ‘ext4_init_block_bitmap’: fs/ext4/balloc.c:215:21: error: passing argument 2 of ‘__set_bit_le’ from incompatible pointer type [-Werror=incompatible-pointer-types] ext4_set_bit(bit, bh->b_data); ^ In file included from ./arch/x86/include/asm/bitops.h:517:0, from ./include/linux/bitops.h:36, from ./include/linux/kernel.h:10, from ./include/linux/list.h:8, from ./include/linux/preempt.h:10, from ./include/linux/spinlock.h:50, from ./include/linux/seqlock.h:35, from ./include/linux/time.h:5, from fs/ext4/balloc.c:14: ./include/asm-generic/bitops/le.h:71:20: note: expected ‘long unsigned int *’ but argument is of type ‘char *’ static inline void __set_bit_le(int nr, unsigned long *addr) ^ fs/ext4/balloc.c:225:44: error: passing argument 2 of ‘__set_bit_le’ from incompatible pointer type [-Werror=incompatible-pointer-types] ext4_set_bit(EXT4_B2C(sbi, tmp - start), bh->b_data); ^ In file included from ./arch/x86/include/asm/bitops.h:517:0, from ./include/linux/bitops.h:36, from ./include/linux/kernel.h:10, from ./include/linux/list.h:8, from ./include/linux/preempt.h:10, from ./include/linux/spinlock.h:50, from ./include/linux/seqlock.h:35, from ./include/linux/time.h:5, from fs/ext4/balloc.c:14: ./include/asm-generic/bitops/le.h:71:20: note: expected ‘long unsigned int *’ but argument is of type ‘char *’ static inline void __set_bit_le(int nr, unsigned long *addr) ^ fs/ext4/balloc.c:229:44: error: passing argument 2 of ‘__set_bit_le’ from incompatible pointer type [-Werror=incompatible-pointer-types] ext4_set_bit(EXT4_B2C(sbi, tmp - start), bh->b_data); ^ In file included from ./arch/x86/include/asm/bitops.h:517:0, from ./include/linux/bitops.h:36, from ./include/linux/kernel.h:10, from ./include/linux/list.h:8, from ./include/linux/preempt.h:10, from ./include/linux/spinlock.h:50, from ./include/linux/seqlock.h:35, from ./include/linux/time.h:5, from fs/ext4/balloc.c:14: ./include/asm-generic/bitops/le.h:71:20: note: expected ‘long unsigned int *’ but argument is of type ‘char *’ static inline void __set_bit_le(int nr, unsigned long *addr) ^ fs/ext4/balloc.c:235:45: error: passing argument 2 of ‘__set_bit_le’ from incompatible pointer type [-Werror=incompatible-pointer-types] ext4_set_bit(EXT4_B2C(sbi, tmp - start), bh->b_data); I think ext4 needs some cleanup before my patch can be applied, what do you think? Jocke From 18c2d4659a33ac047b31a0e33c524811a2a8f642 Mon Sep 17 00:00:00 2001 From: Joakim Tjernlund <joakim.tjernlund@infinera.com> Date: Thu, 9 Mar 2017 14:19:41 +0100 Subject: [PATCH 1/4] x86: Match bitops prototypes Adjust bitops function prototype in arch/x86/include/asm/le.h to match the generic ones in asm-generic/bitops/le.h That is, replace void* with unsigned long* --- include/asm-generic/bitops/le.h | 42 ++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) -- 2.10.2