Re: [PATCH] userfaultfd: don't fail on unrecognized features

[Axel Rasmussen <axelrasmussen@google.com>]

On Tue, Mar 28, 2023 at 3:34 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Mar 28, 2023 at 02:52:35PM -0700, Axel Rasmussen wrote:
> > I don't see being very strict here as useful. Another example might be
> > madvise() - for example trying to MADV_PAGEOUT on a kernel that
> > doesn't support it. There is no way the kernel can proceed here, since
> > it simply doesn't know how to do what you're asking for. In this case
> > an error makes sense.
>
> IMHO, PAGEOUT is not a great example.  I wished we can have a way to probe
> what madvise() the system supports, and I know many people wanted that too.
> I even had a feeling that we'll have it some day.
>
> So now I'm going back to look at this patch assuming I'm reviewing it, I'm
> still not convinced the old API needs changing.
>
> Userfaultfd allows probing with features=0 with/without this patch, so I
> see this patch as something that doesn't bring a direct functional benefit,

The benefit is we combine probing for features and creating a
userfaultfd into a single step, so userspace doesn't have to open +
manipulate a userfaultfd twice. In my mind, both approaches achieve
the same thing, it's just that one requires extra steps to get there.

To me, it's still unclear why there is any harm in supporting the
simpler way? And, I also don't see any way in which the more complex
way is better?

> but some kind of api change due to subjective preferences which I cannot
> say right or wrong.  Now the patch is already merged.  If we need to change
> either this patch or the man page to make them match again, again I'd
> prefer we simply revert it to keep everything like before and copy stable.

I think we need to change documentation either way. But, I think the
changes needed are actually bigger if we want to revert.

With the simpler behavior, the selftest and the example program in the
man page are ~correct as-is; otherwise we would need to modify those
to use the two-step probing method.

(By the way, I am excited about the selftest refactoring you talked
about! Thanks for doing that work. It definitely needs it, the
complexity there has gotten significantly worse as we've added more
things onto it [wp, minor faults].)

I think the man page description of how to use the API is incomplete
in either case. Right now it sort of alludes to the fact that you can
probe with features==0, but it doesn't explicitly say "you need to
probe first, then close that userfaultfd and open the real one you
want to use, with a subset of the features reported in the first
step". If we want to keep the old behavior, it should be more explicit
about the steps needed to get a userfaultfd.

You are right that it also doesn't describe "you can just ask for what
you want, and the kernel tells you what subset it can give you; you
need to check that the reported features are acceptable" - the new
behavior. That should be updated.

>
> Thanks,
>
> --
> Peter Xu
>