* ext4 build errors
@ 2017-10-02 14:23 Joakim Tjernlund
2017-10-02 14:55 ` Theodore Ts'o
0 siblings, 1 reply; 7+ messages in thread
From: Joakim Tjernlund @ 2017-10-02 14:23 UTC (permalink / raw)
To: linux-ext4
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(-)
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);
}
--
2.10.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: ext4 build errors
2017-10-02 14:23 ext4 build errors Joakim Tjernlund
@ 2017-10-02 14:55 ` Theodore Ts'o
2017-10-02 15:15 ` Joakim Tjernlund
0 siblings, 1 reply; 7+ messages in thread
From: Theodore Ts'o @ 2017-10-02 14:55 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: linux-ext4
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ext4 build errors
2017-10-02 14:55 ` Theodore Ts'o
@ 2017-10-02 15:15 ` Joakim Tjernlund
2017-10-02 16:54 ` Theodore Ts'o
0 siblings, 1 reply; 7+ messages in thread
From: Joakim Tjernlund @ 2017-10-02 15:15 UTC (permalink / raw)
To: tytso; +Cc: linux-ext4
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ext4 build errors
2017-10-02 15:15 ` Joakim Tjernlund
@ 2017-10-02 16:54 ` Theodore Ts'o
2017-10-02 17:27 ` [EXTERNAL]Re: " Joakim Tjernlund
0 siblings, 1 reply; 7+ messages in thread
From: Theodore Ts'o @ 2017-10-02 16:54 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: linux-ext4
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [EXTERNAL]Re: ext4 build errors
2017-10-02 16:54 ` Theodore Ts'o
@ 2017-10-02 17:27 ` Joakim Tjernlund
2017-10-02 18:40 ` Theodore Ts'o
0 siblings, 1 reply; 7+ messages in thread
From: Joakim Tjernlund @ 2017-10-02 17:27 UTC (permalink / raw)
To: tytso; +Cc: linux-ext4
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [EXTERNAL]Re: ext4 build errors
2017-10-02 17:27 ` [EXTERNAL]Re: " Joakim Tjernlund
@ 2017-10-02 18:40 ` Theodore Ts'o
2017-10-02 20:12 ` [EXTERNAL]Re: " Joakim Tjernlund
0 siblings, 1 reply; 7+ messages in thread
From: Theodore Ts'o @ 2017-10-02 18:40 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: linux-ext4
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [EXTERNAL]Re: [EXTERNAL]Re: ext4 build errors
2017-10-02 18:40 ` Theodore Ts'o
@ 2017-10-02 20:12 ` Joakim Tjernlund
0 siblings, 0 replies; 7+ messages in thread
From: Joakim Tjernlund @ 2017-10-02 20:12 UTC (permalink / raw)
To: tytso; +Cc: linux-ext4
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-10-02 20:13 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-02 14:23 ext4 build errors Joakim Tjernlund
2017-10-02 14:55 ` Theodore Ts'o
2017-10-02 15:15 ` Joakim Tjernlund
2017-10-02 16:54 ` Theodore Ts'o
2017-10-02 17:27 ` [EXTERNAL]Re: " Joakim Tjernlund
2017-10-02 18:40 ` Theodore Ts'o
2017-10-02 20:12 ` [EXTERNAL]Re: " Joakim Tjernlund
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.