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
