Re: [Qemu-ppc] [PATCH 4/5] Migration: migrate ccs_list in spapr state

[David Gibson]

On Thu, Apr 21, 2016 at 10:22:10AM -0700, Jianjun Duan wrote:
> 
> 
> On 04/19/2016 10:14 PM, David Gibson wrote:
> >On Fri, Apr 15, 2016 at 01:33:04PM -0700, Jianjun Duan wrote:
> >>ccs_list in spapr state maintains the device tree related
> >>information on the rtas side for hotplugged devices. In racing
> >>situations between hotplug events and migration operation, a rtas
> >>hotplug event could be migrated from the source guest to target
> >>guest, or the source guest could have not yet finished fetching
> >>the device tree when migration is started, the target will try
> >>to finish fetching the device tree. By migrating ccs_list, the
> >>target can fetch the device tree properly.
> >>
> >>We tracked the size of ccs_list queue, and introduced a dynamic
> >>cache for ccs_list to get around having to create VMSD for the
> >>queue. There also existence tests in place for the newly added
> >>fields in the spapr state VMSD to make sure forward migration
> >>is not broken.
> >>
> >>Signed-off-by: Jianjun Duan <address@hidden>
> >So there's problems here at a couple of levels.
> >
> >1. Even more so that the indicators, this is transitional state, which
> >it would be really nice if we can avoid migrating.  At the very least
> >the new state should go into a subsection with an is_needed function
> >which will skip it if we don't have a configure connector in progress.
> >That means we'll be able to backwards migrate as long as we're not in
> >the middle of a hotplug, which won't be possible with this version.
> >
> >Again, if there's some way we can defer or retry the operation instead
> >of migrating the interim state, that would be even better.
> I am not sure why transitional state should not be migrated.

Because every extra piece of state to migrate is something which can
go wrong.  Getting migration working properly is a difficult and
fragile process as it is - every extra bit of state we add makes it
harder.

> I think the
> fact that it changes is the reason why it should be migrated. As for
> backward migration, I thought about it, but decided to leave it later
> to beat the time. We can do it later, or do it this time and delayed the
> patches more. I agree that subsection seems to be the solution here.

Leaving backwards migration until later - after you've already
introduced a new stream version - will make implementing it much, much
more difficult.

> >2. Having to copy the list of elements out into an array just for
> >migration is pretty horrible.  I'm almost include to suggest we should
> >add list migration into the savevm core rather than this.
> I thought about a general approach of adding something to savevm to
> handle the queue. But the queue used here uses macro and doesn't really
> support polymorphism.  And we need to use  QTAILQ_FOREACH(var, head, field)
> to access it. It is not easy as we may need to modify the macro definition
> to carry
> type name of the elements of the queue.
> 
> Also I am following Alexey's code here ([PATCH qemu v15 07/17] spapr_iommu:
> Migrate full state).
> It was reviewed by you.

Yeah.  It's not incorrect, but it's ugly and every extra time I see
it, the impetus to find a better way increases.

