> 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
