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

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

[Lukas Bulwahn]

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.").

Reported-by: Randy Dunlap <rdunlap@infradead.org>
Reported-by: Mete Polat <metepolat2000@gmail.com>
Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
applies cleanly on next-20210804, but only x86 compile-checked.

Michel, Davidlohr, Jesper, David, please ack.

Mete, you might want to re-run your RBT test suite for this change.

Andrew, once acked, please pick this minor cleanup into your tree.

 include/linux/rbtree.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/rbtree.h b/include/linux/rbtree.h
index d31ecaf4fdd3..e9390be1ba67 100644
--- 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;
-- 
2.17.1

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

[Arnd Bergmann]

On Thu, Aug 5, 2021 at 3:32 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> 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.").
>
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Reported-by: Mete Polat <metepolat2000@gmail.com>
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---
> applies cleanly on next-20210804, but only x86 compile-checked.
>
> Michel, Davidlohr, Jesper, David, please ack.
>
> Mete, you might want to re-run your RBT test suite for this change.
>
> Andrew, once acked, please pick this minor cleanup into your tree.

Do you know why it needed the extra alignment on cris at the time?

The revert would appear to change the alignment to 16 bits instead
of 32 bits on m68k as well (not 8 bits as on cris), but I don't know if that
can cause problems there.

        Arnd

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

[Lukas Bulwahn]

On Thu, Aug 5, 2021 at 4:02 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Aug 5, 2021 at 3:32 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> 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.").
> >
> > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > Reported-by: Mete Polat <metepolat2000@gmail.com>
> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > ---
> > applies cleanly on next-20210804, but only x86 compile-checked.
> >
> > Michel, Davidlohr, Jesper, David, please ack.
> >
> > Mete, you might want to re-run your RBT test suite for this change.
> >
> > Andrew, once acked, please pick this minor cleanup into your tree.
>
> Do you know why it needed the extra alignment on cris at the time?
>

I basically only know what David Woodhouse wrote in his commit message:

"... allegedly it was necessary for struct address_space on CRIS,
because it otherwise ended up being only byte-aligned."

> The revert would appear to change the alignment to 16 bits instead
> of 32 bits on m68k as well (not 8 bits as on cris), but I don't know if that
> can cause problems there.
>

I guess this patch would need to be exposed to testing on the various
architectures. I personally can only serve with testing x86 and maybe
with a bit of some arm architecture.

Should we CC the various architecture mailing lists and maintainers on
such a patch, or is that just noise for these maintainers?

Or could we just quickly pick this up into anyone's (Andrew's) tree
for linux-next and expose it to wider testing on other architectures
that way? If we see the patch cause failures (rbtree is widely used in
mm, fs, etc. and hence boot up would probably already fail), then we
know and we can adjust the comment with a patch v2 to know for the
future that this alignment is also relevant for other architectures.

Lukas

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

[Davidlohr Bueso]

On 2021-08-05 07:02, Arnd Bergmann wrote:
> The revert would appear to change the alignment to 16 bits instead
> of 32 bits on m68k as well (not 8 bits as on cris), but I don't know if 
> that
> can cause problems there.

Yeah I tried this a while back and it broke m68k, so it was a no go:

https://lore.kernel.org/lkml/CAMuHMdXeZvJ0X6Ah2CpLRoQJm+YhxAWBt-rUpxoyfOLTcHp+0g@mail.gmail.com/

Thanks,
Davidlohr

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

[Peter Zijlstra]

On Thu, Aug 05, 2021 at 08:02:28AM -0700, Davidlohr Bueso wrote:
> On 2021-08-05 07:02, Arnd Bergmann wrote:
> > The revert would appear to change the alignment to 16 bits instead
> > of 32 bits on m68k as well (not 8 bits as on cris), but I don't know if
> > that
> > can cause problems there.
> 
> Yeah I tried this a while back and it broke m68k, so it was a no go:
> 
> https://lore.kernel.org/lkml/CAMuHMdXeZvJ0X6Ah2CpLRoQJm+YhxAWBt-rUpxoyfOLTcHp+0g@mail.gmail.com/

I'm still thinking that any architecture that doesn't respect natural
alignment is playing with fire. For giggles we should put a runtime
alignment check in READ_ONCE() and see what goes *bang*.

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

[Mete Polat]

On Thu, Aug 05, 2021 at 08:02:28AM -0700, Davidlohr Bueso wrote:
> On 2021-08-05 07:02, Arnd Bergmann wrote:
> > The revert would appear to change the alignment to 16 bits instead
> > of 32 bits on m68k as well (not 8 bits as on cris), but I don't know if
> > that
> > can cause problems there.
> 
> Yeah I tried this a while back and it broke m68k, so it was a no go:
> 
> https://lore.kernel.org/lkml/CAMuHMdXeZvJ0X6Ah2CpLRoQJm+YhxAWBt-rUpxoyfOLTcHp+0g@mail.gmail.com/

