| Message ID | 20170505204746.14116-5-danielhb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 > } > };
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 > > > } > > > }; >
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 } };
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(+)