[RFC,0/4] Optimize mremap during mutual alignment within PMD

Hello,

I am posting this as an RFC for any feedback. I have tested them suitably and I
am continuing to test them.

These patches optimizes the start addresses in move_page_tables(). It addresses a
warning [1] that occurs due to a downward, overlapping move on a mutually-aligned
offset within a PMD during exec. By initiating the copy process at the PMD
level when such alignment is present, we can prevent this warning and speed up
the copying process at the same time. Linus Torvalds suggested this idea.

Please check the individual patches for more details.

thanks,

 - Joel

[1] https://lore.kernel.org/all/ZB2GTBD%2FLWTrkOiO@dhcp22.suse.cz/

Joel Fernandes (Google) (4):
mm/mremap: Optimize the start addresses in move_page_tables()
selftests: mm: Fix failure case when new remap region was not found
selftests: mm: Add a test for mutually aligned moves > PMD size
selftests: mm: Add a test for remapping to area immediately after
existing mapping

mm/mremap.c                              | 49 +++++++++++++++++
tools/testing/selftests/mm/mremap_test.c | 69 +++++++++++++++++++++---
2 files changed, 112 insertions(+), 6 deletions(-)

--
2.40.1.606.ga4b1b128d6-goog

On Wed, May 17, 2023 at 7:18 PM Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
>
> This warning will only trigger when there is mutual alignment in the
> move operation. A solution, as suggested by Linus Torvalds [2], is to
> initiate the copy process at the PMD level whenever such alignment is
> present.

So this patch is actually simpler than I thought it would be.

But I'm a bit nervous about it. In particular, it ends doing

        old_end = old_addr + len;
        ... expand old_addr/new_addr down to the pmd boundary ..
        return len + old_addr - old_end;        /* how much done */

doesn't that return value end up being nonsensical now?

In particular, I think it can return a *negative* value, because of
how old_addr was moved down, and the "now much done" might indeed be
"negative" in the sense that it failed the move even "before" the
original starting point.

And that negative value then ends up being a large positive one as an
"unsigned long", of course.

So I get the feeling that it wants something like

        if (old_addr + len < old_end)
                return 0;

there at the end.

But maybe there is something in there that guarantees that that case
never happens. I didn't think too deeply about it, I just felt this
looked odd.

               Linus

On Wed, May 17, 2023 at 09:12:07PM -0700, Linus Torvalds wrote:
> On Wed, May 17, 2023 at 7:18 PM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> >
> > This warning will only trigger when there is mutual alignment in the
> > move operation. A solution, as suggested by Linus Torvalds [2], is to
> > initiate the copy process at the PMD level whenever such alignment is
> > present.
> 
> So this patch is actually simpler than I thought it would be.
> 
> But I'm a bit nervous about it. In particular, it ends doing
> 
>         old_end = old_addr + len;
>         ... expand old_addr/new_addr down to the pmd boundary ..
>         return len + old_addr - old_end;        /* how much done */
> 
> doesn't that return value end up being nonsensical now?

Aargh, Sorry to miss that. Yes, it ends up being bogus in the case where the
loop broke out early due to failure (but only on the first PMD move failure
AFAICS). In the success case (or failures after the first PMD move), it does
not matter because old_addr is updated to what it was without the
optimization.

> In particular, I think it can return a *negative* value, because of
> how old_addr was moved down, and the "now much done" might indeed be
> "negative" in the sense that it failed the move even "before" the
> original starting point.
> 
> And that negative value then ends up being a large positive one as an
> "unsigned long", of course.
> 
> So I get the feeling that it wants something like
> 
>         if (old_addr + len < old_end)
>                 return 0;

I think that will fix it (thanks!). The main thing I think is to not mess up
the second call to move_page_tables() in mremap where it tries to move the
half-moved stuff back:

  move_page_tables(new_vma, new_addr, vma, old_addr, moved_len,  true);

There moved_len comes from the return value of the first call to
move_page_tables().

If we realigned, and then the first PMD alloc failed, moved_len might be
negative as you pointed. If the first PMD move passed, then there is no
issue as I mentioned above.

I will fix this in the next revision and also add a test case for this, I am
wondering how to test it without some kind of error-injection to make the
first PMD copy fail. In any case, I will try to hack my local kernel to test
that.

thanks,

 - Joel


> there at the end.
> 
> But maybe there is something in there that guarantees that that case
> never happens. I didn't think too deeply about it, I just felt this
> looked odd.
> 
>                Linus