Re: [PATCH v2] rbtree: remove unneeded explicit alignment in struct rb_node

[Mete Polat]

On Mon, Aug 09, 2021 at 02:25:35PM +0200, Geert Uytterhoeven wrote:
> Hi Mete,
> 
> On Fri, Aug 6, 2021 at 5:22 PM Mete Polat <metepolat2000@xxxxxxxxx> wrote:
> > Commit e977145aeaad ("[RBTREE] Add explicit alignment to sizeof(long) for
> > struct rb_node.") adds an explicit alignment to the struct rb_node due to
> > some speciality of the CRIS architecture.
> >
> > The support for the CRIS architecture was removed with commit c690eddc2f3b
> > ("CRIS: Drop support for the CRIS port")
> >
> > So, remove this now unneeded explicit alignment in struct rb_node as well.
> >
> > This basically reverts commit e977145aeaad ("[RBTREE] Add explicit
> > alignment to sizeof(long) for struct rb_node.").
> >
> > The rbtree node color is stored in the LSB of '__rb_parent_color'.
> > Only mask the first bit in '__rb_parent()', otherwise it modifies the
> > node's parent address on m68k.
> >
> > Reported-by: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
> > Reported-by: Mete Polat <metepolat2000@xxxxxxxxx>
> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@xxxxxxxxx>
> > Signed-off-by: Mete Polat <metepolat2000@xxxxxxxxx>
> > ---
> > I have tested it on x86, but not on m68k. Can you ack that Geert?
> 
> Unfortunately not.  Just booting on ARAnyM gives:
> 
> Unable to handle kernel access at virtual address 84abc227
> Oops: 00000000
> Modules linked in:
> PC: [<b45b0005>] 0xb45b0005
> SR: 2700  SP: fc24ede1  a2: 01322b40
> d0: b45aff7c    d1: 00002708    d2: 00000000    d3: 0033a6a4
> d4: 00000005    d5: 95847357    a0: b45b0005    a1: 000f4240
> Process sh (pid: 402, task=672891a8)
> Frame format=7 eff addr=b45b0005 ssw=0546 faddr=b45b0005
> wb 1 stat/addr/data: 0000 00000000 00000000
> wb 2 stat/addr/data: 0000 00000000 00000000
> wb 3 stat/addr/data: 0000 b45b0005 00000000
> push data: 00000000 00000000 00000000 00000000
> Stack from 0131debc:
>         0004ddf4 0122e0a4 0122e0a4 00477c26 00000000 00000000 00000005 95847357
>         00002708 0004558e 0033ab30 00000002 01003180 0047273c 0131df88 0033a6a4
>         00477c3a 00477c24 0004dfee 00000005 95847357 00002708 0000000f 00000001
>         9915a000 0000000d 0004d430 01322b40 00000001 00002604 00000000 00006e50
>         00000001 00000000 0004560c 0000000d 00000000 00000000 efc3aa78 efc3aa78
>         c0158c0c 8009ff68 0047273c efc3aa64 c01537a8 0131df8c 000456b6 0047273c
> Call Trace: [<0004ddf4>] __hrtimer_run_queues.constprop.0+0xba/0x144
>  [<00002708>] calibrate_delay+0xd6/0x18a
>  [<0004558e>] __irq_wake_thread+0x0/0x40
>  [<0033ab30>] printk+0x0/0x18
>  [<0033a6a4>] warn_slowpath_fmt+0x0/0x62
>  [<0004dfee>] hrtimer_run_queues+0xb4/0xc0
>  [<00002708>] calibrate_delay+0xd6/0x18a
>  [<0004d430>] update_process_times+0x38/0x8c
>  [<00002604>] wait_for_initramfs+0x30/0x58
>  [<00006e50>] mfp_timer_c_handler+0x2c/0x3c
>  [<0004560c>] __handle_irq_event_percpu+0x3e/0xcc
>  [<000456b6>] handle_irq_event_percpu+0x1c/0x52
>  [<0004570a>] handle_irq_event+0x1e/0x30
>  [<00047aa2>] handle_simple_irq+0x4a/0x4e
>  [<00045062>] handle_irq_desc+0x1e/0x28
>  [<00002cac>] do_IRQ+0x20/0x32
>  [<00002bcc>] user_irqvec_fixup+0xc/0x14
>  [<0010c00b>] mb_cache_create+0x47/0xdc
> 
> Code: dde0 dde0 dde0 dde0 dde0 dde0 dde0 dde0 Bad PC value.
> Disabling lock debugging due to kernel taint
> Kernel panic - not syncing: Aiee, killing interrupt handler!
> ---[ end Kernel panic - not syncing: Aiee, killing interrupt handler! ]---
> 
> > --- a/include/linux/rbtree.h
> > +++ b/include/linux/rbtree.h
> > @@ -25,8 +25,7 @@ struct rb_node {
> >         unsigned long  __rb_parent_color;
> >         struct rb_node *rb_right;
> >         struct rb_node *rb_left;
> > -} __attribute__((aligned(sizeof(long))));
> > -    /* The alignment might seem pointless, but allegedly CRIS needs it */
> > +};
> >
> >  struct rb_root {
> >         struct rb_node *rb_node;
> > diff --git a/include/linux/rbtree_augmented.h b/include/linux/rbtree_augmented.h
> > index d1c53e9d8c75..94b6a0f4499e 100644
> > --- a/include/linux/rbtree_augmented.h
> > +++ b/include/linux/rbtree_augmented.h
> > @@ -145,7 +145,7 @@ RB_DECLARE_CALLBACKS(RBSTATIC, RBNAME,                                            \
> >  #define        RB_RED          0
> >  #define        RB_BLACK        1
> >
> > -#define __rb_parent(pc)    ((struct rb_node *)(pc & ~3))
> > +#define __rb_parent(pc)    ((struct rb_node *)(pc & ~1))
> 
> Are you sure you can do this, i.e. that nothing else relies on the
> alignment to (at least) 4 bytes?

I am quite sure that the RBT algorithm itself only relies on the LSB,
but I am unsure yet if RCU requires this natural alignment as Peter
mentioned.

> 
> There is another definition of rb_parent().  If I make a similar change there:
> 
> --- a/include/linux/rbtree.h
> +++ b/include/linux/rbtree.h
> @@ -31,7 +31,7 @@ struct rb_root {
>         struct rb_node *rb_node;
>  };
> 
> -#define rb_parent(r)   ((struct rb_node *)((r)->__rb_parent_color & ~3))
> +#define rb_parent(r)   ((struct rb_node *)((r)->__rb_parent_color & ~1))
> 
>  #define RB_ROOT        (struct rb_root) { NULL, }
>  #define        rb_entry(ptr, type, member) container_of(ptr, type, member)
> 
> the system boots on ARAnyM, but I prefer to be a bit cautious...

Ah yes I forgot that the same makro is also defined in the other header
file as well; and similary in some other locations:

./scripts/gdb/linux/rbtree.py:
parent = gdb.Value(node['__rb_parent_color'] & ~3)
./tools/include/linux/rbtree.h:
#define rb_parent(r)   ((struct rb_node *)((r)->__rb_parent_color & ~3))


> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
> 
> 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