diff mbox series

[v12,13/84] KVM: Annotate that all paths in hva_to_pfn() might sleep

Message ID 20240726235234.228822-14-seanjc@google.com (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series KVM: Stop grabbing references to PFNMAP'd pages | expand

Commit Message

Sean Christopherson July 26, 2024, 11:51 p.m. UTC
Now that hva_to_pfn() no longer supports being called in atomic context,
move the might_sleep() annotation from hva_to_pfn_slow() to hva_to_pfn().

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Alex Bennée Aug. 8, 2024, noon UTC | #1
Sean Christopherson <seanjc@google.com> writes:

> Now that hva_to_pfn() no longer supports being called in atomic context,
> move the might_sleep() annotation from hva_to_pfn_slow() to
> hva_to_pfn().

The commentary for hva_to_pfn_fast disagrees.

  /*
   * The fast path to get the writable 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.
   */
  static bool hva_to_pfn_fast(struct kvm_follow_pfn *kfp, kvm_pfn_t *pfn)

At which point did it loose the ability to run in the atomic context? I
couldn't work it out from the commits.

>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  virt/kvm/kvm_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 84c73b4fc804..03af1a0090b1 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2807,8 +2807,6 @@ static int hva_to_pfn_slow(unsigned long addr, bool *async, bool write_fault,
>  	struct page *page;
>  	int npages;
>  
> -	might_sleep();
> -
>  	if (writable)
>  		*writable = write_fault;
>  
> @@ -2947,6 +2945,8 @@ kvm_pfn_t hva_to_pfn(unsigned long addr, bool interruptible, bool *async,
>  	kvm_pfn_t pfn;
>  	int npages, r;
>  
> +	might_sleep();
> +
>  	if (hva_to_pfn_fast(addr, write_fault, writable, &pfn))
>  		return pfn;
Sean Christopherson Aug. 8, 2024, 1:16 p.m. UTC | #2
On Thu, Aug 08, 2024, Alex Bennée wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > Now that hva_to_pfn() no longer supports being called in atomic context,
> > move the might_sleep() annotation from hva_to_pfn_slow() to
> > hva_to_pfn().
> 
> The commentary for hva_to_pfn_fast disagrees.
> 
>   /*
>    * The fast path to get the writable 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.
>    */
>   static bool hva_to_pfn_fast(struct kvm_follow_pfn *kfp, kvm_pfn_t *pfn)
> 
> At which point did it loose the ability to run in the atomic context? I
> couldn't work it out from the commits.

It didn't lose the ability per se (calling hva_to_pfn_fast() in atomic context
would still be functionally ok), rather the previous patch

  KVM: Drop @atomic param from gfn=>pfn and hva=>pfn APIs

removed support for doing so in order to simplify hva_to_pfn() as a whole.
Alex Bennée Aug. 8, 2024, 3:18 p.m. UTC | #3
Sean Christopherson <seanjc@google.com> writes:

> On Thu, Aug 08, 2024, Alex Bennée wrote:
>> Sean Christopherson <seanjc@google.com> writes:
>> 
>> > Now that hva_to_pfn() no longer supports being called in atomic context,
>> > move the might_sleep() annotation from hva_to_pfn_slow() to
>> > hva_to_pfn().
>> 
>> The commentary for hva_to_pfn_fast disagrees.
>> 
>>   /*
>>    * The fast path to get the writable 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.
>>    */
>>   static bool hva_to_pfn_fast(struct kvm_follow_pfn *kfp, kvm_pfn_t *pfn)
>> 
>> At which point did it loose the ability to run in the atomic context? I
>> couldn't work it out from the commits.
>
> It didn't lose the ability per se (calling hva_to_pfn_fast() in atomic context
> would still be functionally ok), rather the previous patch
>
>   KVM: Drop @atomic param from gfn=>pfn and hva=>pfn APIs
>
> removed support for doing so in order to simplify hva_to_pfn() as a whole.

It still sticks out given the only caller no longer enforces this. 

How about:

    * true indicates success, otherwise false is returned.  It's also the
    * only part that could run in an atomic context if we wanted to
    * (although no callers expect it to).

?
Sean Christopherson Aug. 8, 2024, 3:31 p.m. UTC | #4
On Thu, Aug 08, 2024, Alex Bennée wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Thu, Aug 08, 2024, Alex Bennée wrote:
> >> Sean Christopherson <seanjc@google.com> writes:
> >> 
> >> > Now that hva_to_pfn() no longer supports being called in atomic context,
> >> > move the might_sleep() annotation from hva_to_pfn_slow() to
> >> > hva_to_pfn().
> >> 
> >> The commentary for hva_to_pfn_fast disagrees.
> >> 
> >>   /*
> >>    * The fast path to get the writable 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.
> >>    */
> >>   static bool hva_to_pfn_fast(struct kvm_follow_pfn *kfp, kvm_pfn_t *pfn)
> >> 
> >> At which point did it loose the ability to run in the atomic context? I
> >> couldn't work it out from the commits.
> >
> > It didn't lose the ability per se (calling hva_to_pfn_fast() in atomic context
> > would still be functionally ok), rather the previous patch
> >
> >   KVM: Drop @atomic param from gfn=>pfn and hva=>pfn APIs
> >
> > removed support for doing so in order to simplify hva_to_pfn() as a whole.
> 
> It still sticks out given the only caller no longer enforces this. 

