by Axel Rasmussen

[permalink] [raw]

Subject: Re: [PATCH v4 4/4] mm: userfaultfd: add UFFDIO_CONTINUE_MODE_WP to install WP PTEs

On Thu, Mar 9, 2023 at 1:11 AM Mike Rapoport <[email protected]> wrote:
>
> On Wed, Mar 08, 2023 at 02:19:32PM -0800, Axel Rasmussen wrote:
> > UFFDIO_COPY already has UFFDIO_COPY_MODE_WP, so when installing a new
> > PTE to resolve a missing fault, one can install a write-protected one.
> > This is useful when using UFFDIO_REGISTER_MODE_{MISSING,WP} in
> > combination.
> >
> > This was motivated by testing HugeTLB HGM [1], and in particular its
> > interaction with userfaultfd features. Existing userfaultfd code
> > supports using WP and MINOR modes together (i.e. you can register an
> > area with both enabled), but without this CONTINUE flag the combination
> > is in practice unusable.
> >
> > So, add an analogous UFFDIO_CONTINUE_MODE_WP, which does the same thing
> > as UFFDIO_COPY_MODE_WP, but for *minor* faults.
> >
> > Update the selftest to do some very basic exercising of the new flag.
> >
> > [1]: https://patchwork.kernel.org/project/linux-mm/cover/[email protected]/
> >
> > Acked-by: Peter Xu <[email protected]>
> > Signed-off-by: Axel Rasmussen <[email protected]>
>
> Acked-by: Mike Rapoport (IBM) <[email protected]>
>
> > ---
> > fs/userfaultfd.c | 8 ++++++--
> > include/linux/userfaultfd_k.h | 3 ++-
> > include/uapi/linux/userfaultfd.h | 7 +++++++
> > mm/userfaultfd.c | 5 +++--
> > tools/testing/selftests/mm/userfaultfd.c | 4 ++++
> > 5 files changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/uapi/linux/userfaultfd.h b/include/uapi/linux/userfaultfd.h
> > index 005e5e306266..14059a0861bf 100644
> > --- a/include/uapi/linux/userfaultfd.h
> > +++ b/include/uapi/linux/userfaultfd.h
> > @@ -297,6 +297,13 @@ struct uffdio_writeprotect {
> > struct uffdio_continue {
> > struct uffdio_range range;
> > #define UFFDIO_CONTINUE_MODE_DONTWAKE ((__u64)1<<0)
> > + /*
> > + * UFFDIO_CONTINUE_MODE_WP will map the page write protected on
> > + * the fly. UFFDIO_CONTINUE_MODE_WP is available only if the
> > + * write protected ioctl is implemented for the range
> > + * according to the uffdio_register.ioctls.
> > + */
> > +#define UFFDIO_CONTINUE_MODE_WP ((__u64)1<<1)
>
> Please add the description of the new flag to Documentation/ and to the
> userfaultfd man pages.

Funny enough, neither flag is mentioned in Documentation/ today - I'll
add a short passage about both.

Happy to update the man pages as well, I'll send that patch separately.

Thanks for reviewing!

>
> > __u64 mode;
> >
> > /*
>
> --
> Sincerely yours,
> Mike.

2023-03-09 22:46:34

by Axel Rasmussen

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] mm: userfaultfd: combine 'mode' and 'wp_copy' arguments

On Wed, Mar 8, 2023 at 2:43 PM Peter Xu <[email protected]> wrote:
>
> All nitpicks below.
>
> On Wed, Mar 08, 2023 at 02:19:31PM -0800, Axel Rasmussen wrote:
> > +static inline bool uffd_flags_has_mode(uffd_flags_t flags, enum mfill_atomic_mode expected)
> > +{
> > + return (flags & MFILL_ATOMIC_MODE_MASK) == ((__force uffd_flags_t) expected);
> > +}
>
> I would still call it uffd_flags_get_mode() or uffd_flags_mode(), "has"
> sounds a bit like there can be >1 modes set but it's not.

I want a helper which does the comparison, instead of just returning
the mode, because it avoids all callers needing to do the __force cast
themselves to appease sparse.

How about uffd_flags_mode_is() ?

>
> > +
> > +static inline uffd_flags_t uffd_flags_set_mode(uffd_flags_t flags, enum mfill_atomic_mode mode)
> > +{
> > + return flags | ((__force uffd_flags_t) mode);
> > +}
>
> IIUC this __force mostly won't work in any way because it protects
> e.g. illegal math ops upon it (to only allow bitops, iiuc) but here it's an
> OR so it's always legal..
>
> So I'd just drop it and also clear the mode mask to be very clear it sets
> the mode right, rather than any chance of messing up when set twice:
>
> flags &= ~MFILL_ATOMIC_MODE_MASK;
> return flags | mode;

Without this __force, "make C=1" gives errors like this:

./include/linux/userfaultfd_k.h:66:16: warning: restricted
uffd_flags_t degrades to integer
./include/linux/userfaultfd_k.h:66:22: warning: incorrect type in
return expression (different base types)
./include/linux/userfaultfd_k.h:66:22: expected restricted uffd_flags_t
./include/linux/userfaultfd_k.h:66:22: got unsigned int

This is because the mode being passed in is effectively an integer, so
the | expression loses the restricted type. Casting the mode first
like this appeases sparse.

An alternative would be to do the cast in the definition of the mode
values up-front; but as we noticed before, we can't really usefully do
that with it still being an enum (so we'd have to hard-code things
like the mode mask, etc.)

I do completely agree about clearing the mask bits first, to avoid
mistakes. I'll send an updated version with that change. If we're
going to have an inline helper anyway to do that, for me it makes less
sense to switch away from the num approach (basically the benefit of
that would be to avoid needing this cast, and therefore the helper;
but if we want the helper anyway for other reasons ...).

>
> But feel free to ignore this if there's no other reason to repost, I don't
> think it matters a huge deal.
>
> Acked-by: Peter Xu <[email protected]>
>
> Thanks,
>
> --
> Peter Xu
>