lkml.org 
[lkml]   [2023]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v4 1/7] mm/mremap: Optimize the start addresses in move_page_tables()
From
Hi Lorenzo,

On 6/20/23 07:02, Lorenzo Stoakes wrote:
> On Mon, Jun 19, 2023 at 11:55:08AM -0400, Joel Fernandes wrote:
>> Hi Lorenzo,
>> Thanks for the review! I replied below:
>>
>> On 6/17/23 18:49, Lorenzo Stoakes wrote:
>>> On Wed, May 31, 2023 at 10:08:01PM +0000, Joel Fernandes (Google) wrote:
>>>> Recently, we see reports [1] of a warning that triggers due to
>>>> move_page_tables() doing a downward and overlapping move on a
>>>> mutually-aligned offset within a PMD. By mutual alignment, I
>>>> mean the source and destination addresses of the mremap are at
>>>> the same offset within a PMD.
>>>>
>>>> This mutual alignment along with the fact that the move is downward is
>>>> sufficient to cause a warning related to having an allocated PMD that
>>>> does not have PTEs in it.
>>>>
>>>> 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. Implementing this approach will not only prevent the warning
>>>> from being triggered, but it will also optimize the operation as this
>>>> method should enhance the speed of the copy process whenever there's a
>>
>> [...]
>>
>>>> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>>>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>>>> ---
>>>> mm/mremap.c | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 61 insertions(+)
>>>>
>>>> diff --git a/mm/mremap.c b/mm/mremap.c
>>>> index 411a85682b58..bf355e4d6bd4 100644
>>>> --- a/mm/mremap.c
>>>> +++ b/mm/mremap.c
>>>> @@ -478,6 +478,51 @@ static bool move_pgt_entry(enum pgt_entry entry, struct
>>>> return moved;
>>>> }
>>>>
>>>> +/*
>>>> + * A helper to check if a previous mapping exists. Required for
>>>> + * move_page_tables() and realign_addr() to determine if a previous mapping
>>>> + * exists before we can do realignment optimizations.
>>>> + */
>>>> +static bool can_align_down(struct vm_area_struct *vma, unsigned long addr_to_align,
>>>> + unsigned long mask)
>>>> +{
>>>> + unsigned long addr_masked = addr_to_align & mask;
>>>> + struct vm_area_struct *prev = NULL, *cur = NULL;
>>>> +
>>>> + /*
>>>> + * If @addr_to_align of either source or destination is not the beginning
>>>> + * of the corresponding VMA, we can't align down or we will destroy part
>>>> + * of the current mapping.
>>>> + */
>>>> + if (vma->vm_start != addr_to_align)
>>>> + return false;
>>>
>>> See below, I think we can eliminate this check.
>>>
>>>> +
>>>> + /*
>>>> + * Find the VMA before @vma to see if it subsumes the masked address.
>>>> + * The mmap write lock is held here so the lookup is safe.
>>>> + */
>>>> + cur = find_vma_prev(vma->vm_mm, vma->vm_start, &prev);
>>>> + if (WARN_ON_ONCE(cur != vma))
>>>> + return false;
>>>> +
>>>> + return !prev || prev->vm_end <= addr_masked;
>>>
>>> This is a bit clunky, and I don't think we need the WARN_ON_ONCE() check if
>>> we're under the mmap_lock.
>>>
>>> How about something like:-
>>>
>>> return find_vma_intersection(vma->mm, addr_masked, vma->vm_start) == NULL;
>>>
>>> Which explicitly asserts that the range in [addr_masked, vma->vm_start) is
>>> empty.
>>>
>>> But actually, we should be able to go further and replace the previous
>>> check with:-
>>>
>>> return find_vma_intersection(vma->mm, addr_masked, addr_to_align) == NULL;
>>>
>>> Which will fail if addr_to_align is offset within the VMA.
>>
>> Your suggestion would mean that we do a full VMA search starting from the
>> root. That would not be a nice thing if say we've 1000s of VMAs?
>>
>> Actually Liam told me to use find_vma_prev() because given a VMA, the maple
>> tree would not have to work that hard for the common case to find the
>> previous VMA. Per conversing with him, there is a chance we may have to go
>> one step above in the tree if we hit the edge of a node, but that's not
>> supposed to be the common case. In previous code, the previous VMA could
>> just be obtained using the "previous VMA" pointer, however that pointer has
>> been remove since the maple tree changes and given a VMA, going to the
>> previous one using the maple tree is just as fast (as I'm told).
>
> As far as I can tell, find_vma_prev() already does a walk? I mean this is
> equivalent to find_vma() only retrieving the previous VMA right? I defer to
> Liam, but I'm not sure this would be that much more involved? Perhaps he
> can comment.
>
> An alternative is to create an iterator and use vma_prev(). I find it
> extremely clunky that we search for a VMA we already possess (and it's
> previous one) while not needing the the former.
>
> I'm not hugely familiar with the maple tree (perhaps Liam can comment) but
> I suspect that'd be more performant if that's the concern. Either way I
> would be surprised if this is the correct approach.

I see your point. I am not sure myself, the maple tree functions for both APIs
are indeed similar. We already have looked up the VMA being aligned down. If
there is a way to get the previous VMA quickly, given an existing VMA, I can
incorporate that change.

Ideally, if I had access to the ma_state used for lookup of the VMA being
aligned down, I could perhaps reuse that somehow. But when I checked, that
seemed a lot more invasive to pass that state down to these align functions.

But there is a merit to your suggestion itself in the sense it cuts down a few
more lines of code.

>> Considering this, I would keep the code as-is and perhaps you/we could
>> consider the replacement with another API in a subsequent patch as it does
>> the job for this patch.
>
> See above. I don't think this kind of comment is helpful in code
> review. Your disagreement above suffices, I've responded to it and of
> course if there is no other way this is fine.
>
> But I'd be surprised, and re-looking up a VMA we already have is just
> horrid. It's not really a nitpick, it's a code quality issue in my view.
>
> In any case, let's please try to avoid 'if you are bothered, write a follow
> up patch' style responses. If you disagree with something just say so, it's
> fine! :)

I wasn't disagreeing :) Just saying that the find_vma_prev() suggested in a
previous conversation with Liam fixes the issue (and has been tested a lot in
this series, on my side) so I was hoping to stick to that and we could iterate
more on that in the future.

However, after taking a deeper look at the maple tree, I'd like to give the
find_vma_intersection() option at least a try (with appropriate attribution to you).

Apologies if the response style in my previous email came across badly. That
wasn't my intent and I will try to improve myself.

[..]

>>>> + realign_addr(&old_addr, vma, &new_addr, new_vma, PMD_MASK);
>>>> + }
>>>> +
>>>> if (is_vm_hugetlb_page(vma))
>>>> return move_hugetlb_page_tables(vma, new_vma, old_addr,
>>>> new_addr, len);
>>>> @@ -565,6 +619,13 @@ unsigned long move_page_tables(struct vm_area_struct *vma,
>>>>
>>>> mmu_notifier_invalidate_range_end(&range);
>>>>
>>>> + /*
>>>> + * Prevent negative return values when {old,new}_addr was realigned
>>>> + * but we broke out of the above loop for the first PMD itself.
>>>> + */
>>>> + if (len + old_addr < old_end)
>>>> + return 0;
>>>> +
>>>
>>> I find this a little iffy, I mean I see that if you align [old,new]_addr to
>>> PMD, then from then on in you're relying on the fact that the loop is just
>>> going from old_addr (now aligned) -> old_end and thus has the correct
>>> length.
>>>
>>> Can't we just fix this issue by correcting len? If you take my review above
>>> which checks len in [maybe_]realign_addr(), you could take that as a
>>> pointer and equally update that.
>>>
>>> Then you can drop this check.
>>
>> The drawback of adjusting len is it changes what move_page_tables() users
>> were previously expecting.
>>
>> I think we should look at the return value of move_page_tables() as well,
>> not just len independently.
>>
>> len is what the user requested.
>>
>> "len + old_addr - old_end" is how much was actually copied and is the return value.
>>
>> If everything was copied, old_addr == old_end and len is unchanged.
>
> Ah yeah I see, sorry I missed the fact we're returning a value, that does
> complicate things...
>
> If we retain the hugetlb logic, then we could work around the issue with
> that instance of len by storing the 'actual length' of the range in
> a new var actual_len and passing that.
>
> If we choose to instead just not do this for hugetlb (I wonder if the
> hugetlb handling code actually does the equivalent of this since surely
> these pages have to be handled a PMD at a time?) then we can drop the whole
> actual_len idea [see below on response to hugetlb thing].

Thanks. Yes, you are right. We should already b good with hugetlb handling as
it does appear that hugetlb_move_page_tables() does copy by huge_page_size(h),
so the old_addr should already be PMD-aligned for it to be able to do that.

[..]

>>>> return len + old_addr - old_end; /* how much done */
>>>> }
>>> Also I am concerned in the hugetlb case -> len is passed to
>>> move_hugetlb_page_tables() which is now strictly incorrect, I wonder if
>>> this could cause an issue?
>>>
>>> Correcting len seems the neat way of addressing this.
>>
>> That's a good point. I am wondering if we can just change that from:
>>
>> if (is_vm_hugetlb_page(vma))
>> return move_hugetlb_page_tables(vma, new_vma, old_addr,
>> new_addr, len);
>>
>> to:
>> if (is_vm_hugetlb_page(vma))
>> return move_hugetlb_page_tables(vma, new_vma, old_addr,
>> new_addr, old_addr - new_addr);
>>
>> Or, another option is to turn it off for hugetlb by just moving:
>>
>> if (len >= PMD_SIZE - (old_addr & ~PMD_MASK))
>> realign_addr(...);
>>
>> to after:
>>
>> if (is_vm_hugetlb_page(vma))
>> return move_hugetlb_page_tables(...);
>>
>> thanks,
>
> I think the actual_len solution should sort this right? If not maybe better
> to be conservative and disable for the hugetlb case (I'm not sure if this
> would help given you'd need to be PMD aligned anyway right?), so not to
> hold up the series.
>
> If we do decide not to include hugetlb (the endless 'special case' for so
> much code...) in this then we can drop the actual_len idea altogether.
>
> (Yes I realise it's ironic I'm suggesting deferring to a later patch here
> but there you go ;)

;-). Considering our discussion above that hugetlb mremap addresses should
always starts at a PMD boundary, maybe I can just add a warning to the if() like
so to detect any potential?

if (is_vm_hugetlb_page(vma)) {
WARN_ON_ONCE(old_addr - old_end != len);
return move_hugetlb_page_tables(vma, new_vma, old_addr,
new_addr, len);
}


Thank you so much and I learnt a lot from you and others in -mm community.

- Joel

\
 
 \ /
  Last update: 2023-06-20 23:17    [W:0.094 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site