To allow for a DIMM unplug event to resume its work if a migration
occurs in the middle of it, this patch migrates the non-empty
pending_dimm_unplugs QTAILQ that stores the DIMM information
that the spapr_lmb_release() callback uses.

It was considered an apprach where the DIMM states would be restored
on the post-_load after a migration. The problem is that there is
no way of knowing, from the sPAPRMachineState, if a given DIMM is going
through an unplug process and the callback needs the updated DIMM State.

We could migrate a flag indicating that there is an unplug event going
on for a certain DIMM, fetching this information from the start of the
spapr_del_lmbs call. But this would also require a scan on post_load to
figure out how many nr_lmbs are left. At this point we can just
migrate the nr_lmbs information as well, given that it is being calculated
at spapr_del_lmbs already, and spare a scanning/discovery in the
post-load. All that we need is inside the sPAPRDIMMState structure
that is added to the pending_dimm_unplugs queue at the start of the
spapr_del_lmbs, so it's convenient to just migrated this queue it if it's
not empty.

Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e190eb9..30f0b7b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1437,6 +1437,36 @@ static bool version_before_3(void *opaque, int version_id)
     return version_id < 3;
 }
 
+static bool spapr_pending_dimm_unplugs_needed(void *opaque)
+{
+    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
+    return !QTAILQ_EMPTY(&spapr->pending_dimm_unplugs);
+}
+
+static const VMStateDescription vmstate_spapr_dimmstate = {
+    .name = "spapr_dimm_state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(addr, sPAPRDIMMState),
+        VMSTATE_UINT32(nr_lmbs, sPAPRDIMMState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static const VMStateDescription vmstate_spapr_pending_dimm_unplugs = {
+    .name = "spapr_pending_dimm_unplugs",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = spapr_pending_dimm_unplugs_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_QTAILQ_V(pending_dimm_unplugs, sPAPRMachineState, 1,
+                         vmstate_spapr_dimmstate, sPAPRDIMMState,
+                         next),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 static bool spapr_ov5_cas_needed(void *opaque)
 {
     sPAPRMachineState *spapr = opaque;
@@ -1535,6 +1565,7 @@ static const VMStateDescription vmstate_spapr = {
     .subsections = (const VMStateDescription*[]) {
         &vmstate_spapr_ov5_cas,
         &vmstate_spapr_patb_entry,
+        &vmstate_spapr_pending_dimm_unplugs,
         NULL
     }
 };
-- 
2.9.3

On Fri, May 05, 2017 at 05:47:44PM -0300, Daniel Henrique Barboza wrote:
> To allow for a DIMM unplug event to resume its work if a migration
> occurs in the middle of it, this patch migrates the non-empty
> pending_dimm_unplugs QTAILQ that stores the DIMM information
> that the spapr_lmb_release() callback uses.
> 
> It was considered an apprach where the DIMM states would be restored
> on the post-_load after a migration. The problem is that there is
> no way of knowing, from the sPAPRMachineState, if a given DIMM is going
> through an unplug process and the callback needs the updated DIMM State.
> 
> We could migrate a flag indicating that there is an unplug event going
> on for a certain DIMM, fetching this information from the start of the
> spapr_del_lmbs call. But this would also require a scan on post_load to
> figure out how many nr_lmbs are left. At this point we can just
> migrate the nr_lmbs information as well, given that it is being calculated
> at spapr_del_lmbs already, and spare a scanning/discovery in the
> post-load. All that we need is inside the sPAPRDIMMState structure
> that is added to the pending_dimm_unplugs queue at the start of the
> spapr_del_lmbs, so it's convenient to just migrated this queue it if it's
> not empty.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>

NACK.

As I believe I suggested previously, you can reconstruct this state on
the receiving side by doing a full scan of the DIMM and LMB DRC states.

> ---
>  hw/ppc/spapr.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e190eb9..30f0b7b 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1437,6 +1437,36 @@ static bool version_before_3(void *opaque, int version_id)
>      return version_id < 3;
>  }
>  
> +static bool spapr_pending_dimm_unplugs_needed(void *opaque)
> +{
> +    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> +    return !QTAILQ_EMPTY(&spapr->pending_dimm_unplugs);
> +}
> +
> +static const VMStateDescription vmstate_spapr_dimmstate = {
> +    .name = "spapr_dimm_state",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(addr, sPAPRDIMMState),
> +        VMSTATE_UINT32(nr_lmbs, sPAPRDIMMState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static const VMStateDescription vmstate_spapr_pending_dimm_unplugs = {
> +    .name = "spapr_pending_dimm_unplugs",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = spapr_pending_dimm_unplugs_needed,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_QTAILQ_V(pending_dimm_unplugs, sPAPRMachineState, 1,
> +                         vmstate_spapr_dimmstate, sPAPRDIMMState,
> +                         next),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static bool spapr_ov5_cas_needed(void *opaque)
>  {
>      sPAPRMachineState *spapr = opaque;
> @@ -1535,6 +1565,7 @@ static const VMStateDescription vmstate_spapr = {
>      .subsections = (const VMStateDescription*[]) {
>          &vmstate_spapr_ov5_cas,
>          &vmstate_spapr_patb_entry,
> +        &vmstate_spapr_pending_dimm_unplugs,
>          NULL
>      }
>  };

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

On 05/12/2017 03:12 AM, David Gibson wrote:
> On Fri, May 05, 2017 at 05:47:44PM -0300, Daniel Henrique Barboza wrote:
>> To allow for a DIMM unplug event to resume its work if a migration
>> occurs in the middle of it, this patch migrates the non-empty
>> pending_dimm_unplugs QTAILQ that stores the DIMM information
>> that the spapr_lmb_release() callback uses.
>>
>> It was considered an apprach where the DIMM states would be restored
>> on the post-_load after a migration. The problem is that there is
>> no way of knowing, from the sPAPRMachineState, if a given DIMM is going
>> through an unplug process and the callback needs the updated DIMM State.
>>
>> We could migrate a flag indicating that there is an unplug event going
>> on for a certain DIMM, fetching this information from the start of the
>> spapr_del_lmbs call. But this would also require a scan on post_load to
>> figure out how many nr_lmbs are left. At this point we can just
>> migrate the nr_lmbs information as well, given that it is being calculated
>> at spapr_del_lmbs already, and spare a scanning/discovery in the
>> post-load. All that we need is inside the sPAPRDIMMState structure
>> that is added to the pending_dimm_unplugs queue at the start of the
>> spapr_del_lmbs, so it's convenient to just migrated this queue it if it's
>> not empty.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> NACK.
>
> As I believe I suggested previously, you can reconstruct this state on
> the receiving side by doing a full scan of the DIMM and LMB DRC states.

Just had an idea that I think it's in the line of what you're 
suggesting. Given
that the information we need is only created in the spapr_del_lmbs
(as per patch 1), we can use the absence of this information in the
release callback as a sort of a flag, an indication that a migration got
in the way and we need to reconstruct the nr_lmbs states again, using
the same scanning function I've used in v8.

The flow would be like this (considering the changes in the
previous 3 patches so far):

------------

/* Callback to be called during DRC release. */
void spapr_lmb_release(DeviceState *dev)
{
      HotplugHandler *hotplug_ctrl;

      uint64_t addr = spapr_dimm_get_address(PC_DIMM(dev));
      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
      sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, addr);

     // no DIMM state found in spapr - re-create it to find out how may 
LMBs are left
     if (ds == NULL) {
         uint32 nr_lmbs  = ***call_scanning_LMB_DRCs_function(dev)***
         // recreate the sPAPRDIMMState element and add it back to spapr
     }

     ( resume callback as usual )

-----------

Is this approach be adequate? Another alternative would be to use another
way of detecting if an LMB unplug is happening and, if positive, do the same
process in the post_load(). In this case I'll need to take a look in the 
code and
see how we can detect an ongoing unplug besides what I've said above.


Thanks,


Daniel

>
>> ---
>>   hw/ppc/spapr.c | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index e190eb9..30f0b7b 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -1437,6 +1437,36 @@ static bool version_before_3(void *opaque, int version_id)
>>       return version_id < 3;
>>   }
>>   
>> +static bool spapr_pending_dimm_unplugs_needed(void *opaque)
>> +{
>> +    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
>> +    return !QTAILQ_EMPTY(&spapr->pending_dimm_unplugs);
>> +}
>> +
>> +static const VMStateDescription vmstate_spapr_dimmstate = {
>> +    .name = "spapr_dimm_state",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT64(addr, sPAPRDIMMState),
>> +        VMSTATE_UINT32(nr_lmbs, sPAPRDIMMState),
>> +        VMSTATE_END_OF_LIST()
>> +    },
>> +};
>> +
>> +static const VMStateDescription vmstate_spapr_pending_dimm_unplugs = {
>> +    .name = "spapr_pending_dimm_unplugs",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .needed = spapr_pending_dimm_unplugs_needed,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_QTAILQ_V(pending_dimm_unplugs, sPAPRMachineState, 1,
>> +                         vmstate_spapr_dimmstate, sPAPRDIMMState,
>> +                         next),
>> +        VMSTATE_END_OF_LIST()
>> +    },
>> +};
>> +
>>   static bool spapr_ov5_cas_needed(void *opaque)
>>   {
>>       sPAPRMachineState *spapr = opaque;
>> @@ -1535,6 +1565,7 @@ static const VMStateDescription vmstate_spapr = {
>>       .subsections = (const VMStateDescription*[]) {
>>           &vmstate_spapr_ov5_cas,
>>           &vmstate_spapr_patb_entry,
>> +        &vmstate_spapr_pending_dimm_unplugs,
>>           NULL
>>       }
>>   };

Quoting Daniel Henrique Barboza (2017-05-12 14:54:57)
> 
> 
> On 05/12/2017 03:12 AM, David Gibson wrote:
> > On Fri, May 05, 2017 at 05:47:44PM -0300, Daniel Henrique Barboza wrote:
> >> To allow for a DIMM unplug event to resume its work if a migration
> >> occurs in the middle of it, this patch migrates the non-empty
> >> pending_dimm_unplugs QTAILQ that stores the DIMM information
> >> that the spapr_lmb_release() callback uses.
> >>
> >> It was considered an apprach where the DIMM states would be restored
> >> on the post-_load after a migration. The problem is that there is
> >> no way of knowing, from the sPAPRMachineState, if a given DIMM is going
> >> through an unplug process and the callback needs the updated DIMM State.
> >>
> >> We could migrate a flag indicating that there is an unplug event going
> >> on for a certain DIMM, fetching this information from the start of the
> >> spapr_del_lmbs call. But this would also require a scan on post_load to
> >> figure out how many nr_lmbs are left. At this point we can just
> >> migrate the nr_lmbs information as well, given that it is being calculated
> >> at spapr_del_lmbs already, and spare a scanning/discovery in the
> >> post-load. All that we need is inside the sPAPRDIMMState structure
> >> that is added to the pending_dimm_unplugs queue at the start of the
> >> spapr_del_lmbs, so it's convenient to just migrated this queue it if it's
> >> not empty.
> >>
> >> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> > NACK.
> >
> > As I believe I suggested previously, you can reconstruct this state on
> > the receiving side by doing a full scan of the DIMM and LMB DRC states.
> 
> Just had an idea that I think it's in the line of what you're 
> suggesting. Given
> that the information we need is only created in the spapr_del_lmbs
> (as per patch 1), we can use the absence of this information in the
> release callback as a sort of a flag, an indication that a migration got
> in the way and we need to reconstruct the nr_lmbs states again, using
> the same scanning function I've used in v8.
> 
> The flow would be like this (considering the changes in the
> previous 3 patches so far):
> 
> ------------
> 
> /* Callback to be called during DRC release. */
> void spapr_lmb_release(DeviceState *dev)
> {
>       HotplugHandler *hotplug_ctrl;
> 
>       uint64_t addr = spapr_dimm_get_address(PC_DIMM(dev));
>       sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>       sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, addr);
> 
>      // no DIMM state found in spapr - re-create it to find out how may 
> LMBs are left
>      if (ds == NULL) {
>          uint32 nr_lmbs  = ***call_scanning_LMB_DRCs_function(dev)***
>          // recreate the sPAPRDIMMState element and add it back to spapr
>      }
> 
>      ( resume callback as usual )
> 
> -----------
> 
> Is this approach be adequate? Another alternative would be to use another

Seems reasonable to me.

> way of detecting if an LMB unplug is happening and, if positive, do the same
> process in the post_load(). In this case I'll need to take a look in the 
> code and
> see how we can detect an ongoing unplug besides what I've said above.

You could scan DRCs/LMBs for each DIMM and generate an sPAPRDIMMState for any
case where DRCs/LMBs are not all present. But doing it the way you've
proposed above lets us re-generate them as needed and avoid the exhaustive
scan. I'd be sure to document it in the comments as a migration-specific hook
though since you lose that understanding by moving it out of postload and
future refactorings might forget why we have a hook that regenerates it
even though spapr_del_lmbs always sets it initially.

> 
> 
> Thanks,
> 
> 
> Daniel
> 
> >
> >> ---
> >>   hw/ppc/spapr.c | 31 +++++++++++++++++++++++++++++++
> >>   1 file changed, 31 insertions(+)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index e190eb9..30f0b7b 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -1437,6 +1437,36 @@ static bool version_before_3(void *opaque, int version_id)
> >>       return version_id < 3;
> >>   }
> >>   
> >> +static bool spapr_pending_dimm_unplugs_needed(void *opaque)
> >> +{
> >> +    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> >> +    return !QTAILQ_EMPTY(&spapr->pending_dimm_unplugs);
> >> +}
> >> +
> >> +static const VMStateDescription vmstate_spapr_dimmstate = {
> >> +    .name = "spapr_dimm_state",
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_UINT64(addr, sPAPRDIMMState),
> >> +        VMSTATE_UINT32(nr_lmbs, sPAPRDIMMState),
> >> +        VMSTATE_END_OF_LIST()
> >> +    },
> >> +};
> >> +
> >> +static const VMStateDescription vmstate_spapr_pending_dimm_unplugs = {
> >> +    .name = "spapr_pending_dimm_unplugs",
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >> +    .needed = spapr_pending_dimm_unplugs_needed,
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_QTAILQ_V(pending_dimm_unplugs, sPAPRMachineState, 1,
> >> +                         vmstate_spapr_dimmstate, sPAPRDIMMState,
> >> +                         next),
> >> +        VMSTATE_END_OF_LIST()
> >> +    },
> >> +};
> >> +
> >>   static bool spapr_ov5_cas_needed(void *opaque)
> >>   {
> >>       sPAPRMachineState *spapr = opaque;
> >> @@ -1535,6 +1565,7 @@ static const VMStateDescription vmstate_spapr = {
> >>       .subsections = (const VMStateDescription*[]) {
> >>           &vmstate_spapr_ov5_cas,
> >>           &vmstate_spapr_patb_entry,
> >> +        &vmstate_spapr_pending_dimm_unplugs,
> >>           NULL
> >>       }
> >>   };
> 

On Fri, May 12, 2017 at 04:54:57PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 05/12/2017 03:12 AM, David Gibson wrote:
> > On Fri, May 05, 2017 at 05:47:44PM -0300, Daniel Henrique Barboza wrote:
> > > To allow for a DIMM unplug event to resume its work if a migration
> > > occurs in the middle of it, this patch migrates the non-empty
> > > pending_dimm_unplugs QTAILQ that stores the DIMM information
> > > that the spapr_lmb_release() callback uses.
> > > 
> > > It was considered an apprach where the DIMM states would be restored
> > > on the post-_load after a migration. The problem is that there is
> > > no way of knowing, from the sPAPRMachineState, if a given DIMM is going
> > > through an unplug process and the callback needs the updated DIMM State.
> > > 
> > > We could migrate a flag indicating that there is an unplug event going
> > > on for a certain DIMM, fetching this information from the start of the
> > > spapr_del_lmbs call. But this would also require a scan on post_load to
> > > figure out how many nr_lmbs are left. At this point we can just
> > > migrate the nr_lmbs information as well, given that it is being calculated
> > > at spapr_del_lmbs already, and spare a scanning/discovery in the
> > > post-load. All that we need is inside the sPAPRDIMMState structure
> > > that is added to the pending_dimm_unplugs queue at the start of the
> > > spapr_del_lmbs, so it's convenient to just migrated this queue it if it's
> > > not empty.
> > > 
> > > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com>
> > NACK.
> > 
> > As I believe I suggested previously, you can reconstruct this state on
> > the receiving side by doing a full scan of the DIMM and LMB DRC states.
> 
> Just had an idea that I think it's in the line of what you're suggesting.
> Given
> that the information we need is only created in the spapr_del_lmbs
> (as per patch 1), we can use the absence of this information in the
> release callback as a sort of a flag, an indication that a migration got
> in the way and we need to reconstruct the nr_lmbs states again, using
> the same scanning function I've used in v8.
> 
> The flow would be like this (considering the changes in the
> previous 3 patches so far):
> 
> ------------
> 
> /* Callback to be called during DRC release. */
> void spapr_lmb_release(DeviceState *dev)
> {
>      HotplugHandler *hotplug_ctrl;
> 
>      uint64_t addr = spapr_dimm_get_address(PC_DIMM(dev));
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, addr);
> 
>     // no DIMM state found in spapr - re-create it to find out how may LMBs
> are left
>     if (ds == NULL) {
>         uint32 nr_lmbs  = ***call_scanning_LMB_DRCs_function(dev)***
>         // recreate the sPAPRDIMMState element and add it back to spapr
>     }
> 
>     ( resume callback as usual )
> 
> -----------

Yes, the above seems like a reasonable plan.

> Is this approach be adequate? Another alternative would be to use another
> way of detecting if an LMB unplug is happening and, if positive, do the same
> process in the post_load(). In this case I'll need to take a look in the
> code and
> see how we can detect an ongoing unplug besides what I've said above.

You could, but I think the lazy approach above is preferable.

> 
> Thanks,
> 
> 
> Daniel
> 
> > 
> > > ---
> > >   hw/ppc/spapr.c | 31 +++++++++++++++++++++++++++++++
> > >   1 file changed, 31 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index e190eb9..30f0b7b 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1437,6 +1437,36 @@ static bool version_before_3(void *opaque, int version_id)
> > >       return version_id < 3;
> > >   }
> > > +static bool spapr_pending_dimm_unplugs_needed(void *opaque)
> > > +{
> > > +    sPAPRMachineState *spapr = (sPAPRMachineState *)opaque;
> > > +    return !QTAILQ_EMPTY(&spapr->pending_dimm_unplugs);
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_spapr_dimmstate = {
> > > +    .name = "spapr_dimm_state",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_UINT64(addr, sPAPRDIMMState),
> > > +        VMSTATE_UINT32(nr_lmbs, sPAPRDIMMState),
> > > +        VMSTATE_END_OF_LIST()
> > > +    },
> > > +};
> > > +
> > > +static const VMStateDescription vmstate_spapr_pending_dimm_unplugs = {
> > > +    .name = "spapr_pending_dimm_unplugs",
> > > +    .version_id = 1,
> > > +    .minimum_version_id = 1,
> > > +    .needed = spapr_pending_dimm_unplugs_needed,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_QTAILQ_V(pending_dimm_unplugs, sPAPRMachineState, 1,
> > > +                         vmstate_spapr_dimmstate, sPAPRDIMMState,
> > > +                         next),
> > > +        VMSTATE_END_OF_LIST()
> > > +    },
> > > +};
> > > +
> > >   static bool spapr_ov5_cas_needed(void *opaque)
> > >   {
> > >       sPAPRMachineState *spapr = opaque;
> > > @@ -1535,6 +1565,7 @@ static const VMStateDescription vmstate_spapr = {
> > >       .subsections = (const VMStateDescription*[]) {
> > >           &vmstate_spapr_ov5_cas,
> > >           &vmstate_spapr_patb_entry,
> > > +        &vmstate_spapr_pending_dimm_unplugs,
> > >           NULL
> > >       }
> > >   };
> 

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