| Message ID | 1725373521-451395-5-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | memfd-pin huge page fixes | expand |
Hi Steve, > Subject: [PATCH V1 4/5] mm/gup: fix memfd_pin_folios hugetlb page > allocation > > When memfd_pin_folios -> memfd_alloc_folio creates a hugetlb page, the > index is wrong. The subsequent call to filemap_get_folios_contig thus > cannot find it, and fails, and memfd_pin_folios loops forever. > To fix, adjust the index for the huge_page_order. > > memfd_alloc_folio also forgets to unlock the folio, so the next touch > of the page calls hugetlb_fault which blocks forever trying to take > the lock. Unlock it. Where exactly is the lock taken from? I did a quick search but couldn't immediately figure out in which function is the lock taken, while allocating the folio. > > Fixes: 89c1905d9c14 ("mm/gup: introduce memfd_pin_folios() for pinning > memfd folios") > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > mm/memfd.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/mm/memfd.c b/mm/memfd.c > index bfe0e71..bcb131d 100644 > --- a/mm/memfd.c > +++ b/mm/memfd.c > @@ -79,10 +79,13 @@ struct folio *memfd_alloc_folio(struct file *memfd, > pgoff_t idx) > * alloc from. Also, the folio will be pinned for an indefinite > * amount of time, so it is not expected to be migrated away. > */ > - gfp_mask = htlb_alloc_mask(hstate_file(memfd)); > + struct hstate *h = hstate_file(memfd); > + > + gfp_mask = htlb_alloc_mask(h); > gfp_mask &= ~(__GFP_HIGHMEM | __GFP_MOVABLE); > + idx >>= huge_page_order(h); > > - folio = alloc_hugetlb_folio_reserve(hstate_file(memfd), > + folio = alloc_hugetlb_folio_reserve(h, > numa_node_id(), > NULL, > gfp_mask); > @@ -95,6 +98,7 @@ struct folio *memfd_alloc_folio(struct file *memfd, > pgoff_t idx) > free_huge_folio(folio); > return ERR_PTR(err); > } > + folio_unlock(folio); Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com> Thanks, Vivek > return folio; > } > return ERR_PTR(-ENOMEM); > -- > 1.8.3.1
On 9/3/2024 9:06 PM, Kasireddy, Vivek wrote: > Hi Steve, > >> Subject: [PATCH V1 4/5] mm/gup: fix memfd_pin_folios hugetlb page >> allocation >> >> When memfd_pin_folios -> memfd_alloc_folio creates a hugetlb page, the >> index is wrong. The subsequent call to filemap_get_folios_contig thus >> cannot find it, and fails, and memfd_pin_folios loops forever. >> To fix, adjust the index for the huge_page_order. >> >> memfd_alloc_folio also forgets to unlock the folio, so the next touch >> of the page calls hugetlb_fault which blocks forever trying to take >> the lock. Unlock it. > Where exactly is the lock taken from? I did a quick search but couldn't > immediately figure out in which function is the lock taken, while allocating > the folio. memfd_alloc_folio -> hugetlb_add_to_page_cache -> __folio_set_locked I forgot to add that detail to the commit message. See for example hugetlbfs_fallocate which calls hugetlb_add_to_page_cache and then calls folio_unlock before returning. - Steve >> Fixes: 89c1905d9c14 ("mm/gup: introduce memfd_pin_folios() for pinning >> memfd folios") >> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> mm/memfd.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/mm/memfd.c b/mm/memfd.c >> index bfe0e71..bcb131d 100644 >> --- a/mm/memfd.c >> +++ b/mm/memfd.c >> @@ -79,10 +79,13 @@ struct folio *memfd_alloc_folio(struct file *memfd, >> pgoff_t idx) >> * alloc from. Also, the folio will be pinned for an indefinite >> * amount of time, so it is not expected to be migrated away. >> */ >> - gfp_mask = htlb_alloc_mask(hstate_file(memfd)); >> + struct hstate *h = hstate_file(memfd); >> + >> + gfp_mask = htlb_alloc_mask(h); >> gfp_mask &= ~(__GFP_HIGHMEM | __GFP_MOVABLE); >> + idx >>= huge_page_order(h); >> >> - folio = alloc_hugetlb_folio_reserve(hstate_file(memfd), >> + folio = alloc_hugetlb_folio_reserve(h, >> numa_node_id(), >> NULL, >> gfp_mask); >> @@ -95,6 +98,7 @@ struct folio *memfd_alloc_folio(struct file *memfd, >> pgoff_t idx) >> free_huge_folio(folio); >> return ERR_PTR(err); >> } >> + folio_unlock(folio); > Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com> > > Thanks, > Vivek > >> return folio; >> } >> return ERR_PTR(-ENOMEM); >> -- >> 1.8.3.1 >
diff --git a/mm/memfd.c b/mm/memfd.c index bfe0e71..bcb131d 100644 --- a/mm/memfd.c +++ b/mm/memfd.c @@ -79,10 +79,13 @@ struct folio *memfd_alloc_folio(struct file *memfd, pgoff_t idx) * alloc from. Also, the folio will be pinned for an indefinite * amount of time, so it is not expected to be migrated away. */ - gfp_mask = htlb_alloc_mask(hstate_file(memfd)); + struct hstate *h = hstate_file(memfd); + + gfp_mask = htlb_alloc_mask(h); gfp_mask &= ~(__GFP_HIGHMEM | __GFP_MOVABLE); + idx >>= huge_page_order(h); - folio = alloc_hugetlb_folio_reserve(hstate_file(memfd), + folio = alloc_hugetlb_folio_reserve(h, numa_node_id(), NULL, gfp_mask); @@ -95,6 +98,7 @@ struct folio *memfd_alloc_folio(struct file *memfd, pgoff_t idx) free_huge_folio(folio); return ERR_PTR(err); } + folio_unlock(folio); return folio; } return ERR_PTR(-ENOMEM);
When memfd_pin_folios -> memfd_alloc_folio creates a hugetlb page, the index is wrong. The subsequent call to filemap_get_folios_contig thus cannot find it, and fails, and memfd_pin_folios loops forever. To fix, adjust the index for the huge_page_order. memfd_alloc_folio also forgets to unlock the folio, so the next touch of the page calls hugetlb_fault which blocks forever trying to take the lock. Unlock it. Fixes: 89c1905d9c14 ("mm/gup: introduce memfd_pin_folios() for pinning memfd folios") Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- mm/memfd.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)