On Thu, Aug 5, 2021 at 3:32 PM Lukas Bulwahn <[email protected]> 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 <[email protected]>
> Reported-by: Mete Polat <[email protected]>
> Signed-off-by: Lukas Bulwahn <[email protected]>
> ---
> 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
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
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*.
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
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
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.
On Thu, Aug 05, 2021 at 04:02:22PM +0200, Arnd Bergmann wrote:
> On Thu, Aug 5, 2021 at 3:32 PM Lukas Bulwahn <[email protected]> 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 <[email protected]>
> > Reported-by: Mete Polat <[email protected]>
> > Signed-off-by: Lukas Bulwahn <[email protected]>
> > ---
> > 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