| Message ID | 20240726235234.228822-14-seanjc@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | KVM: Stop grabbing references to PFNMAP'd pages | expand |
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;
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.
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). ?
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.
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 --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;
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(-)