| Message ID | 1497245555-32472-2-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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. We should also adjust the downtime estimation logic so we don't allow for transferring an HPT that isn't there. > 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. 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. > if (section_hdr) { > Error *local_err = NULL; >
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. > > 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. > 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.
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. >
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); + } 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; + } + if (section_hdr) { Error *local_err = NULL;
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(-)