| Message ID | 160371603337.305923.17158585537944509438.stgit@bahia.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spapr: Error handling fixes and cleanups (round 5) | expand |
On 10/26/20 1:40 PM, Greg Kurz wrote: > qemu_memalign() aborts if OOM. Drop some dead code. > > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/ppc/spapr.c | 6 ------ > hw/ppc/spapr_hcall.c | 8 ++------ > 2 files changed, 2 insertions(+), 12 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 0cc19b5863a4..f098d0ee6d98 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1521,12 +1521,6 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > int i; > > spapr->htab = qemu_memalign(size, size); > - if (!spapr->htab) { > - error_setg_errno(errp, errno, > - "Could not allocate HPT of order %d", shift); > - return; Wasn't the idea to use qemu_try_memalign() here? > - } > - > memset(spapr->htab, 0, size); > spapr->htab_shift = shift; > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index 607740150fa2..34e146f628fb 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -361,12 +361,8 @@ static void *hpt_prepare_thread(void *opaque) > size_t size = 1ULL << pending->shift; > > pending->hpt = qemu_memalign(size, size); > - if (pending->hpt) { > - memset(pending->hpt, 0, size); > - pending->ret = H_SUCCESS; > - } else { > - pending->ret = H_NO_MEM; Ditto. > - } > + memset(pending->hpt, 0, size); > + pending->ret = H_SUCCESS; > > qemu_mutex_lock_iothread(); >
On Mon, 26 Oct 2020 14:43:08 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 10/26/20 1:40 PM, Greg Kurz wrote: > > qemu_memalign() aborts if OOM. Drop some dead code. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/ppc/spapr.c | 6 ------ > > hw/ppc/spapr_hcall.c | 8 ++------ > > 2 files changed, 2 insertions(+), 12 deletions(-) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 0cc19b5863a4..f098d0ee6d98 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -1521,12 +1521,6 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > > int i; > > > > spapr->htab = qemu_memalign(size, size); > > - if (!spapr->htab) { > > - error_setg_errno(errp, errno, > > - "Could not allocate HPT of order %d", shift); > > - return; > > Wasn't the idea to use qemu_try_memalign() here? > Well... I have mixed feeling around this. The HTAB was first introduced by commit: commit f43e35255cffb6ac6230dd09d308f7909f823f96 Author: David Gibson <david@gibson.dropbear.id.au> Date: Fri Apr 1 15:15:22 2011 +1100 Virtual hash page table handling on pSeries machine using qemu_mallocz(), which was aborting on OOM. It then got replaced by g_malloc0() when qemu_mallocz() got deprecated and eventually by qemu_memalign() when KVM support was added. Surviving OOM when allocating the HTAB never seemed to be an option until this commit that introduced the check: commit c5f54f3e31bf693f70a98d4d73ea5dbe05689857 Author: David Gibson <david@gibson.dropbear.id.au> Date: Tue Feb 9 10:21:56 2016 +1000 pseries: Move hash page table allocation to reset time I don't really see in the patch and in the changelog an obvious desire to try to handle OOM. > > - } > > - > > memset(spapr->htab, 0, size); > > spapr->htab_shift = shift; > > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > > index 607740150fa2..34e146f628fb 100644 > > --- a/hw/ppc/spapr_hcall.c > > +++ b/hw/ppc/spapr_hcall.c > > @@ -361,12 +361,8 @@ static void *hpt_prepare_thread(void *opaque) > > size_t size = 1ULL << pending->shift; > > > > pending->hpt = qemu_memalign(size, size); > > - if (pending->hpt) { > > - memset(pending->hpt, 0, size); > > - pending->ret = H_SUCCESS; > > - } else { > > - pending->ret = H_NO_MEM; > > Ditto. > This one was introduced by commit: commit 0b0b831016ae93bc14698a5d7202eb77feafea75 Author: David Gibson <david@gibson.dropbear.id.au> Date: Fri May 12 15:46:49 2017 +1000 pseries: Implement HPT resizing I agree that maybe the intent here could have been to use qemu_try_memalign(), but again I don't quite see any strong justification to handle OOM in the changelog. David, Any insight to share ? > > - } > > + memset(pending->hpt, 0, size); > > + pending->ret = H_SUCCESS; > > > > qemu_mutex_lock_iothread(); > > >
On Mon, Oct 26, 2020 at 03:46:47PM +0100, Greg Kurz wrote: > On Mon, 26 Oct 2020 14:43:08 +0100 > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > On 10/26/20 1:40 PM, Greg Kurz wrote: > > > qemu_memalign() aborts if OOM. Drop some dead code. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > --- > > > hw/ppc/spapr.c | 6 ------ > > > hw/ppc/spapr_hcall.c | 8 ++------ > > > 2 files changed, 2 insertions(+), 12 deletions(-) > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index 0cc19b5863a4..f098d0ee6d98 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -1521,12 +1521,6 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > > > int i; > > > > > > spapr->htab = qemu_memalign(size, size); > > > - if (!spapr->htab) { > > > - error_setg_errno(errp, errno, > > > - "Could not allocate HPT of order %d", shift); > > > - return; > > > > Wasn't the idea to use qemu_try_memalign() here? > > > > Well... I have mixed feeling around this. The HTAB was first > introduced by commit: > > commit f43e35255cffb6ac6230dd09d308f7909f823f96 > Author: David Gibson <david@gibson.dropbear.id.au> > Date: Fri Apr 1 15:15:22 2011 +1100 > > Virtual hash page table handling on pSeries machine > > using qemu_mallocz(), which was aborting on OOM. It then got > replaced by g_malloc0() when qemu_mallocz() got deprecated > and eventually by qemu_memalign() when KVM support was added. > > Surviving OOM when allocating the HTAB never seemed to be an > option until this commit that introduced the check: > > commit c5f54f3e31bf693f70a98d4d73ea5dbe05689857 > Author: David Gibson <david@gibson.dropbear.id.au> > Date: Tue Feb 9 10:21:56 2016 +1000 > > pseries: Move hash page table allocation to reset time > > I don't really see in the patch and in the changelog an obvious > desire to try to handle OOM. This one is probably ok. AFAICT all failures returned here would be more or less fatal in the caller, one way or another (&error_fatal in two cases, and failure to load an incoming migration stream in the other). > > > - } > > > - > > > memset(spapr->htab, 0, size); > > > spapr->htab_shift = shift; > > > > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > > > index 607740150fa2..34e146f628fb 100644 > > > --- a/hw/ppc/spapr_hcall.c > > > +++ b/hw/ppc/spapr_hcall.c > > > @@ -361,12 +361,8 @@ static void *hpt_prepare_thread(void *opaque) > > > size_t size = 1ULL << pending->shift; > > > > > > pending->hpt = qemu_memalign(size, size); > > > - if (pending->hpt) { > > > - memset(pending->hpt, 0, size); > > > - pending->ret = H_SUCCESS; > > > - } else { > > > - pending->ret = H_NO_MEM; > > > > Ditto. > > > > This one was introduced by commit: > > commit 0b0b831016ae93bc14698a5d7202eb77feafea75 > Author: David Gibson <david@gibson.dropbear.id.au> > Date: Fri May 12 15:46:49 2017 +1000 > > pseries: Implement HPT resizing > > I agree that maybe the intent here could have been to use qemu_try_memalign(), > but again I don't quite see any strong justification to handle OOM in the > changelog. > > David, > > Any insight to share ? Aborting on an HPT resize failure is definitely not ok, though. This one needs to be a qemu_try_memalign().
On Tue, 27 Oct 2020 12:56:40 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Mon, Oct 26, 2020 at 03:46:47PM +0100, Greg Kurz wrote: > > On Mon, 26 Oct 2020 14:43:08 +0100 > > Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > > > On 10/26/20 1:40 PM, Greg Kurz wrote: > > > > qemu_memalign() aborts if OOM. Drop some dead code. > > > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > --- > > > > hw/ppc/spapr.c | 6 ------ > > > > hw/ppc/spapr_hcall.c | 8 ++------ > > > > 2 files changed, 2 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > > index 0cc19b5863a4..f098d0ee6d98 100644 > > > > --- a/hw/ppc/spapr.c > > > > +++ b/hw/ppc/spapr.c > > > > @@ -1521,12 +1521,6 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, > > > > int i; > > > > > > > > spapr->htab = qemu_memalign(size, size); > > > > - if (!spapr->htab) { > > > > - error_setg_errno(errp, errno, > > > > - "Could not allocate HPT of order %d", shift); > > > > - return; > > > > > > Wasn't the idea to use qemu_try_memalign() here? > > > > > > > Well... I have mixed feeling around this. The HTAB was first > > introduced by commit: > > > > commit f43e35255cffb6ac6230dd09d308f7909f823f96 > > Author: David Gibson <david@gibson.dropbear.id.au> > > Date: Fri Apr 1 15:15:22 2011 +1100 > > > > Virtual hash page table handling on pSeries machine > > > > using qemu_mallocz(), which was aborting on OOM. It then got > > replaced by g_malloc0() when qemu_mallocz() got deprecated > > and eventually by qemu_memalign() when KVM support was added. > > > > Surviving OOM when allocating the HTAB never seemed to be an > > option until this commit that introduced the check: > > > > commit c5f54f3e31bf693f70a98d4d73ea5dbe05689857 > > Author: David Gibson <david@gibson.dropbear.id.au> > > Date: Tue Feb 9 10:21:56 2016 +1000 > > > > pseries: Move hash page table allocation to reset time > > > > I don't really see in the patch and in the changelog an obvious > > desire to try to handle OOM. > > > This one is probably ok. AFAICT all failures returned here would be > more or less fatal in the caller, one way or another (&error_fatal in > two cases, and failure to load an incoming migration stream in the > other). > > > > > - } > > > > - > > > > memset(spapr->htab, 0, size); > > > > spapr->htab_shift = shift; > > > > > > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > > > > index 607740150fa2..34e146f628fb 100644 > > > > --- a/hw/ppc/spapr_hcall.c > > > > +++ b/hw/ppc/spapr_hcall.c > > > > @@ -361,12 +361,8 @@ static void *hpt_prepare_thread(void *opaque) > > > > size_t size = 1ULL << pending->shift; > > > > > > > > pending->hpt = qemu_memalign(size, size); > > > > - if (pending->hpt) { > > > > - memset(pending->hpt, 0, size); > > > > - pending->ret = H_SUCCESS; > > > > - } else { > > > > - pending->ret = H_NO_MEM; > > > > > > Ditto. > > > > > > > This one was introduced by commit: > > > > commit 0b0b831016ae93bc14698a5d7202eb77feafea75 > > Author: David Gibson <david@gibson.dropbear.id.au> > > Date: Fri May 12 15:46:49 2017 +1000 > > > > pseries: Implement HPT resizing > > > > I agree that maybe the intent here could have been to use qemu_try_memalign(), > > but again I don't quite see any strong justification to handle OOM in the > > changelog. > > > > David, > > > > Any insight to share ? > > Aborting on an HPT resize failure is definitely not ok, though. This > one needs to be a qemu_try_memalign(). > Ok, I'll fix that.
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 0cc19b5863a4..f098d0ee6d98 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1521,12 +1521,6 @@ void spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, int i; spapr->htab = qemu_memalign(size, size); - if (!spapr->htab) { - error_setg_errno(errp, errno, - "Could not allocate HPT of order %d", shift); - return; - } - memset(spapr->htab, 0, size); spapr->htab_shift = shift; diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index 607740150fa2..34e146f628fb 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -361,12 +361,8 @@ static void *hpt_prepare_thread(void *opaque) size_t size = 1ULL << pending->shift; pending->hpt = qemu_memalign(size, size); - if (pending->hpt) { - memset(pending->hpt, 0, size); - pending->ret = H_SUCCESS; - } else { - pending->ret = H_NO_MEM; - } + memset(pending->hpt, 0, size); + pending->ret = H_SUCCESS; qemu_mutex_lock_iothread();
qemu_memalign() aborts if OOM. Drop some dead code. Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/ppc/spapr.c | 6 ------ hw/ppc/spapr_hcall.c | 8 ++------ 2 files changed, 2 insertions(+), 12 deletions(-)