Re: [PATCH v4 3/9] mm: pagewalk: Don't split transhuge pmds when a pmd_entry is present

From: Thomas HellstrÃm (VMware)
Date: Wed Oct 09 2019 - 21:10:04 EST

On 10/10/19 1:51 AM, Linus Torvalds wrote:
On Wed, Oct 9, 2019 at 3:31 PM Thomas HellstrÃm (VMware)
<thomas_os@xxxxxxxxxxxx> 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.

Thank you for your patience!

Yes, I very well *do* understand that we need to split a huge pmd to walk the pte range, and I've never been against removing that conditional. What I've said is that it is pointless anyway, because we've already verified that the only path coming from the pmd_entry (with the patch applied) has the pmd *already split* and stable.

Your original patch does exactly the same!

So please let's move on from the splitting issue. We don't disagree here. The conditional is gone to never be resurrected.


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.

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.

Agreed.


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?

Indeed.


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.

Sure. I'm fine with this. So for next spin, I'll skip this patch, and do the positive value rework as part of the prerequisites for the huge page enablement. IIRC there is another user of positive values that should be trivial to fix.

Does that sound OK?

Thanks,

Thomas