Re: [PATCH 4/5] mm: Do early cow for pinned pages during fork() for ptes

[Peter Xu]

On Thu, Sep 24, 2020 at 02:48:00PM +0300, Kirill Tkhai wrote:
> > +/*
> > + * Duplicate the page for this PTE.  Returns zero if page copied (so we need to
> > + * retry on the same PTE again to arm the copied page very soon), or negative
> > + * if error happened.  In all cases, the old page will be properly released.
> > + */
> > +static int page_duplicate(struct mm_struct *src_mm, struct vm_area_struct *vma,
> > +			  unsigned long address, struct copy_mm_data *data)
> > +{
> > +	struct page *new_page = NULL;
> > +	int ret;
> > +
> > +	/* This should have been set in change_one_pte() when reach here */
> > +	WARN_ON_ONCE(!data->cow_old_page);
> 
> Despite WARN() is preferred over BUG() in kernel, it looks a little strange that
> we catch WARN once here, but later avoid panic in put_page().

Do you mean "it'll panic in put_page()"?  I'll agree if so, seems this
WARN_ON_ONCE() won't help much.

> 
> > +	new_page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, address);
> > +	if (!new_page) {
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	copy_user_highpage(new_page, data->cow_old_page, address, vma);
> > +	ret = mem_cgroup_charge(new_page, src_mm, GFP_KERNEL);
> 
> All failing operations should go first, while copy_user_highpage() should go last.

Since I'll rebase to Linus's patch, I'll move this into the critical section
because the preallocated page can be used by any pte after that.  The spin
locks will need to be taken longer for that, but assuming that's not a problem
for an unlikely path.

> > @@ -859,6 +989,25 @@ static int copy_pte_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
> >  			    spin_needbreak(src_ptl) || spin_needbreak(dst_ptl))
> >  				break;
> >  		}
> > +
> > +		if (unlikely(data.cow_new_page)) {
> > +			/*
> > +			 * If cow_new_page set, we must be at the 2nd round of
> > +			 * a previous COPY_MM_BREAK_COW.  Try to arm the new
> > +			 * page now.  Note that in all cases page_break_cow()
> > +			 * will properly release the objects in copy_mm_data.
> > +			 */
> > +			WARN_ON_ONCE(copy_ret != COPY_MM_BREAK_COW);
> > +			if (pte_install_copied_page(dst_mm, new, src_pte,
> > +						    dst_pte, addr, rss,
> > +						    &data)) {
> 
> It looks a little confusing, that all helpers in this function return 0 in case of success,
> while pte_install_copied_page() returns true. Won't be better to return 0 and -EAGAIN instead
> from it?

IMHO it's fine as long as no real errno will be popped out of the new helper.
But no strong opinion either, I'll see what I can do after the rebase.

Thanks for reviewing the patch even if it's going away.

-- 
Peter Xu