| Message ID | 20230602161921.208564-2-amoorthy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve scalability of KVM + userfaultfd live migration via annotated memory faults. | expand |
Don't put trailing punctation in shortlogs, i.e. drop the period. On Fri, Jun 02, 2023, Anish Moorthy wrote: > hva_to_pfn_fast() currently just fails for read-only faults, which is > unnecessary. Instead, try pinning the page without passing FOLL_WRITE. s/pinning/getting (or maybe grabbing?), because "pinning" is already way too overloaded in the context of gup(), e.g. FOLL_PIN vs. FOLL_GET. > This allows read-only faults to (potentially) be resolved without "read-only faults" is somewhat confusing, because every architecture passes a non-NULL @writable for read faults. If it weren't for KVM_ARM_MTE_COPY_TAGS, this could be "faults to read-only memslots". Not sure how to concisely and accurately describe this. :-/
On Wed, Jun 14, 2023 at 7:39 AM Sean Christopherson <seanjc@google.com> wrote: > > Don't put trailing punctation in shortlogs, i.e. drop the period. > > s/pinning/getting (or maybe grabbing?), because "pinning" is already way too > overloaded in the context of gup(), e.g. FOLL_PIN vs. FOLL_GET. Done > "read-only faults" is somewhat confusing, because every architecture passes a > non-NULL @writable for read faults. If it weren't for KVM_ARM_MTE_COPY_TAGS, > this could be "faults to read-only memslots". Not sure how to concisely and > accurately describe this. :-/ "Read faults when establishing writable mappings is forbidden" maybe? That should be accurate, although it's certainly not concise.
I figured I'd start double checking my documentation changes before sending out the next version, since those have been a persistent issue. So, here's what I've currently got for the commit message here > hva_to_pfn_fast() currently just fails for read faults where > establishing writable mappings is forbidden, which is unnecessary. > Instead, try getting the page without passing FOLL_WRITE. This allows > the aforementioned faults to (potentially) be resolved without falling > back to slow GUP.
On Thu, Aug 10, 2023, Anish Moorthy wrote: > I figured I'd start double checking my documentation changes before > sending out the next version, since those have been a persistent > issue. So, here's what I've currently got for the commit message here > > > hva_to_pfn_fast() currently just fails for read faults where > > establishing writable mappings is forbidden, which is unnecessary. > > Instead, try getting the page without passing FOLL_WRITE. This allows > > the aforementioned faults to (potentially) be resolved without falling > > back to slow GUP. Looks good! One nit, I would drop the "read" part of "read faults". This behavior also applies to executable faults. You captured the key part well (writable mappings forbidden), so I don't think there's any need to further clarify what types of faults this applies to.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index cb5c13eee193..fd80a560378c 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2482,7 +2482,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. */ @@ -2496,10 +2496,9 @@ static bool hva_to_pfn_fast(unsigned long addr, bool write_fault, * or the caller allows to map a writable pfn for a read fault * request. */ - if (!(write_fault || writable)) - return false; + unsigned int gup_flags = (write_fault || writable) ? FOLL_WRITE : 0; - if (get_user_page_fast_only(addr, FOLL_WRITE, page)) { + if (get_user_page_fast_only(addr, gup_flags, page)) { *pfn = page_to_pfn(page[0]); if (writable)
hva_to_pfn_fast() currently just fails for read-only faults, which is unnecessary. Instead, try pinning the page without passing FOLL_WRITE. This allows read-only faults to (potentially) be resolved without falling back to slow GUP. Suggested-by: James Houghton <jthoughton@google.com> Signed-off-by: Anish Moorthy <amoorthy@google.com> --- virt/kvm/kvm_main.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)