"David Hildenbrand (Arm)" <[email protected]> writes:
>> 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.
>
Circling back to this since Binbin also had a comment about the comment!
I'll drop the use of the const local variable and do
if (folio_ref_count(folio) != folio_nr_pages(folio) + 1)
, and add a comment rephrased without the pronouns:
No references outstanding if guest_memfd sees 1 reference per
folio-page in the pagecache and 1 reference from
filemap_get_folios()
, and then also take the comment and rename the function from
kvm_gmem_is_safe_for_conversion to kvm_gmem_has_outstanding_references.
if (to_private) {
/*
* 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_has_outstanding_references(inode, start, nr_pages,
err_index)) {
mas_destroy(&mas);
r = -EAGAIN;
goto out;
}
}
I think renaming the function from kvm_gmem_is_safe_for_conversion to
kvm_gmem_has_outstanding_references should be better since it doesn't
really make a statement on what kind of conversion the outstanding
references are for.
>
> --
> Cheers,
>
> David
