Date:   Wed, 24 Aug 2022 23:06:25 +0800
From:   Baolin Wang <baolin.wang@...ux.alibaba.com>
To:     David Hildenbrand <david@...hat.com>,
        Mike Kravetz <mike.kravetz@...cle.com>
Cc:     akpm@...ux-foundation.org, songmuchun@...edance.com,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/5] mm/hugetlb: fix races when looking up a CONT-PTE
 size hugetlb page



On 8/24/2022 10:33 PM, David Hildenbrand wrote:
> On 24.08.22 16:30, Baolin Wang wrote:
>>
>>
>> On 8/24/2022 7:55 PM, David Hildenbrand wrote:
>>> On 24.08.22 11:41, Baolin Wang wrote:
>>>>
>>>>
>>>> On 8/24/2022 3:31 PM, David Hildenbrand wrote:
>>>>>>>>>
>>>>>>>>> IMHO, these follow_huge_xxx() functions are arch-specified at first and
>>>>>>>>> were moved into the common hugetlb.c by commit 9e5fc74c3025 ("mm:
>>>>>>>>> hugetlb: Copy general hugetlb code from x86 to mm"), and now there are
>>>>>>>>> still some arch-specified follow_huge_xxx() definition, for example:
>>>>>>>>> ia64: follow_huge_addr
>>>>>>>>> powerpc: follow_huge_pd
>>>>>>>>> s390: follow_huge_pud
>>>>>>>>>
>>>>>>>>> What I mean is that follow_hugetlb_page() is a common and
>>>>>>>>> not-arch-specified function, is it suitable to change it to be
>>>>>>>>> arch-specified?
>>>>>>>>> And thinking more, can we rename follow_hugetlb_page() as
>>>>>>>>> hugetlb_page_faultin() and simplify it to only handle the page faults of
>>>>>>>>> hugetlb like the faultin_page() for normal page? That means we can make
>>>>>>>>> sure only follow_page_mask() can handle hugetlb.
>>>>>>>>>
>>>>>>>
>>>>>>> Something like that might work, but you still have two page table walkers
>>>>>>> for hugetlb.  I like David's idea (if I understand it correctly) of
>>>>>>
>>>>>> What I mean is we may change the hugetlb handling like normal page:
>>>>>> 1) use follow_page_mask() to look up a hugetlb firstly.
>>>>>> 2) if can not get the hugetlb, then try to page fault by
>>>>>> hugetlb_page_faultin().
>>>>>> 3) if page fault successed, then retry to find hugetlb by
>>>>>> follow_page_mask().
>>>>>
>>>>> That implies putting more hugetlbfs special code into generic GUP,
>>>>> turning it even more complicated. But of course, it depends on how the
>>>>> end result looks like. My gut feeling was that hugetlb is better handled
>>>>> in follow_hugetlb_page() separately (just like we do with a lot of other
>>>>> page table walkers).
>>>>
>>>> OK, fair enough.
>>>>
>>>>>>
>>>>>> Just a rough thought, and I need more investigation for my idea and
>>>>>> David's idea.
>>>>>>
>>>>>>> using follow_hugetlb_page for both cases.  As noted, it will need to be
>>>>>>> taught how to not trigger faults in the follow_page_mask case.
>>>>>>
>>>>>> Anyway, I also agree we need some cleanup, and firstly I think we should
>>>>>> cleanup these arch-specified follow_huge_xxx() on some architectures
>>>>>> which are similar with the common ones. I will look into these.
>>>>>
>>>>> There was a recent discussion on that, e.g.:
>>>>>
>>>>> https://lkml.kernel.org/r/20220818135717.609eef8a@thinkpad
>>>>
>>>> Thanks.
>>>>
>>>>>
>>>>>>
>>>>>> However, considering cleanup may need more investigation and
>>>>>> refactoring, now I prefer to make these bug-fix patches of this patchset
>>>>>> into mainline firstly, which are suitable to backport to old version to
>>>>>> fix potential race issues. Mike and David, how do you think? Could you
>>>>>> help to review these patches? Thanks.
>>>>>
>>>>> Patch #1 certainly add more special code just to handle another hugetlb
>>>>> corner case (CONT pages), and maybe just making it all use
>>>>> follow_hugetlb_page() would be even cleaner and less error prone.
>>>>>
>>>>> I agree that locking is shaky, but I'm not sure if we really want to
>>>>> backport this to stable trees:
>>>>>
>>>>> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>>>>>
>>>>> "It must fix a real bug that bothers people (not a, “This could be a
>>>>> problem...” type thing)."
>>>>>
>>>>>
>>>>> Do we actually have any instance of this being a real (and not a
>>>>> theoretical) problem? If not, I'd rather clean it all up right away.
>>>>
>>>> I think this is a real problem (not theoretical), and easy to write some
>>>> code to show the issue. For example, suppose thread A is trying to look
>>>> up a CONT-PTE size hugetlb page under the lock, however antoher thread B
>>>> can migrate the CONT-PTE hugetlb page at the same time, which will cause
>>>> thread A to get an incorrect page, if thread A want to do something for
>>>> this incorrect page, error occurs.
>>>>
>>>> Actually we also want to backport these fixes to the distro with old
>>>> kernel versions to make the hugetlb more stable. Otherwise we must hit
>>>> these issues sooner or later if the customers use CONT-PTE/PMD hugetlb.
>>>>
>>>> Anyway, if you and Mike still think these issues are not important
>>>> enough to be fixed in the old versions, I can do the cleanup firstly.
>>>>
>>>
>>> [asking myself which follow_page() users actually care about hugetlb,
>>> and why we need this handling in follow_page at all]
>>>
>>> Which follow_page() user do we care about here? Primarily mm/migrate.c
>>> only I assume?
>>
>> Right, mainly affects the move_pages() syscall I think. Yes, I can not
>> know all of the users of the move_pages() syscall now or in the future
>> in our data center, but like I said the move_pages() syscall + hugetlb
>> can be a real potential stability issue.
>>
> 
> I wonder if we can get rid of follow_page() completely, there are not
> too many users. Or alternatively simply make it use general GUP
> infrastructure more clearly. We'd need something like FOLL_NOFAULT that
> also covers "absolutely no faults".

I am not sure I get your point. So you want change to use 
__get_user_pages() (or silimar wrappers) to look up a normal page or 
hugetlb instead of follow_page()? and adding a new FOLL_NOFAULT flag to 
__get_user_pages().

If I understand correctly, we still need more work to move those 
arch-specified follow_huge_xxx() into follow_hugetlb_page() firstly like 
we disscussed before? Which seems not backportable too.

I am not againt your idea, and I also agree that we should do some 
cleanup. But the point is if we need backport patches to fix this issue, 
which affects move_pages() syscall, if the answer is yes, I think my 
current fixes are suitable to backport.

Powered by blists - more mailing lists