Re: [PATCH v10 11/41] KVM: guest_memfd: Ensure pages are not in use before conversion

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

> helps the reader understand what's being checked without having to look at the
> details, and also helps communicate the ordering dependency without needing a
> comment.
> 
> There are definitely times where the usage of a function bleeds into its name,
> but usually that's because the name and the usage are on and the same.  E.g.
> get_user() describes both the usage and the "what".  And it's easy/possible to
> go too far in the opposite direction, e.g. by giving a play-by-play of what a
> function is doing, but that's why we have bikshedding sessions :-)

Note that the problem I have with kvm_gmem_is_safe_for_conversion() that it is
all about *conversion to private*, not *conversion to shared*. In that sense,
the function name is just confusing.

> 
>> Perhaps a little ahead of its time,
> 
> Ya.
> 
>> but later with restructuring for huge pages, we also need no additional
>> refcounts other than gmem's own so that restructuring is safe, hence this
>> function name was meant to extend there as well.
> 
> Given that I've read that at least five times and still don't understand the
> nuance, I think it's safe (ha!) to say we'll need to revisit and review those
> changes no matter what. :-)
> 
>  
>> In this case "unexpected" (especially since the next patch adds checks
>> for maybe dma pinned and unmapping), begs the question "unexpected in
>> what way"?
> 
> Ya, that's why I like "outstanding", it succinctly captures that one or more
> references have been "loaned" but not yet "repaid".
> 
>>>
>>> I'd rather add a comment than have this filemap_get_folios_refcount.
> 
> +1, the local variable just made me scratch my head.
> 
>>>
>>> /*
>>>  * We expect one reference per folio-page in the pagecache and one
>>>  * reference from filemap_get_folios().
> 
> Nit, please no pronouns in KVM code.

Whatever floats KVM's boat :)

> 
>>>  */
>>> if (folio_ref_count(folio) != folio_nr_pages(folio) + 1)
>>>
>>
>> This comment explains what's "unexpected". I can do this and switch it
>> to kvm_gmem_mem_has_unexpected_refs() unless people have other
>> suggestions.
>>
>> I wish there was a folio_pagecache_refs(folio) that
>> folio_expected_ref_count() can share with this, and also
>> folio_swapcache_refs(), to solidify the definition of refcounts taken by
>> the pagecache.
> 
> ...
> 
>>>
>>> I'd add a comment here for the "why are we unmapping".
>>>
>>
>> Does this sound right:
>>
>> Unmap here to ensure that userspace page tables have no mappings, which
>> also ensures refcounts from those mappings are dropped.
> 
> How about:
> 
> 		/*
> 		 * Forcefully unmap the pages from all userspace page tables,
> 		 * and then verify there are no outstanding references, e.g.
> 		 * acquired via GUP or similar.  Tell userspace to try again if
> 		 * there are oustanding references and hope that whatever has
> 		 * pinned the page will put its reference "soon".
> 		 */
> 		unmap_mapping_pages(mapping, start, nr_pages, false);
> 
>                 if (!kvm_gmem_is_safe_for_conversion(inode, start, nr_pages,
>                                                      err_index)) {
>                         mas_destroy(&mas);
>                         r = -EAGAIN;
>                         goto out;
>                 }


Sounds good besides the function still not being clear about *which* kind of
conversion.


-- 
Cheers,

David