The problem is that the field '__rb_parent_color' in struct rb_node is
storing the color AND the pointer to the parent node at the same time.
The color is stored in the least significant bit which is fine when
rb_node is at least 16-bit aligned. I guess, it does not work on m68k
because the makro

#define __rb_parent(pc)    ((struct rb_node *)(pc & ~3))

used to retrieve the parent pointer zeros the first two bits, not only
the first one.

Maybe the effiency to store this one color bit in another field was
required in the early days but I think moving the color to a seperate
field is really the better way to go. It also makes reasoning about the
algorithm easier.

I will create a patch.

Mete
> 
> Thanks, Davidlohr

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

[Michel Lespinasse]

On Thu, Aug 05, 2021 at 07:20:26PM +0200, Mete Polat wrote:
> On Thu, Aug 05, 2021 at 08:02:28AM -0700, Davidlohr Bueso wrote:
> > On 2021-08-05 07:02, Arnd Bergmann wrote:
> > > The revert would appear to change the alignment to 16 bits instead
> > > of 32 bits on m68k as well (not 8 bits as on cris), but I don't know if
> > > that
> > > can cause problems there.
> > 
> > Yeah I tried this a while back and it broke m68k, so it was a no go:
> > 
> > https://lore.kernel.org/lkml/CAMuHMdXeZvJ0X6Ah2CpLRoQJm+YhxAWBt-rUpxoyfOLTcHp+0g@mail.gmail.com/
> 
> The problem is that the field '__rb_parent_color' in struct rb_node is
> storing the color AND the pointer to the parent node at the same time.
> The color is stored in the least significant bit which is fine when
> rb_node is at least 16-bit aligned. I guess, it does not work on m68k
> because the makro
> 
> #define __rb_parent(pc)    ((struct rb_node *)(pc & ~3))
> 
> used to retrieve the parent pointer zeros the first two bits, not only
> the first one.
> 
> Maybe the effiency to store this one color bit in another field was
> required in the early days but I think moving the color to a seperate
> field is really the better way to go. It also makes reasoning about the
> algorithm easier.
> 
> I will create a patch.

I think moving the color to a separate word would be costly, both in space
(growing the struct rb_node) and in time. Feel free to try it, but I would
expect the rbtree performance tests to regress significantly.

__rb_parent() could probably be modified - it only needs to mask one bit,
I'm not sure why it masks two.

As to what would happen on 68k... hard to say, but I expect it should
be fine (if the compiler cared for the structs to be aligned, it
should do it on its own). Still, not sure how to test that either.

-- 
Michel "walken" Lespinasse

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

[Peter Zijlstra]

On Fri, Aug 06, 2021 at 01:52:45AM -0700, Michel Lespinasse wrote:
> I think moving the color to a separate word would be costly, both in space
> (growing the struct rb_node) and in time. Feel free to try it, but I would
> expect the rbtree performance tests to regress significantly.
> 
> __rb_parent() could probably be modified - it only needs to mask one bit,
> I'm not sure why it masks two.
> 
> As to what would happen on 68k... hard to say, but I expect it should
> be fine (if the compiler cared for the structs to be aligned, it
> should do it on its own). Still, not sure how to test that either.

Somewhere, on the infinite length TODO list, I have an item to implement
threaded RB trees, which would need the LSB of the left and right words
too.

In general, the kernel is overflowing with code that (ab)uses the LSBs
of pointers and relies on at least natural alignment. There is
absolutely no point in fixing just this one instance.

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

[Jesper Nilsson]

On Thu, Aug 05, 2021 at 04:02:22PM +0200, Arnd Bergmann wrote:
> On Thu, Aug 5, 2021 at 3:32 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> 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.").
> >
> > Reported-by: Randy Dunlap <rdunlap@infradead.org>
> > Reported-by: Mete Polat <metepolat2000@gmail.com>
> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > ---
> > applies cleanly on next-20210804, but only x86 compile-checked.
> >
> > Michel, Davidlohr, Jesper, David, please ack.
> >
> > Mete, you might want to re-run your RBT test suite for this change.
> >
> > Andrew, once acked, please pick this minor cleanup into your tree.
> 
> Do you know why it needed the extra alignment on cris at the time?

The problem for CRIS was that the architecture always had packed
structs (there was no way to avoid it in GCC) and CRIS could all data
on any byte boundary.

That was ok for normal pointers in structs, all pointers would be allocated
inside the same page and it didn't matter that they were aligned on odd byte
addresses for the CPU (except for being a little slower)

However, when the lowest bits were used as flags, that would clash on CRIS since
all bits were valid and possible address bits, so any sub-struct in a struct might be
aligned on an odd address, and using the pointer to the sub-struct would inadvertently
set the flag bits.

That's why just adding a forced alignment on the sub-struct fixed the problem,
since the compiler would respect such alignments, and thus leave the lower bits
free for trickery.

> The revert would appear to change the alignment to 16 bits instead
> of 32 bits on m68k as well (not 8 bits as on cris), but I don't know if that
> can cause problems there.
> 
>         Arnd

/^JN - Jesper Nilsson
--
                  Jesper Nilsson -- jesper_at_jni.nu