Re: [PATCH v6 01/23] mm: Introduce PTE_MARKER swap entry

From: Peter Xu <peterx@redhat.com>
To: Alistair Popple <apopple@nvidia.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Axel Rasmussen <axelrasmussen@google.com>,
	Nadav Amit <nadav.amit@gmail.com>,
	Mike Rapoport <rppt@linux.vnet.ibm.com>,
	Hugh Dickins <hughd@google.com>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	"Kirill A . Shutemov" <kirill@shutemov.name>,
	Jerome Glisse <jglisse@redhat.com>,
	Matthew Wilcox <willy@infradead.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	David Hildenbrand <david@redhat.com>,
	Andrea Arcangeli <aarcange@redhat.com>
Subject: Re: [PATCH v6 01/23] mm: Introduce PTE_MARKER swap entry
Date: Fri, 3 Dec 2021 12:21:12 +0800	[thread overview]
Message-ID: <YambOGGK/K7saiHM@xz-m1.local> (raw)
In-Reply-To: <11462319.U46FXHIEPT@nvdebian>

On Fri, Dec 03, 2021 at 02:30:00PM +1100, Alistair Popple wrote:
> On Monday, 15 November 2021 6:55:00 PM AEDT Peter Xu wrote:
> 
> [...]
> 
> > diff --git a/include/linux/swapops.h b/include/linux/swapops.h
> > index d356ab4047f7..5103d2a4ae38 100644
> > --- a/include/linux/swapops.h
> > +++ b/include/linux/swapops.h
> > @@ -247,6 +247,84 @@ static inline int is_writable_migration_entry(swp_entry_t entry)
> >  
> >  #endif
> >  
> > +typedef unsigned long pte_marker;
> > +
> > +#define  PTE_MARKER_MASK     (0)
> > +
> > +#ifdef CONFIG_PTE_MARKER
> > +
> > +static inline swp_entry_t make_pte_marker_entry(pte_marker marker)
> > +{
> > +	return swp_entry(SWP_PTE_MARKER, marker);
> > +}
> > +
> > +static inline bool is_pte_marker_entry(swp_entry_t entry)
> > +{
> > +	return swp_type(entry) == SWP_PTE_MARKER;
> > +}
> > +
> > +static inline pte_marker pte_marker_get(swp_entry_t entry)
> > +{
> > +	return swp_offset(entry) & PTE_MARKER_MASK;
> 
> I'm not sure the PTE_MARKER_MASK adds much, especially as we only have one
> user. I don't see a problem with open-coding these kind of checks (ie.

It's more or less a safety belt to make sure anything pte_marker_get() returned
will be pte_marker defined bits only.

> swp_offset(entry) & PTE_MARKER_UFFD_WP) as you kind of end up doing that anyway.
> Alternatively if you want helper functions I think it would be better to define
> them for each marker. Eg: is_pte_marker_uffd_wp().

Yes we can have something like is_pte_marker_uffd_wp(), I didn't do that
explicitly because I want us to be clear that pte_marker is a bitmask, so
calling "is_*" will be slightly opaque - strictly speaking it should be
"pte_marker_has_uffd_wp_bit()" if there will be more bits defined, but then the
name of the helper will look a bit odd too.  Hence I just keep the only
interface to fetch the whole marker and use "&" in the call sites to check.

> 
> > +}
> > +
> > +static inline bool is_pte_marker(pte_t pte)
> > +{
> > +	return is_swap_pte(pte) && is_pte_marker_entry(pte_to_swp_entry(pte));
> > +}
> > +
> > +#else /* CONFIG_PTE_MARKER */
> > +
> > +static inline swp_entry_t make_pte_marker_entry(pte_marker marker)
> > +{
> > +	/* This should never be called if !CONFIG_PTE_MARKER */
> 
> Can we leave this function undefined then? That way we will get an obvious
> build error.

We can, but then we need more macros to cover the common code.  E.g. currently
in hugetlb_change_protection() we have:

        /* None pte */
        if (unlikely(uffd_wp))
                /* Safe to modify directly (none->non-present). */
                set_huge_pte_at(mm, address, ptep,
                                make_pte_marker(PTE_MARKER_UFFD_WP));

If we drop this definition, to let it compile with !PTE_MARKER, we'll need:

+#ifdef PTE_MARKER
        /* None pte */
        if (unlikely(uffd_wp))
                /* Safe to modify directly (none->non-present). */
                set_huge_pte_at(mm, address, ptep,
                                make_pte_marker(PTE_MARKER_UFFD_WP));
+#endif

Comparing to adding macro checks over a few other places, I figured maybe it's
easier to define them in the header once then we proper WARN_ON_ONCE() if
triggered (while they should just never).

> 
> Overall I'm liking the swap entry approach a lot more than the special pte
> approach, but maybe that's just because I'm more familiar with special swap
> entries :-)

Swap entry solution is definitely cleaner to me if not considering wasting it
with one bit.

Operating on pte directly is actually slightly more challenging, because we
don't have the protection of is_swap_pte() anymore.  It can help shield out
quite some strange stuff due to the pte->swp level hierachy.

Thanks,

-- 
Peter Xu