Message ID | 20200416155906.267462-1-peterx@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: No need to retry for hva_to_pfn_remapped() | expand |
On 16/04/20 17:59, Peter Xu wrote: > hva_to_pfn_remapped() calls fixup_user_fault(), which has already > handled the retry gracefully. Even if "unlocked" is set to true, it > means that we've got a VM_FAULT_RETRY inside fixup_user_fault(), > however the page fault has already retried and we should have the pfn > set correctly. No need to do that again. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > virt/kvm/kvm_main.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 2f1f2f56e93d..6aaed69641a5 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -1824,8 +1824,6 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma, > r = fixup_user_fault(current, current->mm, addr, > (write_fault ? FAULT_FLAG_WRITE : 0), > &unlocked); > - if (unlocked) > - return -EAGAIN; > if (r) > return r; > > @@ -1896,15 +1894,12 @@ static kvm_pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async, > goto exit; > } > > -retry: > vma = find_vma_intersection(current->mm, addr, addr + 1); > > if (vma == NULL) > pfn = KVM_PFN_ERR_FAULT; > else if (vma->vm_flags & (VM_IO | VM_PFNMAP)) { > r = hva_to_pfn_remapped(vma, addr, write_fault, writable, &pfn); > - if (r == -EAGAIN) > - goto retry; > if (r < 0) > pfn = KVM_PFN_ERR_FAULT; > } else { > Queued, thanks. Paolo
On Mon, May 04, 2020 at 06:59:30PM +0200, Paolo Bonzini wrote: > On 16/04/20 17:59, Peter Xu wrote: > > hva_to_pfn_remapped() calls fixup_user_fault(), which has already > > handled the retry gracefully. Even if "unlocked" is set to true, it > > means that we've got a VM_FAULT_RETRY inside fixup_user_fault(), > > however the page fault has already retried and we should have the pfn > > set correctly. No need to do that again. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > virt/kvm/kvm_main.c | 5 ----- > > 1 file changed, 5 deletions(-) > > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 2f1f2f56e93d..6aaed69641a5 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -1824,8 +1824,6 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma, > > r = fixup_user_fault(current, current->mm, addr, > > (write_fault ? FAULT_FLAG_WRITE : 0), > > &unlocked); > > - if (unlocked) > > - return -EAGAIN; > > if (r) > > return r; > > > > @@ -1896,15 +1894,12 @@ static kvm_pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async, > > goto exit; > > } > > > > -retry: > > vma = find_vma_intersection(current->mm, addr, addr + 1); > > > > if (vma == NULL) > > pfn = KVM_PFN_ERR_FAULT; > > else if (vma->vm_flags & (VM_IO | VM_PFNMAP)) { > > r = hva_to_pfn_remapped(vma, addr, write_fault, writable, &pfn); > > - if (r == -EAGAIN) > > - goto retry; > > if (r < 0) > > pfn = KVM_PFN_ERR_FAULT; > > } else { > > > > Queued, thanks. Paolo, Is it still possible to unqueue this patch? I overlooked the fact that if unlocked==true then the vma pointer could be invalidated, so the 2nd follow_pfn() is potentially racy. Sorry for the trouble!
On 08/05/20 04:25, Peter Xu wrote: >> Queued, thanks. > > Paolo, Is it still possible to unqueue this patch? I overlooked the fact that > if unlocked==true then the vma pointer could be invalidated, so the 2nd > follow_pfn() is potentially racy. Sorry for the trouble! > Sure, thanks. Paolo
On 08/05/20 04:25, Peter Xu wrote: > Even if "unlocked" is set to true, it > means that we've got a VM_FAULT_RETRY inside fixup_user_fault(), I have just reverted it. Thanks! Paolo
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 2f1f2f56e93d..6aaed69641a5 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1824,8 +1824,6 @@ static int hva_to_pfn_remapped(struct vm_area_struct *vma, r = fixup_user_fault(current, current->mm, addr, (write_fault ? FAULT_FLAG_WRITE : 0), &unlocked); - if (unlocked) - return -EAGAIN; if (r) return r; @@ -1896,15 +1894,12 @@ static kvm_pfn_t hva_to_pfn(unsigned long addr, bool atomic, bool *async, goto exit; } -retry: vma = find_vma_intersection(current->mm, addr, addr + 1); if (vma == NULL) pfn = KVM_PFN_ERR_FAULT; else if (vma->vm_flags & (VM_IO | VM_PFNMAP)) { r = hva_to_pfn_remapped(vma, addr, write_fault, writable, &pfn); - if (r == -EAGAIN) - goto retry; if (r < 0) pfn = KVM_PFN_ERR_FAULT; } else {
hva_to_pfn_remapped() calls fixup_user_fault(), which has already handled the retry gracefully. Even if "unlocked" is set to true, it means that we've got a VM_FAULT_RETRY inside fixup_user_fault(), however the page fault has already retried and we should have the pfn set correctly. No need to do that again. Signed-off-by: Peter Xu <peterx@redhat.com> --- virt/kvm/kvm_main.c | 5 ----- 1 file changed, 5 deletions(-)