Re: [PATCH 4/9] mm/rmap: change hugepage_add_new_anon_rmap to take in a folio

[Sidhartha Kumar]

On 1/19/23 10:00 PM, Matthew Wilcox wrote:
> On Thu, Jan 19, 2023 at 01:14:41PM -0800, Sidhartha Kumar wrote:
> > @@ -5599,9 +5603,9 @@ static vm_fault_t hugetlb_wp(struct mm_struct *mm, struct vm_area_struct *vma,
> >   		goto out_release_all;
> >   	}
> >   
> > -	copy_user_huge_page(new_page, old_page, address, vma,
> > +	copy_user_huge_page(&new_folio->page, old_page, address, vma,
> >   			    pages_per_huge_page(h));
>
> We have a folio_copy(), but it feels to me like we need a
> folio_copy_user() so that we can use copy_user_page() on machines
> with virtual caches.
>
> > @@ -6176,6 +6186,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> >   	spinlock_t *ptl;
> >   	int ret = -ENOMEM;
> >   	struct page *page;
> > +	struct folio *folio = NULL;
> >   	int writable;
> >   	bool page_in_pagecache = false;
> >   
> > @@ -6251,12 +6262,15 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
> >   		*pagep = NULL;
> >   	}
> >   
> > +	if (page)
> > +		folio = page_folio(page);
> > +
> >   	/*
> > -	 * The memory barrier inside __SetPageUptodate makes sure that
> > +	 * The memory barrier inside __folio_mark_uptodate makes sure that
> >   	 * preceding stores to the page contents become visible before
> >   	 * the set_pte_at() write.
> >   	 */
> > -	__SetPageUptodate(page);
> > +	__folio_mark_uptodate(folio);

Hi Matthew,

In the snippet:

page = alloc_huge_page(dst_vma, dst_addr, 0);
if (IS_ERR(page)) {
	put_page(*pagep);
	ret = -ENOMEM;
	*pagep = NULL;
	goto out;
}
copy_user_huge_page(page, *pagep, dst_addr, dst_vma,
		pages_per_huge_page(h));

I thought the IS_ERR() call does not handle the NULL case and is a check 
for high memory addresses, and copy_user_huge_page() path does not seem 
to handle the NULL case as well but alloc_huge_page() can possibly 
return NULL so I was unsure about how to handle the folio conversion.

> I suggest that "page" can never be NULL or __SetPageUptodate() would
> have crashed.