Re: [PATCH 4/8] kvm: Allow hva_pfn_fast to resolve read-only faults.

From: Sean Christopherson <seanjc@google.com>
To: Oliver Upton <oliver.upton@linux.dev>
Cc: Anish Moorthy <amoorthy@google.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Marc Zyngier <maz@kernel.org>,
	James Houghton <jthoughton@google.com>,
	Ben Gardon <bgardon@google.com>,
	David Matlack <dmatlack@google.com>,
	Ricardo Koller <ricarkol@google.com>,
	Chao Peng <chao.p.peng@linux.intel.com>,
	Axel Rasmussen <axelrasmussen@google.com>,
	kvm@vger.kernel.org, kvmarm@lists.linux.dev
Subject: Re: [PATCH 4/8] kvm: Allow hva_pfn_fast to resolve read-only faults.
Date: Wed, 15 Feb 2023 09:03:02 -0800	[thread overview]
Message-ID: <Y+0QRsZ4yWyUdpnc@google.com> (raw)
In-Reply-To: <Y+yfhELf/TbsosO9@linux.dev>

On Wed, Feb 15, 2023, Oliver Upton wrote:
> On Wed, Feb 15, 2023 at 01:16:10AM +0000, Anish Moorthy wrote:
> > The upcoming mem_fault_nowait commits will make it so that, when the
> > relevant cap is enabled, hva_to_pfn will return after calling
> > hva_to_pfn_fast without ever attempting to pin memory via
> > hva_to_pfn_slow.
> > 
> > hva_to_pfn_fast currently just fails for read-only faults. However,
> > there doesn't seem to be a reason that we can't just try pinning the
> > page without FOLL_WRITE instead of immediately falling back to slow-GUP.
> > This commit implements that behavior.

State what the patch does, avoid pronouns, and especially don't have "This commmit"
or "This patch" anywhere.  From Documentation/process/submitting-patches.rst:

  Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
  instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
  to do frotz", as if you are giving orders to the codebase to change
  its behaviour.

> > Suggested-by: James Houghton <jthoughton@google.com>
> > Signed-off-by: Anish Moorthy <amoorthy@google.com>
> > ---
> >  virt/kvm/kvm_main.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index d255964ec331e..dae5f48151032 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -2479,7 +2479,7 @@ static inline int check_user_page_hwpoison(unsigned long addr)
> >  }
> >  
> >  /*
> > - * The fast path to get the writable pfn which will be stored in @pfn,
> > + * The fast path to get the pfn which will be stored in @pfn,
> >   * true indicates success, otherwise false is returned.  It's also the
> >   * only part that runs if we can in atomic context.
> >   */
> > @@ -2487,16 +2487,18 @@ static bool hva_to_pfn_fast(unsigned long addr, bool write_fault,
> >  			    bool *writable, kvm_pfn_t *pfn)
> >  {
> >  	struct page *page[1];
> > +	bool found_by_fast_gup =
> > +		get_user_page_fast_only(
> > +			addr,
> > +			/*
> > +			 * Fast pin a writable pfn only if it is a write fault request
> > +			 * or the caller allows to map a writable pfn for a read fault
> > +			 * request.
> > +			 */
> > +			(write_fault || writable) ? FOLL_WRITE : 0,
> > +			page);
> >  
> > -	/*
> > -	 * Fast pin a writable pfn only if it is a write fault request
> > -	 * or the caller allows to map a writable pfn for a read fault
> > -	 * request.
> > -	 */
> > -	if (!(write_fault || writable))
> > -		return false;
> > -
> > -	if (get_user_page_fast_only(addr, FOLL_WRITE, page)) {
> > +	if (found_by_fast_gup) {
> 
> You could have a smaller diff (and arrive at something more readable)

Heh, this whole series just screams "google3". :-)

Anish, please read through 

  Documentation/process/coding-style.rst

and 

  Documentation/process/submitting-patches.rst

particularaly the "Describe your changes" and "Style-check your changes" your
changes sections.  Bonus points if you work through the mostly redundant process/
documentation, e.g. these have supplementary info.

  Documentation/process/4.Coding.rst
  Documentation/process/5.Posting.rst