Re: [PATCH v3 06/27] shmem/userfaultfd: Handle uffd-wp special pte in page fault handler

From: Peter Xu <peterx@redhat.com>
To: Alistair Popple <apopple@nvidia.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Andrea Arcangeli <aarcange@redhat.com>,
	"Kirill A . Shutemov" <kirill@shutemov.name>,
	Axel Rasmussen <axelrasmussen@google.com>,
	Nadav Amit <nadav.amit@gmail.com>,
	Hugh Dickins <hughd@google.com>,
	Jerome Glisse <jglisse@redhat.com>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	Andrew Morton <akpm@linux-foundation.org>,
	Miaohe Lin <linmiaohe@huawei.com>,
	Mike Rapoport <rppt@linux.vnet.ibm.com>,
	Matthew Wilcox <willy@infradead.org>,
	Mike Kravetz <mike.kravetz@oracle.com>
Subject: Re: [PATCH v3 06/27] shmem/userfaultfd: Handle uffd-wp special pte in page fault handler
Date: Thu, 17 Jun 2021 11:10:42 -0400	[thread overview]
Message-ID: <YMtl8pqUL/zLOZiy@t490s> (raw)
In-Reply-To: <5906045.o7RC9TDvkT@nvdebian>

On Thu, Jun 17, 2021 at 06:59:09PM +1000, Alistair Popple wrote:
> > +static vm_fault_t do_swap_pte(struct vm_fault *vmf)
> > +{
> > +	/*
> > +	 * We need to handle special swap ptes before handling ptes that
> > +	 * contain swap entries, always.
> > +	 */
> > +	if (unlikely(pte_swp_uffd_wp_special(vmf->orig_pte)))
> > +		return uffd_wp_handle_special(vmf);
> > +
> > +	return do_swap_page(vmf);
> 
> Probably pretty minor in the scheme of things but why not add this special
> case directly to do_swap_page()? Your earlier "shmem/userfaultfd: Handle
> uffd-wp special pte in page fault handler" adds this to do_swap_page()
> anyway:
> 
> 	/*
> 	 * We should never call do_swap_page upon a swap special pte; just be
> 	 * safe to bail out if it happens.
> 	 */
> 	if (WARN_ON_ONCE(is_swap_special_pte(vmf->orig_pte)))
> 		goto out;
> 
> So this patch could instead replace the warning with the call to
> uffd_wp_handle_special(), which also means you can remove the extra
> pte_unmap_same(vmf) check in uffd_wp_handle_special().
> 
> I suppose you might have to worry about other callers of do_swap_page(),
> but the only other one I could see was __collapse_huge_page_swapin().
> Initially I thought that might be able to trigger the warning here but I
> see it checks pte_has_swap_entry() first which should skip it if it finds
> the special pte.

Yes I wanted to keep the existing caller untouched, and I wanted to keep its
semantics too to not bother with the new idea (it turns out do_swap_page should
have a history long enough to be beyond when git is introduced to Linux).

The other reason is that this series is the first one to introduce the new swap
pte which actually does not have a page on the back, so I figured maybe it's
good to call the new handler do_swap_pte() (as swap pte can either contain swap
entry or not), then we keep do_swap_page() if it's an old typed swap pte (which
contains the swap entry).

Thanks,

-- 
Peter Xu