Re: [PATCH v3 1/4] mm: Introduce vm_uffd_ops API

["Liam R. Howlett" <Liam.Howlett@oracle.com>]

* Peter Xu <peterx@redhat.com> [251006 17:02]:
> On Mon, Oct 06, 2025 at 03:06:39PM -0400, Liam R. Howlett wrote:
> > * Peter Xu <peterx@redhat.com> [251003 10:02]:
> > > On Wed, Oct 01, 2025 at 04:39:50PM +0200, David Hildenbrand wrote:
> > > > On 01.10.25 16:35, Peter Xu wrote:
> > > > > On Wed, Oct 01, 2025 at 03:58:14PM +0200, David Hildenbrand wrote:
> > > > > > > > > > I briefly wondered whether we could use actual UFFD_FEATURE_* here, but they
> > > > > > > > > > are rather unsuited for this case here (e.g., different feature flags for
> > > > > > > > > > hugetlb support/shmem support etc).
> > 
> > I think this supports the need for a code clean up before applying an
> > API that generalizes it?
> > 
> > I would expect the uffd code that needs the same uffd_feature would
> > logically have the same uffd flags for the uffd_ops, but that's not the
> > case here?
> > 
> > Is this because uffd_feature != UFFD_FEATURE_* ... or are the internal
> > UFFD_FEATURE_* not the same thing?
> > 
> > > > > > > > > > 
> > > > > > > > > > But reading "uffd_ioctls" below, can't we derive the suitable vma flags from
> > > > > > > > > > the supported ioctls?
> > > > > > > > > > 
> > > > > > > > > > _UFFDIO_COPY | _UFDIO_ZEROPAGE -> VM_UFFD_MISSING
> > > > > > > > > > _UFFDIO_WRITEPROTECT -> VM_UFFD_WP
> > > > > > > > > > _UFFDIO_CONTINUE -> VM_UFFD_MINOR
> > > > > > > > > 
> > > > > > > > > Yes we can deduce that, but it'll be unclear then when one stares at a
> > > > > > > > > bunch of ioctls and cannot easily digest the modes the memory type
> > > > > > > > > supports.  Here, the modes should be the most straightforward way to
> > > > > > > > > describe the capability of a memory type.
> > > > > > > > 
> > > > > > > > I rather dislike the current split approach between vm-flags and ioctls.
> > > > > > > > 
> > > > > > > > I briefly thought about abstracting it for internal purposes further and
> > > > > > > > just have some internal backend ("memory type") flags.
> > > > > > > > 
> > > > > > > > UFFD_BACKEND_FEAT_MISSING -> _UFFDIO_COPY and VM_UFFD_MISSING
> > > > > > > > UFFD_BACKEND_FEAT_ZEROPAGE -> _UFDIO_ZEROPAGE
> > > > > > > > UFFD_BACKEND_FEAT_WP -> _UFFDIO_WRITEPROTECT and VM_UFFD_WP
> > > > > > > > UFFD_BACKEND_FEAT_MINOR -> _UFFDIO_CONTINUE and VM_UFFD_MINOR
> > > > > > > > UFFD_BACKEND_FEAT_POISON -> _UFFDIO_POISON
> > > > > > > 
> > > > > > > This layer of mapping can be helpful to some, but maybe confusing to
> > > > > > > others.. who is familiar with existing userfaultfd definitions.
> > > > > > > 
> > > > > > 
> > > > > > Just wondering, is this confusing to you, and if so, which part?
> > > > > > 
> > > > > > To me it makes perfect sense and cleans up this API and not have to sets of
> > > > > > flags that are somehow interlinked.
> > > > > 
> > > > > It adds the extra layer of mapping that will only be used in vm_uffd_ops
> > > > > and the helper that will consume it.
> > > > 
> > > > Agreed, while making the API cleaner. I don't easily see what's confusing
> > > > about that, though.
> > > 
> > > It will introduce another set of userfaultfd features, making it hard to
> > > say what is the difference between the new set and UFFD_FEATURE_*.
> > 
> > If it's not using UFFD_FEATURE_ defines, then please don't use
> > uffd_feature for it in the uffd_ops.  That seems like a recipe for
> > confusion.
> > 
> > > 
> > > > 
> > > > I think it can be done with a handful of LOC and avoid having to use VM_
> > > > flags in this API.
> > > 
> > > I waited for a few days, unfortunately we didn't get a second opinion.
> > 
> > Sorry, been pretty busy here.
> > 
> > If we can avoid the flags/features, then I'd rather that (the derived
> > from uffd_ops == NULL for support).  We can always add something else
> > later.
> > 
> > If we have to have a feature/flag setting, then please avoid using
> > uffd_feature unless we use it with UFFD_FEATURE_ - which I think, we've
> > ruled out?
> 
> Yes, there was no plan to use UFFD_FEATURE_* in vm_uffd_ops.  It's because
> UFFD_FEATURE_* was introduced to almost let userapp have a way to probe the
> capability of the kernel, rather than the layer to describe what features
> userfaultfd has.
> 
> For example, we have UFFD_FEATURE_MISSING_SHMEM declaring that "the current
> kernel supports MISSING mode userfaultfd on shmem".  This feature flag is
> essentially of no use for any other memory types, hence not applicable to
> vm_uffd_ops.  OTOH, we don't have a feature flag to represent "userfaultfd
> MISSING mode".
> 

hehe, the overloaded terms here are numerous, but I think I get what you
are saying.  It's funny that FEATURE_MISSING isn't a check for a missing
feature, but really to check if the register mode missing is available.

I'd also rather not have ioctls and features/flags.  It seems reasonable
to drop the ioctl, like David said.

I assume there is some future plan for flags, or is this for versioning?

I'd like to one day even remove the suggested backend types and instead
use handlers in the uffd_ops directly, although it is difficult to know
if this is reasonable today.

Thanks,
Liam