| Message ID | 20210804154046.88552-3-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: pv: implement lazy destroy | expand |
On 04.08.21 17:40, Claudio Imbrenda wrote: > Improve make_secure_pte to avoid stalls when the system is heavily > overcommitted. This was especially problematic in kvm_s390_pv_unpack, > because of the loop over all pages that needed unpacking. > > Also fix kvm_s390_pv_init_vm to avoid stalls when the system is heavily > overcommitted. I suggest splitting this change into a separate patch and adding a bit more meat to the description why using the other variant is possible in the called context. I was kind of surprise to find that change buried in this patch. Then, you can give both patches a more descriptive patch subject. > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > Fixes: 214d9bbcd3a672 ("s390/mm: provide memory management functions for protected KVM guests") > --- > arch/s390/kernel/uv.c | 29 +++++++++++++++++++++++------ > arch/s390/kvm/pv.c | 2 +- > 2 files changed, 24 insertions(+), 7 deletions(-) > > diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c > index aeb0a15bcbb7..68a8fbafcb9c 100644 > --- a/arch/s390/kernel/uv.c > +++ b/arch/s390/kernel/uv.c > @@ -180,7 +180,7 @@ static int make_secure_pte(pte_t *ptep, unsigned long addr, > { > pte_t entry = READ_ONCE(*ptep); > struct page *page; > - int expected, rc = 0; > + int expected, cc = 0; > > if (!pte_present(entry)) > return -ENXIO; > @@ -196,12 +196,25 @@ static int make_secure_pte(pte_t *ptep, unsigned long addr, > if (!page_ref_freeze(page, expected)) > return -EBUSY; > set_bit(PG_arch_1, &page->flags); > - rc = uv_call(0, (u64)uvcb); > + /* > + * If the UVC does not succeed or fail immediately, we don't want to > + * loop for long, or we might get stall notifications. > + * On the other hand, this is a complex scenario and we are holding a lot of > + * locks, so we can't easily sleep and reschedule. We try only once, > + * and if the UVC returned busy or partial completion, we return > + * -EAGAIN and we let the callers deal with it. > + */ > + cc = __uv_call(0, (u64)uvcb); > page_ref_unfreeze(page, expected); > - /* Return -ENXIO if the page was not mapped, -EINVAL otherwise */ > - if (rc) > - rc = uvcb->rc == 0x10a ? -ENXIO : -EINVAL; > - return rc; > + /* > + * Return -ENXIO if the page was not mapped, -EINVAL for other errors. > + * If busy or partially completed, return -EAGAIN. > + */ > + if (cc == UVC_CC_OK) > + return 0; > + else if (cc == UVC_CC_BUSY || cc == UVC_CC_PARTIAL) > + return -EAGAIN; > + return uvcb->rc == 0x10a ? -ENXIO : -EINVAL; > } That looks conceptually like the right thing to me.
On Fri, 6 Aug 2021 09:30:04 +0200 David Hildenbrand <david@redhat.com> wrote: > On 04.08.21 17:40, Claudio Imbrenda wrote: > > Improve make_secure_pte to avoid stalls when the system is heavily > > overcommitted. This was especially problematic in > > kvm_s390_pv_unpack, because of the loop over all pages that needed > > unpacking. > > > > Also fix kvm_s390_pv_init_vm to avoid stalls when the system is > > heavily overcommitted. > > I suggest splitting this change into a separate patch and adding a > bit more meat to the description why using the other variant is > possible in the called context. I was kind of surprise to find that > change buried in this patch. > > Then, you can give both patches a more descriptive patch subject. fair enough, I'll split them > > > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > Fixes: 214d9bbcd3a672 ("s390/mm: provide memory management > > functions for protected KVM guests") --- > > arch/s390/kernel/uv.c | 29 +++++++++++++++++++++++------ > > arch/s390/kvm/pv.c | 2 +- > > 2 files changed, 24 insertions(+), 7 deletions(-) > > > > diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c > > index aeb0a15bcbb7..68a8fbafcb9c 100644 > > --- a/arch/s390/kernel/uv.c > > +++ b/arch/s390/kernel/uv.c > > @@ -180,7 +180,7 @@ static int make_secure_pte(pte_t *ptep, > > unsigned long addr, { > > pte_t entry = READ_ONCE(*ptep); > > struct page *page; > > - int expected, rc = 0; > > + int expected, cc = 0; > > > > if (!pte_present(entry)) > > return -ENXIO; > > @@ -196,12 +196,25 @@ static int make_secure_pte(pte_t *ptep, > > unsigned long addr, if (!page_ref_freeze(page, expected)) > > return -EBUSY; > > set_bit(PG_arch_1, &page->flags); > > - rc = uv_call(0, (u64)uvcb); > > + /* > > + * If the UVC does not succeed or fail immediately, we > > don't want to > > + * loop for long, or we might get stall notifications. > > + * On the other hand, this is a complex scenario and we > > are holding a lot of > > + * locks, so we can't easily sleep and reschedule. We try > > only once, > > + * and if the UVC returned busy or partial completion, we > > return > > + * -EAGAIN and we let the callers deal with it. > > + */ > > + cc = __uv_call(0, (u64)uvcb); > > page_ref_unfreeze(page, expected); > > - /* Return -ENXIO if the page was not mapped, -EINVAL > > otherwise */ > > - if (rc) > > - rc = uvcb->rc == 0x10a ? -ENXIO : -EINVAL; > > - return rc; > > + /* > > + * Return -ENXIO if the page was not mapped, -EINVAL for > > other errors. > > + * If busy or partially completed, return -EAGAIN. > > + */ > > + if (cc == UVC_CC_OK) > > + return 0; > > + else if (cc == UVC_CC_BUSY || cc == UVC_CC_PARTIAL) > > + return -EAGAIN; > > + return uvcb->rc == 0x10a ? -ENXIO : -EINVAL; > > } > > That looks conceptually like the right thing to me. >
diff --git a/arch/s390/kernel/uv.c b/arch/s390/kernel/uv.c index aeb0a15bcbb7..68a8fbafcb9c 100644 --- a/arch/s390/kernel/uv.c +++ b/arch/s390/kernel/uv.c @@ -180,7 +180,7 @@ static int make_secure_pte(pte_t *ptep, unsigned long addr, { pte_t entry = READ_ONCE(*ptep); struct page *page; - int expected, rc = 0; + int expected, cc = 0; if (!pte_present(entry)) return -ENXIO; @@ -196,12 +196,25 @@ static int make_secure_pte(pte_t *ptep, unsigned long addr, if (!page_ref_freeze(page, expected)) return -EBUSY; set_bit(PG_arch_1, &page->flags); - rc = uv_call(0, (u64)uvcb); + /* + * If the UVC does not succeed or fail immediately, we don't want to + * loop for long, or we might get stall notifications. + * On the other hand, this is a complex scenario and we are holding a lot of + * locks, so we can't easily sleep and reschedule. We try only once, + * and if the UVC returned busy or partial completion, we return + * -EAGAIN and we let the callers deal with it. + */ + cc = __uv_call(0, (u64)uvcb); page_ref_unfreeze(page, expected); - /* Return -ENXIO if the page was not mapped, -EINVAL otherwise */ - if (rc) - rc = uvcb->rc == 0x10a ? -ENXIO : -EINVAL; - return rc; + /* + * Return -ENXIO if the page was not mapped, -EINVAL for other errors. + * If busy or partially completed, return -EAGAIN. + */ + if (cc == UVC_CC_OK) + return 0; + else if (cc == UVC_CC_BUSY || cc == UVC_CC_PARTIAL) + return -EAGAIN; + return uvcb->rc == 0x10a ? -ENXIO : -EINVAL; } /* @@ -254,6 +267,10 @@ int gmap_make_secure(struct gmap *gmap, unsigned long gaddr, void *uvcb) mmap_read_unlock(gmap->mm); if (rc == -EAGAIN) { + /* + * If we are here because the UVC returned busy or partial + * completion, this is just a useless check, but it is safe. + */ wait_on_page_writeback(page); } else if (rc == -EBUSY) { /* diff --git a/arch/s390/kvm/pv.c b/arch/s390/kvm/pv.c index c8841f476e91..e007df11a2fe 100644 --- a/arch/s390/kvm/pv.c +++ b/arch/s390/kvm/pv.c @@ -196,7 +196,7 @@ int kvm_s390_pv_init_vm(struct kvm *kvm, u16 *rc, u16 *rrc) uvcb.conf_base_stor_origin = (u64)kvm->arch.pv.stor_base; uvcb.conf_virt_stor_origin = (u64)kvm->arch.pv.stor_var; - cc = uv_call(0, (u64)&uvcb); + cc = uv_call_sched(0, (u64)&uvcb); *rc = uvcb.header.rc; *rrc = uvcb.header.rrc; KVM_UV_EVENT(kvm, 3, "PROTVIRT CREATE VM: handle %llx len %llx rc %x rrc %x",
Improve make_secure_pte to avoid stalls when the system is heavily overcommitted. This was especially problematic in kvm_s390_pv_unpack, because of the loop over all pages that needed unpacking. Also fix kvm_s390_pv_init_vm to avoid stalls when the system is heavily overcommitted. Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> Fixes: 214d9bbcd3a672 ("s390/mm: provide memory management functions for protected KVM guests") --- arch/s390/kernel/uv.c | 29 +++++++++++++++++++++++------ arch/s390/kvm/pv.c | 2 +- 2 files changed, 24 insertions(+), 7 deletions(-)