On 1/26/21 4:07 PM, Jason Gunthorpe wrote: > On Tue, Jan 26, 2021 at 01:21:46PM -0800, Mike Kravetz wrote: >> On 1/26/21 11:21 AM, Joao Martins wrote: >>> On 1/26/21 6:08 PM, Mike Kravetz wrote: >>>> On 1/25/21 12:57 PM, Joao Martins wrote: >>>>> >>>>> +static void record_subpages_vmas(struct page *page, struct >>>>> vm_area_struct *vma, >>>>> + int refs, struct page **pages, >>>>> + struct vm_area_struct **vmas) >>>>> +{ >>>>> + int nr; >>>>> + >>>>> + for (nr = 0; nr < refs; nr++) { >>>>> + if (likely(pages)) >>>>> + pages[nr] = page++; >>>>> + if (vmas) >>>>> + vmas[nr] = vma; >>>>> + } >>>>> +} >>>>> + >>>>> long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct >>>>> *vma, >>>>> struct page **pages, struct vm_area_struct **vmas, >>>>> unsigned long *position, unsigned long *nr_pages, >>>>> @@ -4918,28 +4932,16 @@ long follow_hugetlb_page(struct mm_struct *mm, >>>>> struct vm_area_struct *vma, >>>>> continue; >>>>> } >>>>> >>>>> - refs = 0; >>>>> + refs = min3(pages_per_huge_page(h) - pfn_offset, >>>>> + (vma->vm_end - vaddr) >> PAGE_SHIFT, remainder); >>>>> >>>>> -same_page: >>>>> - if (pages) >>>>> - pages[i] = mem_map_offset(page, pfn_offset); >>>>> + if (pages || vmas) >>>>> + record_subpages_vmas(mem_map_offset(page, pfn_offset), >>>> >>>> The assumption made here is that mem_map is contiguous for the range of >>>> pages in the hugetlb page. I do not believe you can make this assumption >>>> for (gigantic) hugetlb pages which are > MAX_ORDER_NR_PAGES. For example, >>>> >> >> Thinking about this a bit more ... >> >> mem_map can be accessed contiguously if we have a virtual memmap. Correct? >> I suspect virtual memmap may be the most common configuration today. >> However, >> it seems we do need to handle other configurations. >> >>> That would mean get_user_pages_fast() and put_user_pages_fast() are broken >>> for anything >>> handling PUDs or above? See record_subpages() in gup_huge_pud() or even >>> gup_huge_pgd(). >>> It's using the same page++. >> >> Yes, I believe those would also have the issue. >> Cc: John and Jason as they have spent a significant amount of time in gup >> code recently. There may be something that makes that code safe? > > I'm looking at Matt's folio patches and see: > > +static inline struct folio *next_folio(struct folio *folio) > +{ > + return folio + folio_nr_pages(folio); > +} > > And checking page_trans_huge_mapcount(): > > for (i = 0; i < thp_nr_pages(page); i++) { > mapcount = atomic_read(&page[i]._mapcount) + 1; > > And we have the same logic in hmm_vma_walk_pud(): > > if (pud_huge(pud) && pud_devmap(pud)) { > pfn = pud_pfn(pud) + ((addr & ~PUD_MASK) >> PAGE_SHIFT); > for (i = 0; i < npages; ++i, ++pfn) > hmm_pfns[i] = pfn | cpu_flags; > > So, if page[n] does not access the tail pages of a compound we have > many more people who are surprised by this than just GUP. > > Where are these special rules for hugetlb compound tails documented? > Why does it need to be like this?
The commit where this was first addressed/pointed out is 69d177c2fc70 "hugetlbfs: handle pages higher order than MAX_ORDER" from back in 2008. I only know about this because I stumbled upon it a few times in the hugetlb code. Although, it does not appear to be hugetlb specific. As pointed out by Joao, you can also see the differences in pfn_to_page for CONFIG_SPARSE_VMEMMAP and CONFIG_SPARSEMEM. The only time we might have issues is with CONFIG_SPARSEMEM. I would bet CONFIG_SPARSE_VMEMMAP is far more common. Cc: Dave as he has sparsemem history. -- Mike Kravetz > Isn't it saner to forbid a compound and its tails from being > non-linear in the page array? That limits when compounds can be > created, but seems more likely to happen than a full mm audit to find > all the places that assume linearity.