Re: [PATCH 2/2] radix-tree: Fix optimisation problem

From: Linus Torvalds
Date: Sat Sep 24 2016 - 16:21:51 EST

On Fri, Sep 23, 2016 at 1:16 PM, Matthew Wilcox <mawilcox@xxxxxxxxxxxxx> wrote:
> if (radix_tree_is_internal_node(entry)) {
> - unsigned long siboff = get_slot_offset(parent, entry);
> + unsigned long siboff = get_slot_offset(parent,
> + (void **)entry_to_node(entry));

I feel that it is *this* part that I think needs a huge honking comment.

If you are going to make get_slot_offset() different, then you could
just rewrite get_slot_offset() to do

unsigned long diff = (unsigned long) slot - (unsigned
return diff / sizeof(void *);

and add a comment to say "don't do this as a pointer diff, because
'slot' may not be an aligned pointer". No BUG_ON() necessary, because
it "just works".

At that point, gcc should just generate the right code, because it
doesn't see it as a pointer subtraction followed by a pointer

And yes, that crazy " (void **)entry_to_node(entry)" fixes it *too*,
but it needs a *comment*.

Why is that special, when all the other uses of get_slot_offset()
don't have that? *That* is what should be explained. Not some internal

That said, if this code isn't even used, as Konstantin says (THP
selects it - doesn't THP use it?), then the fix really should be to
just remove the odd code instead of adding to it.

Looking around for uses that set "order" to anything but zero, I
really don't see it. So maybe we should just do *that* trivial thing
instead, and remove CONFIG_RADIX_TREE_MULTIORDER, since it's appears
to be buggy and always has been.