Re: [PATCH 1/4] mm: Trial do_wp_page() simplification

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Sep 18, 2020 at 12:40:32PM -0400, Peter Xu wrote:

> Firstly in the draft patch mm->has_pinned is introduced and it's written to 1
> as long as FOLL_GUP is called once.  It's never reset after set.

Worth thinking about also adding FOLL_LONGTERM here, at last as long
as it is not a counter. That further limits the impact.

> One more thing (I think) we need to do is to pass the new vma from
> copy_page_range() down into the end because if we want to start cow during
> fork() then we need to operate on that new vma too when new page linked to it
> rather than the parent's.

Makes sense to me

> One issue is when we charge for cgroup we probably can't do that onto the new
> mm/task, since copy_namespaces() is called after copy_mm().  I don't know
> enough about cgroup, I thought the child will inherit the parent's, but I'm not
> sure.  Or, can we change that order of copy_namespaces() && copy_mm()?  I don't
> see a problem so far but I'd like to ask first..

Know nothing about cgroups, but I would have guessed that the page
table allocations would want to be in the cgroup too, is the struct
page a different bucket?

> The other thing is on how to fail.  E.g., when COW failed due to either
> charging of cgroup or ENOMEM, ideally we should fail fork() too.  Though that
> might need more changes - current patch silently kept the shared page for
> simplicity.

I didn't notice anything tricky here.. Something a bit gross but
simple seemed workable:

@@ -852,7 +852,7 @@ static int copy_pte_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 			continue;
 		}
 		entry.val = copy_one_pte(dst_mm, src_mm, dst_pte, src_pte,
-							vma, addr, rss);
+							vma, addr, rss, &err);
 		if (entry.val)
 			break;
 		progress += 8;
@@ -865,6 +865,9 @@ static int copy_pte_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 	pte_unmap_unlock(orig_dst_pte, dst_ptl);
 	cond_resched();
 
+	if (err)
+		return err;
+
 	if (entry.val) {
 		if (add_swap_count_continuation(entry, GFP_KERNEL) < 0)
 			return -ENOMEM;

It is not really any different from add_swap_count_continuation()
failure, which already works..

> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 496c3ff97cce..b3812fa6383f 100644
> +++ b/include/linux/mm_types.h
> @@ -458,6 +458,7 @@ struct mm_struct {
>  
>  		unsigned long total_vm;	   /* Total pages mapped */
>  		unsigned long locked_vm;   /* Pages that have PG_mlocked set */
> +		unsigned long has_pinned;  /* Whether this mm has pinned any page */

Can be unsigned int or smaller, if there is a better hole in mm_struct

> diff --git a/mm/gup.c b/mm/gup.c
> index e5739a1974d5..cab10cefefe4 100644
> +++ b/mm/gup.c
> @@ -1255,6 +1255,17 @@ static __always_inline long __get_user_pages_locked(struct mm_struct *mm,
>  		BUG_ON(*locked != 1);
>  	}
>  
> +	/*
> +	 * Mark the mm struct if there's any page pinning attempt.  We're
> +	 * aggresive on this bit since even if the pinned pages were unpinned
> +	 * later on, we'll still keep this bit set for this address space just
> +	 * to make everything easy.
> +	 *
> +	 * TODO: Ideally we can use mm->pinned_vm but only until it's stable.
> +	 */
> +	if (flags & FOLL_PIN)
> +		WRITE_ONCE(mm->has_pinned, 1);

This should probably be its own commit, here is a stab at a commit
message:

Reduce the chance of false positive from page_maybe_dma_pinned() by
keeping track if the mm_struct has ever been used with
pin_user_pages(). mm_structs that have never been passed to
pin_user_pages() cannot have a positive page_maybe_dma_pinned() by
definition. This allows cases that might drive up the page ref_count
to avoid any penalty from handling dma_pinned pages.

Due to complexities with unpining this trivial version is a permanent
sticky bit, future work will be needed to make this a counter.

> +/*
> + * Do early cow for the page and the pte. Return true if page duplicate
> + * succeeded, false otherwise.
> + */
> +static bool duplicate_page(struct mm_struct *mm, struct vm_area_struct *vma,

Suggest calling 'vma' 'new' here for consistency

> +			   unsigned long address, struct page *page,
> +			   pte_t *newpte)
> +{
> +       struct page *new_page;
> +       pte_t entry;
> +
> +       new_page = alloc_page_vma(GFP_HIGHUSER_MOVABLE, vma, address);
> +       if (!new_page)
> +               return false;
> +
> +       copy_user_highpage(new_page, page, address, vma);
> +       if (mem_cgroup_charge(new_page, mm, GFP_KERNEL)) {
> +	       put_page(new_page);
> +	       return false;
> +       }
> +       cgroup_throttle_swaprate(new_page, GFP_KERNEL);
> +       __SetPageUptodate(new_page);

It looks like these GFP flags can't be GFP_KERNEL, this is called
inside the pte_alloc_map_lock() which is a spinlock

One thought is to lift this logic out to around
add_swap_count_continuation()? Would need some serious rework to be
able to store the dst_pte though.

Can't help about the cgroup question

Jason