> >>---
> >>  hw/ppc/spapr.c              | 60 
> >> ++++++++++++++++++++++++++++++++++++++++++++-
> >>  hw/ppc/spapr_rtas.c         |  2 ++
> >>  include/hw/ppc/spapr.h      | 11 +++++++++
> >>  include/migration/vmstate.h |  8 +++++-
> >>  4 files changed, 79 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >>index af4745c..eab95f0 100644
> >>--- a/hw/ppc/spapr.c
> >>+++ b/hw/ppc/spapr.c
> >>@@ -1245,10 +1245,31 @@ static bool spapr_vga_init(PCIBus *pci_bus, Error 
> >>**errp)
> >>      }
> >>  }
> >>+static void spapr_pre_save(void *opaque)
> >>+{
> >>+    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> >>+    sPAPRConfigureConnectorState *ccs;
> >>+    sPAPRConfigureConnectorStateCache *ccs_cache;
> >>+
> >>+    /* Copy ccs_list to ccs_list_cache */
> >>+    spapr->ccs_list_cache = g_new0(sPAPRConfigureConnectorStateCache,
> >>+                                   spapr->ccs_list_num);
> >>+    ccs_cache = spapr->ccs_list_cache;
> >>+    QTAILQ_FOREACH(ccs, &spapr->ccs_list, next) {
> >>+        ccs_cache->drc_index = ccs->drc_index;
> >>+        ccs_cache->fdt_offset = ccs->fdt_offset;
> >>+        ccs_cache->fdt_depth = ccs->fdt_depth;
> >>+        ccs_cache++;
> >>+    }
> >>+}
> >>+
> >>  static int spapr_post_load(void *opaque, int version_id)
> >>  {
> >>      sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> >>      int err = 0;
> >>+    sPAPRConfigureConnectorState *ccs;
> >>+    sPAPRConfigureConnectorStateCache *ccs_cache = spapr->ccs_list_cache;
> >>+    int index = 0;
> >>      /* In earlier versions, there was no separate qdev for the PAPR
> >>       * RTC, so the RTC offset was stored directly in sPAPREnvironment.
> >>@@ -1258,6 +1279,19 @@ static int spapr_post_load(void *opaque, int 
> >>version_id)
> >>          err = spapr_rtc_import_offset(spapr->rtc, spapr->rtc_offset);
> >>      }
> >>+    if (version_id < 4) {
> >>+        return err;
> >>+    }
> >>+    /* Copy ccs_list_cache to ccs_list */
> >>+    for (index = 0; index < spapr->ccs_list_num; index ++) {
> >>+        ccs = g_new0(sPAPRConfigureConnectorState, 1);
> >>+        ccs->drc_index = (ccs_cache + index)->drc_index;
> >>+        ccs->fdt_offset = (ccs_cache + index)->fdt_offset;
> >>+        ccs->fdt_depth = (ccs_cache + index)->fdt_depth;
> >>+        QTAILQ_INSERT_TAIL(&spapr->ccs_list, ccs, next);
> >>+    }
> >>+    g_free(spapr->ccs_list_cache);
> >>+
> >>      return err;
> >>  }
> >>@@ -1266,10 +1300,28 @@ static bool version_before_3(void *opaque, int 
> >>version_id)
> >>      return version_id < 3;
> >>  }
> >>+static bool version_ge_4(void *opaque, int version_id)
> >>+{
> >>+    return version_id >= 4;
> >>+}
> >>+
> >>+static const VMStateDescription vmstate_spapr_ccs_cache = {
> >>+    .name = "spaprconfigureconnectorstate",
> >>+    .version_id = 1,
> >>+    .minimum_version_id = 1,
> >>+    .fields = (VMStateField[]) {
> >>+        VMSTATE_UINT32(drc_index, sPAPRConfigureConnectorStateCache),
> >>+        VMSTATE_INT32(fdt_offset, sPAPRConfigureConnectorStateCache),
> >>+        VMSTATE_INT32(fdt_depth, sPAPRConfigureConnectorStateCache),
> >>+        VMSTATE_END_OF_LIST()
> >>+    },
> >>+};
> >>+
> >>  static const VMStateDescription vmstate_spapr = {
> >>      .name = "spapr",
> >>-    .version_id = 3,
> >>+    .version_id = 4,
> >>      .minimum_version_id = 1,
> >>+    .pre_save = spapr_pre_save,
> >>      .post_load = spapr_post_load,
> >>      .fields = (VMStateField[]) {
> >>          /* used to be @next_irq */
> >>@@ -1279,6 +1331,12 @@ static const VMStateDescription vmstate_spapr = {
> >>          VMSTATE_UINT64_TEST(rtc_offset, sPAPRMachineState, 
> >> version_before_3),
> >>          VMSTATE_PPC_TIMEBASE_V(tb, sPAPRMachineState, 2),
> >>+        /* RTAS state */
> >>+        VMSTATE_INT32_TEST(ccs_list_num, sPAPRMachineState, version_ge_4),
> >You don't generally need to write your own version test functions for 
> >specific-version.  Instead you can just use VMSTATE_INT32_V.
> I agree. I realized that after the code was posted here.

Ok.

> >>+        VMSTATE_STRUCT_VARRAY_ALLOC_TEST(ccs_list_cache, sPAPRMachineState,
> >>+                                         version_ge_4, ccs_list_num, 1,
> >>+                                         vmstate_spapr_ccs_cache,
> >>+                                         
> >>sPAPRConfigureConnectorStateCache),
> >>          VMSTATE_END_OF_LIST()
> >>      },
> >>  };
> >>diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >>index f073258..9cfd559 100644
> >>--- a/hw/ppc/spapr_rtas.c
> >>+++ b/hw/ppc/spapr_rtas.c
> >>@@ -70,6 +70,7 @@ static void spapr_ccs_add(sPAPRMachineState *spapr,
> >>  {
> >>      g_assert(!spapr_ccs_find(spapr, ccs->drc_index));
> >>      QTAILQ_INSERT_HEAD(&spapr->ccs_list, ccs, next);
> >>+    spapr->ccs_list_num++;
> >>  }
> >>  static void spapr_ccs_remove(sPAPRMachineState *spapr,
> >>@@ -77,6 +78,7 @@ static void spapr_ccs_remove(sPAPRMachineState *spapr,
> >>  {
> >>      QTAILQ_REMOVE(&spapr->ccs_list, ccs, next);
> >>      g_free(ccs);
> >>+    spapr->ccs_list_num--;
> >>  }
> >>  void spapr_ccs_reset_hook(void *opaque)
> >>diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >>index 815d5ee..c8be926 100644
> >>--- a/include/hw/ppc/spapr.h
> >>+++ b/include/hw/ppc/spapr.h
> >>@@ -11,6 +11,8 @@ struct VIOsPAPRBus;
> >>  struct sPAPRPHBState;
> >>  struct sPAPRNVRAM;
> >>  typedef struct sPAPRConfigureConnectorState sPAPRConfigureConnectorState;
> >>+typedef struct sPAPRConfigureConnectorStateCache
> >>+        sPAPRConfigureConnectorStateCache;
> >>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
> >>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
> >>@@ -75,6 +77,9 @@ struct sPAPRMachineState {
> >>      /* RTAS state */
> >>      QTAILQ_HEAD(, sPAPRConfigureConnectorState) ccs_list;
> >>+    /* Temporary cache for migration purposes */
> >>+    int32_t ccs_list_num;
> >>+    sPAPRConfigureConnectorStateCache *ccs_list_cache;
> >>      /*< public >*/
> >>      char *kvm_type;
> >>@@ -589,6 +594,12 @@ struct sPAPRConfigureConnectorState {
> >>      QTAILQ_ENTRY(sPAPRConfigureConnectorState) next;
> >>  };
> >>+struct sPAPRConfigureConnectorStateCache {
> >>+    uint32_t drc_index;
> >>+    int fdt_offset;
> >>+    int fdt_depth;
> >>+};
> >>+
> >>  void spapr_ccs_reset_hook(void *opaque);
> >>  #define TYPE_SPAPR_RTC "spapr-rtc"
> >>diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> >>index 1622638..7966979 100644
> >>--- a/include/migration/vmstate.h
> >>+++ b/include/migration/vmstate.h
> >>@@ -549,9 +549,10 @@ extern const VMStateInfo vmstate_info_bitmap;
> >>      .offset     = offsetof(_state, _field),                          \
> >>  }
> >>-#define VMSTATE_STRUCT_VARRAY_ALLOC(_field, _state, _field_num, _version, 
> >>_vmsd, _type) {\
> >>+#define VMSTATE_STRUCT_VARRAY_ALLOC_TEST(_field, _state, _test, 
> >>_field_num, _version, _vmsd, _type) { \
> >>      .name       = (stringify(_field)),                               \
> >>      .version_id = (_version),                                        \
> >>+    .field_exists = (_test),                                         \
> >>      .vmsd       = &(_vmsd),                                          \
> >>      .num_offset = vmstate_offset_value(_state, _field_num, int32_t), \
> >>      .size       = sizeof(_type),                                     \
> >>@@ -677,6 +678,11 @@ extern const VMStateInfo vmstate_info_bitmap;
> >>      VMSTATE_STRUCT_ARRAY_TEST(_field, _state, _num, NULL, _version,   \
> >>              _vmsd, _type)
> >>+#define VMSTATE_STRUCT_VARRAY_ALLOC(_field, _state, _field_num, _version, \
> >>+                                    _vmsd, _type)                         \
> >>+    VMSTATE_STRUCT_VARRAY_ALLOC_TEST(_field, _state, NULL, _field_num,    \
> >>+            _version, _vmsd, _type)
> >>+
> >>  #define VMSTATE_BUFFER_UNSAFE_INFO(_field, _state, _version, _info, 
> >> _size) \
> >>      VMSTATE_BUFFER_UNSAFE_INFO_TEST(_field, _state, NULL, _version, 
> >> _info, \
> >>              _size)
> 

-- 
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

signature.asc
Description: PGP signature