Re: [Qemu-devel] [PATCH v6 1/2] spapr: Add a "no HPT" encoding to HTAB migration stream

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

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

On Tue, Jun 13, 2017 at 10:18:18AM +0530, Bharata B Rao wrote:
> On Mon, Jun 12, 2017 at 05:10:44PM +0800, David Gibson wrote:
> > On Mon, Jun 12, 2017 at 11:02:34AM +0530, Bharata B Rao wrote:
> > > Add a "no HPT" encoding (using value -1) to the HTAB migration
> > > stream (in the place of HPT size) when the guest doesn't allocate HPT.
> > > This will help the target side to match target HPT with the source HPT
> > > and thus enable successful migration.
> > > 
> > > Suggested-by: David Gibson <david@gibson.dropbear.id.au>
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr.c | 29 +++++++++++++++++++++++++----
> > >  1 file changed, 25 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 8b541d9..c425499 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1558,13 +1558,19 @@ static int htab_save_setup(QEMUFile *f, void *opaque)
> > >      sPAPRMachineState *spapr = opaque;
> > >  
> > >      /* "Iteration" header */
> > > -    qemu_put_be32(f, spapr->htab_shift);
> > > +    if (!spapr->htab_shift) {
> > > +        qemu_put_be32(f, -1);
> > > +    } else {
> > > +        qemu_put_be32(f, spapr->htab_shift);
> > > +    }
> > >  
> > >      if (spapr->htab) {
> > >          spapr->htab_save_index = 0;
> > >          spapr->htab_first_pass = true;
> > >      } else {
> > > -        assert(kvm_enabled());
> > > +        if (spapr->htab_shift) {
> > > +            assert(kvm_enabled());
> > > +        }
> > >      }
> > >  
> > >  
> > > @@ -1710,7 +1716,12 @@ static int htab_save_iterate(QEMUFile *f, void *opaque)
> > >      int rc = 0;
> > >  
> > >      /* Iteration header */
> > > -    qemu_put_be32(f, 0);
> > > +    if (!spapr->htab_shift) {
> > > +        qemu_put_be32(f, -1);
> > > +        return 0;
> > > +    } else {
> > > +        qemu_put_be32(f, 0);
> > > +    }
> > >  
> > >      if (!spapr->htab) {
> > >          assert(kvm_enabled());
> > > @@ -1744,7 +1755,12 @@ static int htab_save_complete(QEMUFile *f, void *opaque)
> > >      int fd;
> > >  
> > >      /* Iteration header */
> > > -    qemu_put_be32(f, 0);
> > > +    if (!spapr->htab_shift) {
> > > +        qemu_put_be32(f, -1);
> > > +        return 0;
> > > +    } else {
> > > +        qemu_put_be32(f, 0);
> > > +    }
> > 
> > Do you actually need the modifications for _iterate and _complete?  I
> > would have thought you just wouldn't need to send any more of the HPT
> > stream at all after sending the -1 header.  
> 
> _setup, _iterate, _complete handler routines for HTAB always get interspersed
> with similar routines for ram savevm handlers as per what I have seen.
> And moreover these are called by the core migration code and hence we they get
> called, we need these changes to ensure that we don't attempt to send HPT
> stream.

Ah, yes of course.

> > We should also adjust the downtime estimation logic so we don't allow
> > for transferring an HPT that isn't there.
> 
> I will have to check that out.
> 
> > 
> > >      if (!spapr->htab) {
> > >          int rc;
> > > @@ -1788,6 +1804,11 @@ static int htab_load(QEMUFile *f, void *opaque, int version_id)
> > >  
> > >      section_hdr = qemu_get_be32(f);
> > >  
> > > +    if (section_hdr == -1) {
> > > +        spapr_free_hpt(spapr);
> > > +        return 0;
> > > +    }
> > 
> > Strictly speaking we probably shouldn't just return here.  We should
> > wait and see if there is more data in the stream.
> 
> Because of the way the source sends data (with HPT data getting
> interspersed with ram data, I don't think we can say for sure if
> we got or will get any HPT data following the no-HPT indication.

We definitely shouldn't - there's no way the destination could handle
it without knowing the size first.  We could get a new header giving
an HPT size, and then get HPT data.


> > Any actual content
> > (i.e. section_hdr == 0 sections) would be an error at this point.
> > However, a reset to an HPT guest would be represented by a new
> > non-zero section header, then more data.  This isn't urgent, but it
> > would be nice to fix at some point.
> 
> Sure.
> 
> Regards,
> Bharata.
> 

-- 
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: 819 bytes --]