Re: [PATCH 0/5] mm: workingset: radix tree subtleties & single-page file refaults

[Linus Torvalds <torvalds@linux-foundation.org>]

On Wed, Oct 19, 2016 at 10:24 AM, Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> These patches make the radix tree code explicitely support and track
> such special entries, to eliminate the subtleties and to restore the
> thrash detection for single-page files.

Ugh. I'm not a huge fan. The patches may be good and be a cleanup in
one respect, but they make one of my least favorite parts of the radix
tree code worse.

The radix tree "tag" thing is really horribly confusing, and part of
it is that there are two totally different "tags": the externally
visible tags (separate array), and the magical internal tags (low bits
of the node pointers that tag the pointers as internal or exceptional
entries).

And I think this series actually makes things even *more* complicated,
because now the radix tree itself uses one magical entry in the
externally visible tags for its own internal logic. So now it's
*really* messed up - the external tags aren't entirely external any
more.

Maybe I'm mis-reading it, and I'm just confused by the radix tree
implementation? But if so, it's just another sign of just how
confusing things are.

The external tag array itself is also somewhat nasty, in that it
spreads out the tag bits for one entry maximally (ie different bits
are in different words) so you can't even clear them together. I know
why - it makes both iterating over a specific tag and any_tag_set()
simpler, but it does seem confusing to me how we spread out the data
almost maximally.

I really would love to see somebody take a big look at the two
different tagging methods. If nothing else, explain it to me.

Because maybe this series is all great, and my objection is just that
it makes it even harder for me to understand the code.

For example, could we do this simplification:

 - get rid of RADIX_TREE_TAG_LONGS entirely
 - get rid of CONFIG_BASE_SMALL entirely
 - just say that the tag bitmap is one unsigned long
 - so RADIX_TREE_MAP_SIZE is just BITS_PER_LONG

and then at least we'd get rid of the double array and the confusion
about loops that are actually almost never loops. Because right now
RADIX_TREE_TAG_LONGS is usually 1, but is 2 if you're a 32-bit
platform with !CONFIG_BASE_SMALL. So you need those loops, but it all
looks almost entirely pointless.

I just get the feeling that we have already have unnecessary
complexity here, and that this patch series makes the code even more
impenetrable.

Comments?

                Linus