Oh, sorry, I should have been more explicit.  I'll fix the comment, I simply
missed it.
Alex Bennée Aug. 8, 2024, 4:16 p.m. UTC | #5
Sean Christopherson <seanjc@google.com> writes:

> On Thu, Aug 08, 2024, Alex Bennée wrote:
>> Sean Christopherson <seanjc@google.com> writes:
>> 
>> > On Thu, Aug 08, 2024, Alex Bennée wrote:
>> >> Sean Christopherson <seanjc@google.com> writes:
>> >> 
>> >> > Now that hva_to_pfn() no longer supports being called in atomic context,
>> >> > move the might_sleep() annotation from hva_to_pfn_slow() to
>> >> > hva_to_pfn().
>> >> 
>> >> The commentary for hva_to_pfn_fast disagrees.
>> >> 
>> >>   /*
>> >>    * The fast path to get the writable 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.
>> >>    */
>> >>   static bool hva_to_pfn_fast(struct kvm_follow_pfn *kfp, kvm_pfn_t *pfn)
>> >> 
>> >> At which point did it loose the ability to run in the atomic context? I
>> >> couldn't work it out from the commits.
>> >
>> > It didn't lose the ability per se (calling hva_to_pfn_fast() in atomic context
>> > would still be functionally ok), rather the previous patch
>> >
>> >   KVM: Drop @atomic param from gfn=>pfn and hva=>pfn APIs
>> >
>> > removed support for doing so in order to simplify hva_to_pfn() as a whole.
>> 
>> It still sticks out given the only caller no longer enforces this. 
>
> Oh, sorry, I should have been more explicit.  I'll fix the comment, I simply
> missed it.

No worries, with the fixed comment:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
diff mbox series

Patch

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 84c73b4fc804..03af1a0090b1 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2807,8 +2807,6 @@  static int hva_to_pfn_slow(unsigned long addr, bool *async, bool write_fault,
 	struct page *page;
 	int npages;
 
-	might_sleep();
-
 	if (writable)
 		*writable = write_fault;
 
@@ -2947,6 +2945,8 @@  kvm_pfn_t hva_to_pfn(unsigned long addr, bool interruptible, bool *async,
 	kvm_pfn_t pfn;
 	int npages, r;
 
+	might_sleep();
+
 	if (hva_to_pfn_fast(addr, write_fault, writable, &pfn))
 		return pfn;