Re: [PATCH 3/5] lib: lockless generic and arch independent page table (gpt) v2.

[Linus Torvalds]

Ok, so things are somewhat calm, and I'm trying to take time off to
see what's going on. And I'm not happy.

On Mon, Nov 10, 2014 at 10:28 AM,  <j.glisse@xxxxxxxxx> wrote:
>
> Page table is a common structure format most notably use by cpu mmu. The
> arch depend page table code has strong tie to the architecture which makes
> it unsuitable to be use by other non arch specific code.

Please don't call this thing a "generic page table".

It is no such thing. The *real* page tables are page tables. This is
some kind of "mapping lookup", and has nothing to do with page tables
as far as I can see. Why do you call it a page table?

Also, why isn't this just using our *existing* generic mapping
functionality, which already uses a radix tree, and has a lot of
lockless models? We already *have* something like that, and it's
called a "struct address_space".

And if you *just* want the tree, why don't you use "struct radix_tree_root".

And if it's generic, why do you have that odd insane conditional
locking going on?

In other words, looking at this, I just go "this is re-implementing
existing models, and uses naming that is actively misleading".

I think it's actively horrible, in other words. The fact that you have
one ACK on it already makes me go "Hmm". Is there some actual reason
why this would be called a page table, when even your explanation very
much clarifies that it is explicitly written to *not* be an actual
page table.

I also find it absolutely disgusting how you use USE_SPLIT_PTE_PTLOCKS
for this, which seems to make absolutely zero sense. So you're sharing
the config with the *real* page tables for no reason I can see.

I'm also looking at the "locking". It's insane. It's wrong, and
doesn't have any serialization. Using the bit operations for locking
is not correct. We've gotten over that years ago.

Rik, the fact that you acked this just makes all your other ack's be
suspect. Did you do it just because it was from Red Hat, or do you do
it because you like seeing Acked-by's with your name?

Anyway, this gets a NAK from me. Maybe I'm missing something, but I
think naming is supremely important, and I really don't see the point
of this. At a minimum, it needs a *hell* of a lot more explanations
for all it does. And quite frankly, I don't think that will be
sufficient, since the whole "bitops for locking" looks downright
buggy, and it's not at all clear why you want this in the first place
as opposed to just using gang lookups on the radix trees that we
already have, and that is well-tested and known to scale fine.

So really, it boils down to: why is this any better than radix trees
that are well-named, tested, and work?

                Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/