Re: [PATCH v4 0/4] mm/userfaultfd: modulize memory types

[David Hildenbrand]

On 30.10.25 18:13, Liam R. Howlett wrote:
> * Peter Xu <peterx@xxxxxxxxxx> [251021 12:28]:
>
> ...
>
> > Can you send some patches and show us the code, help everyone to support
> > guest-memfd minor fault, please?
>
> Patches are here:

Hi Liam,

thanks for showing us what userfaultfd could look like when refactored 
according to your idea. I think most of the userfaultfd core code is 
easier to get in your tree.

> https://git.infradead.org/?p=users/jedix/linux-maple.git;a=shortlog;h=refs/heads/modularized_mem
>
> This is actually modularized memory types.  That means there is no
> hugetlb.h or shmem.h included in mm/userfaultfd.c code.

Yeah, I think there is this confusion of "modulize memory types" and 
"support minor fault in guest_memfd".

So I see what you did here as trying to see how far we could go to 
remove any traces of shmem/hugetlb from userfaultd core.

So I'll comment based on that and rather see it as a bigger, more 
extreme rework.

> uffd_flag_t has been removed.  This was turning into a middleware and
> it is not necessary.  Neither is supported_ioctls.

I assume you mean the entries that were proposed in Peters series, not 
something that is upstream.

> hugetlb now uses the same functions as every other memory type,
> including anon memory.
>
> Any memory type can change functionality without adding instructions or
> flags or anything to some other code.
>
> This code passes uffd-unit-test and uffd-wp-mremap (skipped the swap
> tests).
>
> guest-memfd can implement whatever it needs to (or use others
> implementations), like shmem_uffd_ops here:

There is obviously some downside to be had with this approach (some of 
which Mike raised), regarding the interface to "memory types" 
implementing this, but I'll discuss that later.

> static const struct vm_uffd_ops shmem_uffd_ops = {
>          .copy                   =       shmem_mfill_atomic_pte_copy,
>          .zeropage               =       shmem_mfill_atomic_pte_zeropage,
>          .cont                   =       shmem_mfill_atomic_pte_continue,
>          .poison                 =       mfill_atomic_pte_poison,
>          .writeprotect           =       uffd_writeprotect,
>          .is_dst_valid           =       shmem_is_dst_valid,
>          .increment              =       mfill_size,

See below, I wonder if that could be performed by the callbacks invoked 
as part of the prior calls to mfill_loop() etc.

>          .failed_do_unlock       =       uffd_failed_do_unlock,

That one is a bit unfortunate (read: ugly :) ).

failed_do_unlock() is only called from mfill_copy_loop(). Where we 
perform a prior info.uffd_ops->copy.


After calling err = info->op(info);

Couldn't that callback just deal with the -ENOENT case?

So in case of increment/failed_do_unlock, maybe we could find a way to 
just let the ->copy etc communicate/perform that directly.

>          .page_shift             =       uffd_page_shift,

Fortunately, this is not required. The only user in move_present_ptes() 
moves *real* PTEs, and nothing else (no hugetlb PTEs that are PMDs etc. 
in disguise).

>          .complete_register      =       uffd_complete_register,
> };

So, the design is to callback into the memory-type handler, which will 
then use exported uffd functionality to get the job done.

This nicely abstracts hugetlb handling, but could mean that any code 
implementing this interface has to built up on exported uffd 
functionality (not judging, just saying).

As we're using the callbacks as an indication whether features are 
supported, we cannot easily leave them unset to fallback to the default 
handling.

Of course, we could use some placeholder, magic UFFD_DEFAULT_HANDLER 
keyword to just use the uffd_* stuff without exporting them.

So NULL would mean "not supported" and "UFFD_DEFAULT_HANDLER" would mean 
"no special handling needed".

Not sure how often that would be the case, though. For shmem it would 
probably only be the poison callback, for others, I am not sure.

> Where guest-memfd needs to write the one function:
> guest_memfd_pte_continue(), from what I understand.

It would be interesting to see how that one would look like.

I'd assume fairly similar to shmem_mfill_atomic_pte_continue()?

Interesting question would be, how to avoid the code duplication there.

(as a side note, I wonder if we would want to call most of these uffd 
helper uffd_*)


I'll have to think about some of this some more. In particular, 
alternatives to at least get all the shmem logic cleanly out of there 
and maybe only have a handful callback into hugetlb.

IOW, not completely make everything fit the "odd case" and rather focus 
on the "normal cases" when designing this vm_ops interface here.

Not sure if that makes sense, just wanted to raise it.

--
Cheers

David