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

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

* David Hildenbrand (Red Hat) <david@kernel.org> [251107 05:16]:
> [wondering how my mail client decides to use random mail aliases at this
> point. The kernel.org change seems to confuse something :) ]
> 
> > > 
> > > > 
> > > > 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.
> > 
> > No.  This is upstream today.
> 
> Ah, you mean *uffd_flags_t*. I was confused there for a second when grepping
> the codebase.
> 
> Yeah, not sad to see that go ;)

Ah, my bad.  I even continued to make that mistake later in my reply
here.


...

> > > 
> > > 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.
> > 
> > The failure case is only detected after getting a folio, but will need
> > to 'retry' (copy is the only one that does a retry).  Retry gets the
> > destination vma, where the vm_ops comes from.  This is why you need to
> > return to the loop.  So it's not that simple to moving it into the
> > function.
> 
> 
> In mfill_copy_loop() we have
> 
> 		err = info->op(info);
> 		cond_resched();
> 		if (unlikely(err == -ENOENT)) {
> 			err = info->uffd_ops->failed_do_unlock(info);
> 			if (unlikely(err))
> 				return err; /* Unlocked already */
> 
> 			return -ENOENT;
> 		} else {
> 			VM_WARN_ON_ONCE(info->foliop);
> 		}
> 
> 		if (!err) {
> 			uffd_info_inc(info);
> 			if (fatal_signal_pending(current))
> 				err = -EINTR;
> 		}
> 
> 
> Just to be clear, I was thinking about moving the failed_do_unlock()
> handling on -ENOENT into the info->op(). And the inc as well. (different)
> Return values could indicate what we have or don't have to do.

I'm not sure on the structure that people want to have in the end.  I
moved the unlock here because the normal unlocking remains at the same
level and was easier to see in bisect-able chunks.  The annoying name
was to indicate ti was returned unlocked, and the comment above as well.

I honestly wonder if the complication of this bug fix is worth leaving a
folio sitting around.  It's to do with low-memory and avoiding
re-acquiring a folio, AFAICT.  It's a lot of complication if it's just
for a corner case, but maybe it happens a lot or I'm missing something -
I don't know.

...

> > > >           .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).
> > 
> > The hugetlb code had a different value, so I did extract it when I
> > Iunited mfill_atomic() and mfill_atomic_hugetlb().  I am sure there are
> > other changes that could be removed as well, but to logically follow the
> > changes through each step it seemed easier to extract everything that
> > was different into its own function pointer.
> 
> 
> Let me elaborate to see if I am missing something.
> 
> page_shift() is only invoked from move_present_ptes().
> 
> move_present_ptes() works on individual PAGE_SIZE PTEs.
> 
> hugetlb does not support UFFDIO_MOVE, see how validate_move_areas() rejects
> VM_HUGETLB.
> 
> Also, move_present_ptes() wouldn't ever do anything on large folios, see
> move_present_ptes() where we have a
> 
> if (folio_test_large(src_folio) ||
>     ...
> 	err = -EBUSY;
> 	goto out;
> }
> 
> So I think the page_shift() callback can simply be dropped?

Yes, looks like it.

> 
> > 
> > > 
> > > >           .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.
> > 
> > There are certainly a lot of this we would not want to export.  My
> > initial thought was to create two function pointers: one for operations
> > that can be replaced, and one for basic functions that always have a
> > default.  We could do this with two function pointers, either tiered or
> > at the same level.
> > 
> > Most of this is to do with hugetlb having its own code branch into its
> > own loop.  We could even create an op that is returned that only lives
> > in mm/userfaultfd.c and has two variants: hugetlb and not_hugetlb.  This
> > would indeed need the hugetlb.h again, but I'm pretty sure that removing
> > the header is 'too big of a change' anyways.
> 
> Yes, I think leaving hugetlb be the only special thing around would be a
> sensible thing to do. But I would expect shmem+anon etc. to be completely
> modularizable (is that a word?).
> 
> Having a high-level API draft of that could be very valuable.
> 
> > 
> > 
> > > 
> > > > 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.
> > 
> > Yes, this is where I was going here.  I was going to try and finish this
> > off by creating that one function.  That, and reducing the vm_ops to a
> > more sensible size (as mentioned above).
> > 
> > shmem_mfill_atomic_pte_continue() could be cut up into function segments
> > to avoid the duplication.  Or we could make a wrapper that accepts a
> > function pointer.. there are certainly ways we can mitigate duplication.
> > 
> 
> Seeing a prototype of that would be nice.
> 

I'm not up for investing more time into uffd.

...

> > 
> > I think there might be value uniting both hugetlb and the normal code
> > path, even if the operations call signatures are aligned and we just use
> > a pointer to a struct within the "while (src_addr < src_start + len)"
> > loop that exists today.
> > 
> 
> Right, what would be valuable is still leaving hugetlb be special, but
> minimizing the degree to which mm/userfaultfd.c would have to care / treat
> it specially.

The question is how special you want it to remain, I guess.

Thanks,
Liam