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

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.

quoted

Perhaps a little ahead of its time,

Ya.

quoted

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

 

quoted

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

quoted

quoted

I'd rather add a comment than have this filemap_get_folios_refcount.

+1, the local variable just made me scratch my head.

quoted

quoted

/*
 * 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 :)

quoted

quoted

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

...

quoted

quoted

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