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

["David Hildenbrand (Red Hat)" <david@kernel.org>]

[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 ;)

> uffd_flags_t is used for two purposes
> today: 1. deciding which operation to call and 2. pass through if the
> request includes wp.  In fact, some of the callers just create and send
> the flag only within the mm/userfaultfd.c code because wp doesn't make
> sense.  Once dispatched to the operation code, the flag is only ever
> used to check for wp.
> 
> If the code was structured to use the call path to know what underlying
> operation, then the flag can be reduced to a boolean - which is what I
> ended up doing.
> 

[...]

>>
>> 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.

> 
> In upstream today, the return -ENOENT can only happen for copy (in fact
> others mask it out..), but every operation checks for -ENOENT since they
> are all using the same code block.
> 
> All of this code is more complicated because we have to find the vma
> before we know what variation of the operation we need.  Annoyingly,
> this is decided per-mfill_size even though the vma doesn't change,
> currently in  mfill_atomic_pte().
> 
> I think we could find a way to do what you are suggesting, and I think
> it's easier and less risky if the logical operations are not being
> dispatched based on uffd_flag_t.

Yeah :)

> 
>>
>>>           .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?

> 
>>
>>>           .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.

> Really, what is happening here is that the code was written to try and
> use common code.  Then hugetlb came in and duplicated everything
> anyways, so we lost what we were gaining by creating a
> dispatcher/middleware in the end.  Then handling errors complicated it
> further.
> 
> The code has also bee __alway_inline'ed, so the assembly duplicates the
> code anyways.  It's really an attempt to avoid updating multiple
> functions when issues arise.  But now we have error checks that don't
> need to happen and they are running in a loop... also hugetlb has its
> own loop.  And returning -ENOENT has a special meaning so we have to be
> careful of that too.
> 
> I don't think the compliers are smart enough to know that the retry
> loop can be removed for 3/4 cases, so the assembly is probably
> sub-optimal.
> 
>>
>> (as a side note, I wonder if we would want to call most of these uffd helper
>> uffd_*)
> 
> Yeah, sure.  Does it matter for static inlines?  Naming is hard and I
> think shmem_mfill_atomic_pte_continue() is a dumb name as well.. it's
> too short really :)

I think it just makes it clearer that this is some common uffd 
functionality we are using. Tells you immediately when reading the code 
what's common and what's hand-crafted.

Agreed that the names are ... suboptimal.

> 
>>
>>
>> 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.
> 
> Thanks for looking.  I think there is some use to having this example
> and that's why I was asking what it might look like.  I certainly went
> overboard in the last few commits to remove hugetlb all together, but at
> that point it was so close..

Right.

> 
> 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 removal of the uffd_flags_t is also something that might be worth
> exploring.

Agreed.

-- 
Cheers

David