Messages in this thread | | | Date | Wed, 24 Aug 2022 23:06:25 +0800 | Subject | Re: [PATCH v2 1/5] mm/hugetlb: fix races when looking up a CONT-PTE size hugetlb page | From | Baolin Wang <> |
| |
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.
| |