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

[Alistair Popple <apopple@nvidia.com>]

On Friday, 3 December 2021 3:21:12 PM AEDT Peter Xu wrote:
> 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.

Why does a caller need to care if it's a bitmask or not though? Isn't that an
implementation detail that could be left to the "is_*" functions? I must admit
I'm still working through the rest of this series though - is it because you
end up storing some kind of value in the upper bits of the PTE marker?

> > 
> > > +}
> > > +
> > > +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).

Ok, makes sense. Agree that adding macro checks everywhere isn't great.

> > 
> > 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.

So I guess now we have the protection of is_swap_pte() there are probably a few
places where we need to check for marker pte entries when we find swap entries.
I'm not suggesting you haven't already found all of those cases of course, just
noting that it's something to review.

> Thanks,
> 
>