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
