Re: [PATCH] hugetlb: simplify hugetlb handling in follow_page_mask

[Mike Kravetz]

On 08/31/22 10:07, David Hildenbrand wrote:
> On 30.08.22 23:31, Mike Kravetz wrote:
> > On 08/30/22 09:52, Mike Kravetz wrote:
> >> On 08/30/22 10:11, David Hildenbrand wrote:
> >>> On 30.08.22 01:40, Mike Kravetz wrote:
> >>>> During discussions of this series [1], it was suggested that hugetlb
> >>>> handling code in follow_page_mask could be simplified.  At the beginning
> >>>
> >>> Feel free to use a Suggested-by if you consider it appropriate.
> >>>
> >>>> of follow_page_mask, there currently is a call to follow_huge_addr which
> >>>> 'may' handle hugetlb pages.  ia64 is the only architecture which provides
> >>>> a follow_huge_addr routine that does not return error.  Instead, at each
> >>>> level of the page table a check is made for a hugetlb entry.  If a hugetlb
> >>>> entry is found, a call to a routine associated with that entry is made.
> >>>>
> >>>> Currently, there are two checks for hugetlb entries at each page table
> >>>> level.  The first check is of the form:
> >>>> 	if (p?d_huge())
> >>>> 		page = follow_huge_p?d();
> >>>> the second check is of the form:
> >>>> 	if (is_hugepd())
> >>>> 		page = follow_huge_pd().
> >>>
> >>> BTW, what about all this hugepd stuff in mm/pagewalk.c?
> >>>
> >>> Isn't this all dead code as we're essentially routing all hugetlb VMAs
> >>> via walk_hugetlb_range? [yes, all that hugepd stuff in generic code that
> >>> overcomplicates stuff has been annoying me for a long time]
> >>
> >> I am 'happy' to look at cleaning up that code next.  Perhaps I will just
> >> create a cleanup series.
> >>
> > 
> > Technically, that code is not dead IIUC.  The call to walk_hugetlb_range in
> > __walk_page_range is as follows:
> > 
> > 	if (vma && is_vm_hugetlb_page(vma)) {
> > 		if (ops->hugetlb_entry)
> > 			err = walk_hugetlb_range(start, end, walk);
> > 	} else
> > 		err = walk_pgd_range(start, end, walk);
> > 
> > We also have the interface walk_page_range_novma() that will call
> > __walk_page_range without a value for vma.  So, in that case we would
> > end up calling walk_pgd_range, etc.  walk_pgd_range and related routines
> > do have those checks such as:
> > 
> > 		if (is_hugepd(__hugepd(pmd_val(*pmd))))
> > 			err = walk_hugepd_range((hugepd_t *)pmd, addr, next, walk, PMD_SHIFT);
> > 
> > So, it looks like in this case we would process 'hugepd' entries but not
> > 'normal' hugetlb entries.  That does not seem right.
> 
> :/ walking a hugetlb range without knowing whether it's a hugetlb range
> is certainly questionable.
> 
> 
> > 
> > Christophe Leroy added this code with commit e17eae2b8399 "mm: pagewalk: fix
> > walk for hugepage tables".  This was part of the series "Convert powerpc to
> > GENERIC_PTDUMP".  And, the ptdump code uses the walk_page_range_novma
> > interface.  So, this code is certainly not dead.
> 
> Hm, that commit doesn't actually mention how it can happen, what exactly
> will happen ("crazy result") and if it ever happened.
> 
> > 
> > Adding Christophe on Cc:
> > 
> > Christophe do you know if is_hugepd is true for all hugetlb entries, not
> > just hugepd?
> > 
> > On systems without hugepd entries, I guess ptdump skips all hugetlb entries.
> > Sigh!
> 
> IIUC, the idea of ptdump_walk_pgd() is to dump page tables even outside
> VMAs (for debugging purposes?).
> 
> I cannot convince myself that that's a good idea when only holding the
> mmap lock in read mode, because we can just see page tables getting
> freed concurrently e.g., during concurrent munmap() ... while holding
> the mmap lock in read we may only walk inside VMA boundaries.
> 
> That then raises the questions if we're only calling this on special MMs
> (e.g., init_mm) whereby we cannot really see concurrent munmap() and
> where we shouldn't have hugetlb mappings or hugepd entries.
> 

This is going to require a little more thought.

Since Baolin's patch for stable releases is moving forward, I want to
get the cleanup provided by this patch in ASAP.  So, I am going to rebase
this patch on Baolin's with the other fixups.

Will come back to this cleanup later.
-- 
Mike Kravetz