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@xxxxxxxxxx>
>>>> Cc: Peter Xu <peterx@xxxxxxxxxx>
>>>> Signed-off-by: Li Xinhai <lixinhai.lxh@xxxxxxxxx>
>>>
>>> Thank you. That does indeed fix an issue in the current code.
>>>
>>> Reviewed-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
>>
>> 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@xxxxxxxxx>
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 */
