Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
On Tue, Aug 23, 2016 at 07:23:26AM -0700, Paul E. McKenney wrote: > On Tue, Aug 23, 2016 at 03:45:51PM +0200, Geert Uytterhoeven wrote: > > Hi Paul, > > > > On Tue, Aug 23, 2016 at 3:43 PM, Paul E. McKenney > > wrote: > > > On Tue, Aug 23, 2016 at 08:39:18AM +0200, Geert Uytterhoeven wrote: > > >> On Mon, Aug 22, 2016 at 11:16 PM, Paul E. McKenney > > >> wrote: > > >> > On Mon, Aug 22, 2016 at 10:48:57PM +0200, Geert Uytterhoeven wrote: > > >> >> On Mon, Aug 22, 2016 at 9:54 PM, Paul E. McKenney > > >> >> wrote: > > >> >> > On Mon, Aug 22, 2016 at 03:18:54PM -0400, Steven Rostedt wrote: > > >> >> >> On Mon, 22 Aug 2016 20:56:09 +0200 > > >> >> >> Peter Zijlstra wrote: > > >> >> >> > > >> >> >> > > Don't we have __alignof__(void *) to avoid #ifdef CONFIG_M68K > > >> >> >> > > and > > >> >> >> > > other new macros ? > > >> >> > > > >> >> > Hmmm... Does __alignof__(void *) give two-byte alignment on m68k, > > >> >> > allowing something like this? Heh!!! It is already there. ;-) > > >> >> > > > >> >> > struct callback_head { > > >> >> > struct callback_head *next; > > >> >> > void (*func)(struct callback_head *head); > > >> >> > } __attribute__((aligned(sizeof(void *; > > >> >> > > >> >> No, it's aligning to sizeof(void *) (4 on m68k), not __alignof__(void > > >> >> *). > > >> > > > >> > Right you are. Commit 720abae3d68ae from Kirill A. Shutemov in > > >> > November > > >> > 2015. > > >> > > > >> > Given that you haven't complained, I am guessing that this works for > > >> > you. > > >> > If so, I can make the __call_rcu() WARN_ON() more strict. > > >> > Again, does the current state work for you? > > > > >> Yes it does. See also your commit 1146edcbef378922 ("rcu: Loosen > > >> __call_rcu()'s > > >> rcu_head alignment constraint"). > > > > > > Understood! > > > > > > But given that all architectures now provide at least four-byte alignment > > > for the rcu_head structure, isn't it now OK for me to tighten up > > > __call_rcu()'s > > > check, for example, to this? > > > > > > WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1)); > > > > Yes, I agree with that. > > Very good, I have queued the following patch. > > Thanx, Paul > > > > commit 89d39c83d193733ed5fff1c480cd42c9de1da404 > Author: Paul E. McKenney > Date: Tue Aug 23 06:51:47 2016 -0700 > > rcu: Tighted up __call_rcu() rcu_head alignment check > > Commit 720abae3d68ae ("rcu: force alignment on struct > callback_head/rcu_head") forced the rcu_head (AKA callback_head) > structure's alignment to pointer size, that is, to 4-byte boundaries on > 32-bit systems and to 8-byte boundaries on 64-bit systems. This > commit therefore checks for this same alignment in __call_rcu(), > which used to check for two-byte alignment. > > Signed-off-by: Paul E. McKenney > Cc: Geert Uytterhoeven > Cc: Kirill A. Shutemov Looks good to me. Acked-by: Kirill A. Shutemov -- Kirill A. Shutemov
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
On Mon, Aug 22, 2016 at 08:14:43AM -0700, Paul E. McKenney wrote: > The __call_rcu() assertion that checks only the bottom bit of the > rcu_head pointer is a bit counter-intuitive in these days of ubiquitous > 64-bit systems. This commit therefore records the reason for this > odd alignment check, namely that m68k guarantees only two-byte alignment > despite being a 32-bit architectures. Would not something like: #ifdef CONFIG_M68K /* * m68k is weird and doesn't have naturally aligned types. */ WARN_ON_ONCE((unsigned long)head & 1); #else WARN_ON_ONCE((unsigned long)head & (sizeof(unsigned long) - 1)); #endif Be better?
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
On Mon, Aug 22, 2016 at 06:25:53PM +0200, Peter Zijlstra wrote: > On Mon, Aug 22, 2016 at 08:14:43AM -0700, Paul E. McKenney wrote: > > The __call_rcu() assertion that checks only the bottom bit of the > > rcu_head pointer is a bit counter-intuitive in these days of ubiquitous > > 64-bit systems. This commit therefore records the reason for this > > odd alignment check, namely that m68k guarantees only two-byte alignment > > despite being a 32-bit architectures. > > Would not something like: > > #ifdef CONFIG_M68K > /* >* m68k is weird and doesn't have naturally aligned types. >*/ > WARN_ON_ONCE((unsigned long)head & 1); > #else > WARN_ON_ONCE((unsigned long)head & (sizeof(unsigned long) - 1)); > #endif > > Be better? That does have much to say for itself, though I would prefer sizeof(void *) to sizeof(unsigned long). But would it make sense to define a mask on a per-architecture basis, with the default being (sizeof(void *) - 1)? Then maybe an IMPROPERLY_ALIGNED_POINTER(): #ifndef CONFIG_ARCH_POINTER_ALIGNMENT #define CONFIG_ARCH_POINTER_ALIGNMENT (sizeof(void *) - 1) #endif #define IMPROPERLY_ALIGNED_POINTER(p) \ ((p) & CONFIG_ARCH_POINTER_ALIGNMENT) m68k would define ARCH_POINTER_ALIGNMENT to 1, and all other arches would leave it undefined. Then __call_rcu() could to this: WARN_ON_ONCE(IMPROPERLY_ALIGNED_POINTER(head)); Seem reasonable? Thanx, Paul
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
On Mon, Aug 22, 2016 at 10:34 AM, Paul E. McKenney wrote: > That does have much to say for itself, though I would prefer sizeof(void > *) to sizeof(unsigned long). But would it make sense to define a mask > on a per-architecture basis, with the default being (sizeof(void *) - 1)? > Then maybe an IMPROPERLY_ALIGNED_POINTER(): > > #ifndef CONFIG_ARCH_POINTER_ALIGNMENT > #define CONFIG_ARCH_POINTER_ALIGNMENT (sizeof(void *) - 1) > #endif > > #define IMPROPERLY_ALIGNED_POINTER(p) \ > ((p) & CONFIG_ARCH_POINTER_ALIGNMENT) > > m68k would define ARCH_POINTER_ALIGNMENT to 1, and all other arches > would leave it undefined. > > Then __call_rcu() could to this: > > WARN_ON_ONCE(IMPROPERLY_ALIGNED_POINTER(head)); Don't we have __alignof__(void *) to avoid #ifdef CONFIG_M68K and other new macros ?
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
On Mon, Aug 22, 2016 at 11:48:53AM -0700, Eric Dumazet wrote: > On Mon, Aug 22, 2016 at 10:34 AM, Paul E. McKenney > wrote: > > > That does have much to say for itself, though I would prefer sizeof(void > > *) to sizeof(unsigned long). But would it make sense to define a mask > > on a per-architecture basis, with the default being (sizeof(void *) - 1)? > > Then maybe an IMPROPERLY_ALIGNED_POINTER(): > > > > #ifndef CONFIG_ARCH_POINTER_ALIGNMENT > > #define CONFIG_ARCH_POINTER_ALIGNMENT (sizeof(void *) - 1) > > #endif > > > > #define IMPROPERLY_ALIGNED_POINTER(p) \ > > ((p) & CONFIG_ARCH_POINTER_ALIGNMENT) > > > > m68k would define ARCH_POINTER_ALIGNMENT to 1, and all other arches > > would leave it undefined. > > > > Then __call_rcu() could to this: > > > > WARN_ON_ONCE(IMPROPERLY_ALIGNED_POINTER(head)); > > Don't we have __alignof__(void *) to avoid #ifdef CONFIG_M68K and > other new macros ? Yes, but that 'hides' the m68k funny, while doing an explicit #ifdef has documentation value... but I don't care too deeply.
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
On Mon, 22 Aug 2016 20:56:09 +0200 Peter Zijlstra wrote: > > Don't we have __alignof__(void *) to avoid #ifdef CONFIG_M68K and > > other new macros ? > > Yes, but that 'hides' the m68k funny, while doing an explicit #ifdef has > documentation value... but I don't care too deeply. I'd recommend keeping the #ifdef, and then if another architecture comes along that is as weird as m68k, we can use the generic __alignof__(void *). Maybe even add that in the comment, so when/if that arch is created, people will know how to fix it more generically. -- Steve
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
On Mon, Aug 22, 2016 at 03:18:54PM -0400, Steven Rostedt wrote: > On Mon, 22 Aug 2016 20:56:09 +0200 > Peter Zijlstra wrote: > > > > Don't we have __alignof__(void *) to avoid #ifdef CONFIG_M68K and > > > other new macros ? Hmmm... Does __alignof__(void *) give two-byte alignment on m68k, allowing something like this? Heh!!! It is already there. ;-) struct callback_head { struct callback_head *next; void (*func)(struct callback_head *head); } __attribute__((aligned(sizeof(void *; #define rcu_head callback_head If so, that does sound quite attractive! Might need the WARN_ON() anyway, to flag wild pointers if nothing else. Adding Geert on CC for his thoughts. > > Yes, but that 'hides' the m68k funny, while doing an explicit #ifdef has > > documentation value... but I don't care too deeply. Well, if I need the WARN_ON() anyway, perhaps we get both. > I'd recommend keeping the #ifdef, and then if another architecture > comes along that is as weird as m68k, we can use the generic > __alignof__(void *). Maybe even add that in the comment, so when/if > that arch is created, people will know how to fix it more generically. Maybe __call_rcu() can do something like this? WARN_ON_ONCE((unsigned long)head & (__alignof__(struct rcu_head) - 1)); Except that RCU needs at least two-byte alignment, so something like this? WARN_ON_ONCE((unsigned long)head & ((__alignof__(struct rcu_head) - 1) | 0x1)); That way, some future architecture that doesn't believe in any alignment at all will be properly informed of RCU's needs in this area. Thanx, Paul
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
Hi Paul, On Mon, Aug 22, 2016 at 9:54 PM, Paul E. McKenney wrote: > On Mon, Aug 22, 2016 at 03:18:54PM -0400, Steven Rostedt wrote: >> On Mon, 22 Aug 2016 20:56:09 +0200 >> Peter Zijlstra wrote: >> >> > > Don't we have __alignof__(void *) to avoid #ifdef CONFIG_M68K and >> > > other new macros ? > > Hmmm... Does __alignof__(void *) give two-byte alignment on m68k, > allowing something like this? Heh!!! It is already there. ;-) > > struct callback_head { > struct callback_head *next; > void (*func)(struct callback_head *head); > } __attribute__((aligned(sizeof(void *; No, it's aligning to sizeof(void *) (4 on m68k), not __alignof__(void *). > #define rcu_head callback_head > > If so, that does sound quite attractive! Might need the WARN_ON() > anyway, to flag wild pointers if nothing else. > > Adding Geert on CC for his thoughts. __alignof__(void *) is indeed 2 on m68k, and h8300. Note that it is 1 on crisv32! It's 4 or 8 on anything else I have a cross-compiler for. $ cat a.c unsigned x = __alignof__(void *); $ for i in /opt/cross/*/*/bin/*gcc; do echo +++ $i +++; $i -c -S a.c; cat a.s; done | less Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
On Mon, Aug 22, 2016 at 10:48:57PM +0200, Geert Uytterhoeven wrote: > Hi Paul, > > On Mon, Aug 22, 2016 at 9:54 PM, Paul E. McKenney > wrote: > > On Mon, Aug 22, 2016 at 03:18:54PM -0400, Steven Rostedt wrote: > >> On Mon, 22 Aug 2016 20:56:09 +0200 > >> Peter Zijlstra wrote: > >> > >> > > Don't we have __alignof__(void *) to avoid #ifdef CONFIG_M68K and > >> > > other new macros ? > > > > Hmmm... Does __alignof__(void *) give two-byte alignment on m68k, > > allowing something like this? Heh!!! It is already there. ;-) > > > > struct callback_head { > > struct callback_head *next; > > void (*func)(struct callback_head *head); > > } __attribute__((aligned(sizeof(void *; > > No, it's aligning to sizeof(void *) (4 on m68k), not __alignof__(void *). Right you are. Commit 720abae3d68ae from Kirill A. Shutemov in November 2015. Given that you haven't complained, I am guessing that this works for you. If so, I can make the __call_rcu() WARN_ON() more strict. > > #define rcu_head callback_head > > > > If so, that does sound quite attractive! Might need the WARN_ON() > > anyway, to flag wild pointers if nothing else. > > > > Adding Geert on CC for his thoughts. > > __alignof__(void *) is indeed 2 on m68k, and h8300. > > Note that it is 1 on crisv32! Gah... ((__alignof__(void *) + 1) & ~0x1), eh? > It's 4 or 8 on anything else I have a cross-compiler for. > > $ cat a.c > unsigned x = __alignof__(void *); > $ for i in /opt/cross/*/*/bin/*gcc; do echo +++ $i +++; $i -c -S a.c; > cat a.s; done | less Thank you for checking! Again, does the current state work for you? Thanx, Paul
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
Hi Paul, On Mon, Aug 22, 2016 at 11:16 PM, Paul E. McKenney wrote: > On Mon, Aug 22, 2016 at 10:48:57PM +0200, Geert Uytterhoeven wrote: >> On Mon, Aug 22, 2016 at 9:54 PM, Paul E. McKenney >> wrote: >> > On Mon, Aug 22, 2016 at 03:18:54PM -0400, Steven Rostedt wrote: >> >> On Mon, 22 Aug 2016 20:56:09 +0200 >> >> Peter Zijlstra wrote: >> >> >> >> > > Don't we have __alignof__(void *) to avoid #ifdef CONFIG_M68K and >> >> > > other new macros ? >> > >> > Hmmm... Does __alignof__(void *) give two-byte alignment on m68k, >> > allowing something like this? Heh!!! It is already there. ;-) >> > >> > struct callback_head { >> > struct callback_head *next; >> > void (*func)(struct callback_head *head); >> > } __attribute__((aligned(sizeof(void *; >> >> No, it's aligning to sizeof(void *) (4 on m68k), not __alignof__(void *). > > Right you are. Commit 720abae3d68ae from Kirill A. Shutemov in November > 2015. > > Given that you haven't complained, I am guessing that this works for you. > If so, I can make the __call_rcu() WARN_ON() more strict. > >> > #define rcu_head callback_head >> > >> > If so, that does sound quite attractive! Might need the WARN_ON() >> > anyway, to flag wild pointers if nothing else. >> > >> > Adding Geert on CC for his thoughts. >> >> __alignof__(void *) is indeed 2 on m68k, and h8300. >> >> Note that it is 1 on crisv32! > > Gah... ((__alignof__(void *) + 1) & ~0x1), eh? > >> It's 4 or 8 on anything else I have a cross-compiler for. >> >> $ cat a.c >> unsigned x = __alignof__(void *); >> $ for i in /opt/cross/*/*/bin/*gcc; do echo +++ $i +++; $i -c -S a.c; >> cat a.s; done | less > > Thank you for checking! > > Again, does the current state work for you? Yes it does. See also your commit 1146edcbef378922 ("rcu: Loosen __call_rcu()'s rcu_head alignment constraint"). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
On Tue, Aug 23, 2016 at 08:39:18AM +0200, Geert Uytterhoeven wrote: > Hi Paul, > > On Mon, Aug 22, 2016 at 11:16 PM, Paul E. McKenney > wrote: > > On Mon, Aug 22, 2016 at 10:48:57PM +0200, Geert Uytterhoeven wrote: > >> On Mon, Aug 22, 2016 at 9:54 PM, Paul E. McKenney > >> wrote: > >> > On Mon, Aug 22, 2016 at 03:18:54PM -0400, Steven Rostedt wrote: > >> >> On Mon, 22 Aug 2016 20:56:09 +0200 > >> >> Peter Zijlstra wrote: > >> >> > >> >> > > Don't we have __alignof__(void *) to avoid #ifdef CONFIG_M68K and > >> >> > > other new macros ? > >> > > >> > Hmmm... Does __alignof__(void *) give two-byte alignment on m68k, > >> > allowing something like this? Heh!!! It is already there. ;-) > >> > > >> > struct callback_head { > >> > struct callback_head *next; > >> > void (*func)(struct callback_head *head); > >> > } __attribute__((aligned(sizeof(void *; > >> > >> No, it's aligning to sizeof(void *) (4 on m68k), not __alignof__(void *). > > > > Right you are. Commit 720abae3d68ae from Kirill A. Shutemov in November > > 2015. > > > > Given that you haven't complained, I am guessing that this works for you. > > If so, I can make the __call_rcu() WARN_ON() more strict. > > > >> > #define rcu_head callback_head > >> > > >> > If so, that does sound quite attractive! Might need the WARN_ON() > >> > anyway, to flag wild pointers if nothing else. > >> > > >> > Adding Geert on CC for his thoughts. > >> > >> __alignof__(void *) is indeed 2 on m68k, and h8300. > >> > >> Note that it is 1 on crisv32! > > > > Gah... ((__alignof__(void *) + 1) & ~0x1), eh? > > > >> It's 4 or 8 on anything else I have a cross-compiler for. > >> > >> $ cat a.c > >> unsigned x = __alignof__(void *); > >> $ for i in /opt/cross/*/*/bin/*gcc; do echo +++ $i +++; $i -c -S a.c; > >> cat a.s; done | less > > > > Thank you for checking! > > > > Again, does the current state work for you? > > Yes it does. See also your commit 1146edcbef378922 ("rcu: Loosen > __call_rcu()'s > rcu_head alignment constraint"). Understood! But given that all architectures now provide at least four-byte alignment for the rcu_head structure, isn't it now OK for me to tighten up __call_rcu()'s check, for example, to this? WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1)); Thanx, Paul
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
Hi Paul, On Tue, Aug 23, 2016 at 3:43 PM, Paul E. McKenney wrote: > On Tue, Aug 23, 2016 at 08:39:18AM +0200, Geert Uytterhoeven wrote: >> On Mon, Aug 22, 2016 at 11:16 PM, Paul E. McKenney >> wrote: >> > On Mon, Aug 22, 2016 at 10:48:57PM +0200, Geert Uytterhoeven wrote: >> >> On Mon, Aug 22, 2016 at 9:54 PM, Paul E. McKenney >> >> wrote: >> >> > On Mon, Aug 22, 2016 at 03:18:54PM -0400, Steven Rostedt wrote: >> >> >> On Mon, 22 Aug 2016 20:56:09 +0200 >> >> >> Peter Zijlstra wrote: >> >> >> >> >> >> > > Don't we have __alignof__(void *) to avoid #ifdef CONFIG_M68K and >> >> >> > > other new macros ? >> >> > >> >> > Hmmm... Does __alignof__(void *) give two-byte alignment on m68k, >> >> > allowing something like this? Heh!!! It is already there. ;-) >> >> > >> >> > struct callback_head { >> >> > struct callback_head *next; >> >> > void (*func)(struct callback_head *head); >> >> > } __attribute__((aligned(sizeof(void *; >> >> >> >> No, it's aligning to sizeof(void *) (4 on m68k), not __alignof__(void *). >> > >> > Right you are. Commit 720abae3d68ae from Kirill A. Shutemov in November >> > 2015. >> > >> > Given that you haven't complained, I am guessing that this works for you. >> > If so, I can make the __call_rcu() WARN_ON() more strict. >> > Again, does the current state work for you? >> Yes it does. See also your commit 1146edcbef378922 ("rcu: Loosen >> __call_rcu()'s >> rcu_head alignment constraint"). > > Understood! > > But given that all architectures now provide at least four-byte alignment > for the rcu_head structure, isn't it now OK for me to tighten up > __call_rcu()'s > check, for example, to this? > > WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1)); Yes, I agree with that. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
On Tue, Aug 23, 2016 at 4:23 PM, Paul E. McKenney wrote: > commit 89d39c83d193733ed5fff1c480cd42c9de1da404 > Author: Paul E. McKenney > Date: Tue Aug 23 06:51:47 2016 -0700 > > rcu: Tighted up __call_rcu() rcu_head alignment check > > Commit 720abae3d68ae ("rcu: force alignment on struct > callback_head/rcu_head") forced the rcu_head (AKA callback_head) > structure's alignment to pointer size, that is, to 4-byte boundaries on > 32-bit systems and to 8-byte boundaries on 64-bit systems. This > commit therefore checks for this same alignment in __call_rcu(), > which used to check for two-byte alignment. > > Signed-off-by: Paul E. McKenney > Cc: Geert Uytterhoeven > Cc: Kirill A. Shutemov Tested-by: Geert Uytterhoeven Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
On Tue, Aug 23, 2016 at 03:45:51PM +0200, Geert Uytterhoeven wrote: > Hi Paul, > > On Tue, Aug 23, 2016 at 3:43 PM, Paul E. McKenney > wrote: > > On Tue, Aug 23, 2016 at 08:39:18AM +0200, Geert Uytterhoeven wrote: > >> On Mon, Aug 22, 2016 at 11:16 PM, Paul E. McKenney > >> wrote: > >> > On Mon, Aug 22, 2016 at 10:48:57PM +0200, Geert Uytterhoeven wrote: > >> >> On Mon, Aug 22, 2016 at 9:54 PM, Paul E. McKenney > >> >> wrote: > >> >> > On Mon, Aug 22, 2016 at 03:18:54PM -0400, Steven Rostedt wrote: > >> >> >> On Mon, 22 Aug 2016 20:56:09 +0200 > >> >> >> Peter Zijlstra wrote: > >> >> >> > >> >> >> > > Don't we have __alignof__(void *) to avoid #ifdef CONFIG_M68K and > >> >> >> > > other new macros ? > >> >> > > >> >> > Hmmm... Does __alignof__(void *) give two-byte alignment on m68k, > >> >> > allowing something like this? Heh!!! It is already there. ;-) > >> >> > > >> >> > struct callback_head { > >> >> > struct callback_head *next; > >> >> > void (*func)(struct callback_head *head); > >> >> > } __attribute__((aligned(sizeof(void *; > >> >> > >> >> No, it's aligning to sizeof(void *) (4 on m68k), not __alignof__(void > >> >> *). > >> > > >> > Right you are. Commit 720abae3d68ae from Kirill A. Shutemov in November > >> > 2015. > >> > > >> > Given that you haven't complained, I am guessing that this works for you. > >> > If so, I can make the __call_rcu() WARN_ON() more strict. > >> > Again, does the current state work for you? > > >> Yes it does. See also your commit 1146edcbef378922 ("rcu: Loosen > >> __call_rcu()'s > >> rcu_head alignment constraint"). > > > > Understood! > > > > But given that all architectures now provide at least four-byte alignment > > for the rcu_head structure, isn't it now OK for me to tighten up > > __call_rcu()'s > > check, for example, to this? > > > > WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1)); > > Yes, I agree with that. Very good, I have queued the following patch. Thanx, Paul commit 89d39c83d193733ed5fff1c480cd42c9de1da404 Author: Paul E. McKenney Date: Tue Aug 23 06:51:47 2016 -0700 rcu: Tighted up __call_rcu() rcu_head alignment check Commit 720abae3d68ae ("rcu: force alignment on struct callback_head/rcu_head") forced the rcu_head (AKA callback_head) structure's alignment to pointer size, that is, to 4-byte boundaries on 32-bit systems and to 8-byte boundaries on 64-bit systems. This commit therefore checks for this same alignment in __call_rcu(), which used to check for two-byte alignment. Signed-off-by: Paul E. McKenney Cc: Geert Uytterhoeven Cc: Kirill A. Shutemov diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 3a8eec3ba1bd..673bcb3934a3 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -3122,7 +3122,9 @@ __call_rcu(struct rcu_head *head, rcu_callback_t func, unsigned long flags; struct rcu_data *rdp; - WARN_ON_ONCE((unsigned long)head & 0x1); /* Misaligned rcu_head! */ + /* Misaligned rcu_head! */ + WARN_ON_ONCE((unsigned long)head & (sizeof(void *) - 1)); + if (debug_rcu_head_queue(head)) { /* Probable double call_rcu(), so leak the callback. */ WRITE_ONCE(head->func, rcu_leak_callback);
Re: [PATCH tip/core/rcu 2/2] documentation: Record reason for rcu_head two-byte alignment
On Tue, Aug 23, 2016 at 04:30:47PM +0200, Geert Uytterhoeven wrote: > On Tue, Aug 23, 2016 at 4:23 PM, Paul E. McKenney > wrote: > > commit 89d39c83d193733ed5fff1c480cd42c9de1da404 > > Author: Paul E. McKenney > > Date: Tue Aug 23 06:51:47 2016 -0700 > > > > rcu: Tighted up __call_rcu() rcu_head alignment check > > > > Commit 720abae3d68ae ("rcu: force alignment on struct > > callback_head/rcu_head") forced the rcu_head (AKA callback_head) > > structure's alignment to pointer size, that is, to 4-byte boundaries on > > 32-bit systems and to 8-byte boundaries on 64-bit systems. This > > commit therefore checks for this same alignment in __call_rcu(), > > which used to check for two-byte alignment. > > > > Signed-off-by: Paul E. McKenney > > Cc: Geert Uytterhoeven > > Cc: Kirill A. Shutemov > > Tested-by: Geert Uytterhoeven Thank you! I have upgraded your Cc to Tested-by. ;-) Thanx, Paul