Re: [PATCH] spapr: Improve handling of memory unplug with old guests

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3500 bytes --]

On Wed, Jan 13, 2021 at 06:01:27PM +0100, Greg Kurz wrote:
> On Wed, 13 Jan 2021 12:20:58 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Jan 08, 2021 at 06:31:27PM +0100, Greg Kurz wrote:
> > > Since commit 1e8b5b1aa16b ("spapr: Allow memory unplug to always succeed")
> > > trying to unplug memory from a guest that doesn't support it (eg. rhel6)
> > > no longer generates an error like it used to. Instead, it leaves the
> > > memory around : only a subsequent reboot or manual use of drmgr within
> > > the guest can complete the hot-unplug sequence. A flag was added to
> > > SpaprMachineClass so that this new behavior only applies to the default
> > > machine type.
> > > 
> > > We can do better. CAS processes all pending hot-unplug requests. This
> > > means that we don't really care about what the guest supports if
> > > the hot-unplug request happens before CAS.
> > > 
> > > All guests that we care for, even old ones, set enough bits in OV5
> > > that lead to a non-empty bitmap in spapr->ov5_cas. Use that as a
> > > heuristic to decide if CAS has already occured or not.
> > > 
> > > Always accept unplug requests that happen before CAS since CAS will
> > > process them. Restore the previous behavior of rejecting them after
> > > CAS when we know that the guest doesn't support memory hot-unplug.
> > > 
> > > This behavior is suitable for all machine types : this allows to
> > > drop the pre_6_0_memory_unplug flag.
> > > 
> > > Fixes: 1e8b5b1aa16b ("spapr: Allow memory unplug to always succeed")
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > 
> > Applied, sorry it too me so long.
> > 
> 
> No problem. Any estimate for your next PR ?

Intending to do it tomorrow (Tuesday 19th).

> 
> > > ---
> > >  hw/ppc/spapr.c              |   24 +++++++++++++-----------
> > >  hw/ppc/spapr_events.c       |    3 +--
> > >  hw/ppc/spapr_ovec.c         |    7 +++++++
> > >  include/hw/ppc/spapr.h      |    2 +-
> > >  include/hw/ppc/spapr_ovec.h |    1 +
> > >  5 files changed, 23 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 2c403b574e37..6c47466fc2f1 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -4048,6 +4048,18 @@ static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev,
> > >      }
> > >  }
> > >  
> > > +bool spapr_memory_hot_unplug_supported(SpaprMachineState *spapr)
> > > +{
> > > +    return spapr_ovec_test(spapr->ov5_cas, OV5_HP_EVT) ||
> > > +        /*
> > > +         * CAS will process all pending unplug requests.
> > > +         *
> > > +         * HACK: a guest could theoretically have cleared all bits in OV5,
> > > +         * but none of the guests we care for do.
> > > +         */
> > 
> > Hrm.  This is pretty ugly - I thought we had a better canonical way of
> > determining if CAS had already happened this boot, but it appears
> > not.  I don't want to delay this patch, since it is an important fix,
> > but it would be nice if you could do a later cleanup to have a nicer
> > way of detecting CAS-hasn't-happened.
> > 
> 
> Yeah, I fully agree this is ugly. I'll try to find something nicer later.
> 
> Thanks for taking it anyway !
> 
> Cheers,
> 



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]