Message-ID: <CAHk-=wg6n_nGRtJd4MeXZrA5QrrVViJeO4x2w37KDbcDmTh3dg@mail.gmail.com>
Date:   Wed, 9 Oct 2019 16:51:19 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Thomas Hellström (VMware) 
        <thomas_os@...pmail.org>
Cc:     Thomas Hellstrom <thellstrom@...are.com>,
        "Kirill A. Shutemov" <kirill@...temov.name>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux-MM <linux-mm@...ck.org>,
        Matthew Wilcox <willy@...radead.org>,
        Will Deacon <will.deacon@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Rik van Riel <riel@...riel.com>,
        Minchan Kim <minchan@...nel.org>,
        Michal Hocko <mhocko@...e.com>,
        Huang Ying <ying.huang@...el.com>,
        Jérôme Glisse <jglisse@...hat.com>
Subject: Re: [PATCH v4 3/9] mm: pagewalk: Don't split transhuge pmds when a
 pmd_entry is present

On Wed, Oct 9, 2019 at 3:31 PM Thomas Hellström (VMware)
<thomas_os@...pmail.org> wrote:
>
> On 10/9/19 10:20 PM, Linus Torvalds wrote:
> >
> > You *have* to call split_huge_pmd() if you're doing to call the
> > pte_entry() function.
> >
> > End of story.
>
> So is it that you want pte_entry() to be strictly called for *each*
> virtual address, even if we have a pmd_entry()?

Thomas, you're not reading the emails.

You are conflating two issues:

 (a) the conditional split_huge_pmd()

 (b) the what to do about the return value of pmd_entry().

and I'm now FOR THE LAST TIME telling you that (a) is completely
wrong, buggy crap. It will not happen. I missed that part of the patch
in my original read-through, because it was so senseless.

Get it through you head. The "conditional split_huge_pmd()" thing is
wrong, wrong, wrong.

And it is COMPLETELY WRONG regardless of any "each virtual address"
thing that you keep bringing up. The "each virtual address" argument
is irrelevant, pointless, and does not matter.

So stop bringing that thing up. Really. The conditional
split_huge_pmd() is wrong.

It's wrong,  because the whole notion of "don't split pmd and then
call the pte walker" is completely idiotic and utterly senseless,
because without the splitting the pte's DO NOT EXIST.

What part of that is not getting through?

> In that case I completely follow your arguments, meaning we skip this
> patch completely?

Well, yes and no.

The part of the patch that is complete and utter garbage, and that you
really need to *understand* why it's complete and utter garbage is
this part:

                if (!ops->pte_entry)
                        continue;
-
-               split_huge_pmd(walk->vma, pmd, addr);
+               if (!ops->pmd_entry)
+                       split_huge_pmd(walk->vma, pmd, addr);
                if (pmd_trans_unstable(pmd))
                        goto again;
                err = walk_pte_range(pmd, addr, next, walk);

Look, you cannot call "walk_pte_range()" without calling
split_huge_pmd(), because walk_pte_range() cannot deal with a huge
pmd.

walk_pte_range() does that pte_offset_map() on the pmd (well, with
your other patch in place it does the locking version), and then it
walks every pte entry of it. But that cannot possibly work if you
didn't split it.

See what I've been trying tog tell you? YOU CANNOT MAKE THAT SPLITTING
BE CONDITIONAL!

What can be done - and what the line above it does - is to skip
walking entirely. We do it when we don't have a pte_entry walker at
all, obviously.

But we can't do is "oh, we had a pmd walker, so now I'll skip
splitting". That is insane, and cannot possibly make sense.

Now, that gets us back to the (b) part above - what to do about the
return value of "pmd_entry()".

What *would* make sense, for example, is saying "ok, pmd_entry()
already handled this, so we'll just continue, and not bother with the
pte_range() at all".

Note how that is VERY VERY different from "let's not split".

Yes, for that case we also don't split, but we don't split because
we're not even walking it. See the difference?

Not splitting and then walking: wrong, insane, and not going to happen.

Nor splitting because we're not going to walk it: sane, and we already
have one such case.

> My take on the change was that pmd_entry() returning 0 would mean we
> could actually skip the pte level completely and nothing would otherwise
> pass down to the next level for which split_huge_pmd() wasn't a NOP,
> similar to how pud_entry does things.

And that would have been a sensible operation, and yes, you had that

+                       if (!err)
+                               continue;

and that was it. Fine.

BUT.

That was not all that your patch did. Kirill felt that your
PAGE_WALK_FALLBACK thing was hacky, which is why I looked more at the
patch to see what it really wanted to do, and noticed that crazy
conditional splitting that didn't make sense, and has *NOTHING* to do
with your "skip the level completely".

I _agree_ that skipping the level completely makes sense.

But the "don't split and then don't skip the level" makes zero sense
what-so-ever. Ever. Under no circumstances can that be valid as per
above.

There's also the argument that right now, there are no users that
actually want to skip the level.

Even your use case doesn't really want that, because in your use-case,
the only case that would do it is the error case that isn't supposed
to happen.

And if it *is* supposed to happen, in many ways it would be more
sensible to just return a positive value saying "I took care of it, go
on to the next entry", wouldn't you agree?

Now, I actually tried to look through every single existing pmd_entry
case, because I wanted to see who is returning positive values right
now, and which of them might *want* to say "ok, I handled it" or "now
do the pte walking".

Quite a lot of them seem to really want to do that "ok, now do the pte
walking", because they currently do it inside the pmd function because
of how the original interface was badly done. So while we _currently_
do not have a "ok, I did everything for this pmd, skip it" vs a "ok,
continue with pte" case, it clearly does make sense. No question about
it.

I did find one single user of a positive return value:
queue_pages_pte_range() returns

 * 0 - pages are placed on the right node or queued successfully.
 * 1 - there is unmovable page, and MPOL_MF_MOVE* & MPOL_MF_STRICT were
 *     specified.
 * -EIO - only MPOL_MF_STRICT was specified and an existing page was already
 *        on a node that does not follow the policy.

to match queue_pages_range(), but that function has two callers:

 - the first ignores the return value entirely

 - the second would be perfectly fine with -EBUSY as a return value
instead, which would actually make more sense.

Everybody else returns 0 or a negative error, as far as I can tell.

End result:

 (a) right now nobody wants the "skip" behavior. You think you'll
eventually want it

 (b) right now, the "return positive value" is actually a horribly
ugly pointless hack, which could be made to be an error value and
cleaned up in the process

 (c) to me that really argues that we should just make the rule be

     - negative error means break out with error

     - 0 means continue down the next level

     - positive could be trivially be made to just mean "ok, I did it,
you can just continue".

and I think that would make a lot of sense.

So I do think we can get rid of the PAGE_WALK_FALLBACK hack entirely.
It becomes "return 0", which is what everybody really does right now.
If there's a pte_entry(), it will split and call that pte_entry for
each pte, the way it does now. Sensible and simple.

And then - for _future_ uses - we can add that "positive return value
means that I already handled it" and the walker would just skip to the
next entry.

But - again - in no alternate reality, past, future or present, does
it make sense to skip the splitting if you walk pte's.

               Linus

Powered by blists - more mailing lists