Re: [PATCH] mm/hugetlb.c: fix unnecessary address expansion of pmd sharing

[Mike Kravetz <mike.kravetz@oracle.com>]

On 1/2/21 3:56 AM, Li Xinhai wrote:
> 
> 
> On 1/1/21 1:56 AM, Mike Kravetz wrote:
>> On 12/29/20 1:20 PM, Mike Kravetz wrote:
>>> On 12/28/20 8:21 PM, Li Xinhai wrote:
>>>> The current code would unnecessarily expand the address range. Consider
>>>> one example, (start, end) = (1G-2M, 3G+2M), and  (vm_start, vm_end) =
>>>> (1G-4M, 3G+4M), the expected adjustment should be keep (1G-2M, 3G+2M)
>>>> without expand. But the current result will be (1G-4M, 3G+4M). Actually,
>>>> the range (1G-4M, 1G) and (3G, 3G+4M) would never been involved in pmd
>>>> sharing.
>>>>
>>>> After this patch, if pud aligned *start across vm_start, then we know the
>>>> *start and vm_start are in same pud_index, and vm_start is not pud
>>>> aligned, so don't adjust *start. Same logic applied to *end.
>>>>
>>>> Fixes: commit 75802ca66354 ("mm/hugetlb: fix calculation of adjust_range_if_pmd_sharing_possible")
>>>> Cc: Mike Kravetz <mike.kravetz@oracle.com>
>>>> Cc: Peter Xu <peterx@redhat.com>
>>>> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
>>>
>>> Thank you.  That does indeed fix an issue in the current code.
>>>
>>> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
>>
>> Upon further thought, this patch also expands the passed range when not
>> necessary.  Consider the example (start, end) = (1G-6M, 1G-4M), and
>> (vm_start, vm_end) = (1G, 1G-2M).  This patch would adjust the range to
>> (1G, 1G-4M).  However, no adjustment should be performed as no sharing
>> is possible.
>>
> correct, my previous patch did not fully fix the issue.
> 
> Above example maybe typo for vm_start, vm_end. The issue didn't fixed by my patch would be with another example,
> (vm_start, vm_end) = (1G-8M, 1G+2M), (start, end) = (1G-6M, 1G-4M), end should not be adjusted to 1G, although after adjust it still below vm_end.
> 

Sorry, I did incorrectly write that example.  It should have read:

Consider the example (start, end) = (2G-6M, 2G-4M), and
(vm_start, vm_end) = (2G, 2G-2M).  This patch would adjust the range to
(2G, 2G-4M).  However, no adjustment should be performed as no sharing
is possible.

>> Below is proposed code to address the issue.  I'm not sending a formal
>> patch yet as I would like comments on the code first.  It is not a critical
>> issue and any fix can wait a bit.

> Now, this fully fixed the issue.
> 
> One thing to be sure is that the (start, end) as input parameter must already within vma's range, although the range_in_vma test() can cover the out of range cases.
> 
> Reviewed-by: Li Xinhai <lixinhai.lxh@gmail.com>

Thanks for taking a look.

I believe the only case where your patch produced incorrect results is
when the range was within a vma that was smaller than PUD_SIZE.  Do you
agree?

If that is the case, then how about just adding the following to your patch?
I think this is simpler and faster than the 'range_in_vma' checking I proposed.

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 49990c0a02a3..716d1e58a7ae 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -5261,7 +5261,9 @@ void adjust_range_if_pmd_sharing_possible(struct vm_area_struct *vma,
 {
 	unsigned long a_start, a_end;
 
-	if (!(vma->vm_flags & VM_MAYSHARE))
+	/* Quick check for vma capable of pmd sharing */
+	if (!(vma->vm_flags & VM_MAYSHARE) ||
+	    (vma->vm_start - vma->vm_end) < PUD_SIZE)
 		return;
 
 	/* Extend the range to be PUD aligned for a worst case scenario */