On 05/03/2017 12:26 AM, Bharata B Rao wrote:
On Tue, May 2, 2017 at 1:13 PM, Daniel Henrique Barboza
<address@hidden <mailto:address@hidden>>
wrote:
On 05/02/2017 12:40 AM, Bharata B Rao wrote:
On Sun, Apr 30, 2017 at 10:55 PM, Daniel Henrique Barboza
<address@hidden
<mailto:address@hidden>> wrote:
Following up the previous detach_cb change, this patch
removes the
detach_cb_opaque entirely from the code.
The reason is that the drc->detach_cb_opaque object can't be
restored in the post load of the upcoming DRC migration and
no detach
callbacks actually need this opaque. 'spapr_core_release' is
receiving it as NULL, 'spapr_phb_remove_pci_device_cb' is
receiving
a phb object as opaque but is't using it. These were trivial
removal
cases.
However, the LM removal callback 'spapr_lmb_release' is
receiving
and using the opaque object, a 'sPAPRDIMMState' struct. This
struct
holds the number of LMBs the DIMM object contains and the
callback
was using this counter as a countdown to check if all LMB
DRCs were
release before proceeding to the DIMM unplug. To remove the
need of
this callback we have choices such as:
- migrate the 'sPAPRDIMMState' struct. This would require
creating a
QTAILQ to store all DIMMStates and an additional 'dimm_id'
field to
associate the DIMMState with the DIMM object. We could attach
this
QTAILQ to the 'sPAPRPHBState' and retrieve it later in the
callback.
- fetch the state of the LMB DRCs directly by scanning the
state of
them and, if all of them are released, proceed with the DIMM
unplug.
The second approach was chosen. The new
'spapr_all_lmbs_drcs_released'
function scans all LMBs of a given DIMM device to see if
their DRC
state are inactive. If all of them are inactive return
'true', 'false'
otherwise. This function is being called inside the
'spapr_lmb_release'
callback, replacing the role of the 'sPAPRDIMMState'
opaque. The
'sPAPRDIMMState' struct was removed from the code given that
there are
no more uses for it.
After all these changes, there are no roles left for the
'detach_cb_opaque'
attribute of the 'sPAPRDRConnector' as well, so we can safely
remove
it from the code too.
Signed-off-by: Daniel Henrique Barboza
<address@hidden
<mailto:address@hidden>>
---
hw/ppc/spapr.c | 46
+++++++++++++++++++++++++++++++++-------------
hw/ppc/spapr_drc.c | 16 +++++-----------
hw/ppc/spapr_pci.c | 4 ++--
include/hw/ppc/spapr_drc.h | 6 ++----
4 files changed, 42 insertions(+), 30 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index bc11757..8b9a6cf 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1887,21 +1887,43 @@ static void spapr_drc_reset(void
*opaque)
}
}
-typedef struct sPAPRDIMMState {
- uint32_t nr_lmbs;
-} sPAPRDIMMState;
+static bool spapr_all_lmbs_drcs_released(PCDIMMDevice *dimm)
+{
+ Error *local_err = NULL;
+ PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm);
+ MemoryRegion *mr = ddc->get_memory_region(dimm);
+ uint64_t size = memory_region_size(mr);
+
+ uint64_t addr;
+ addr = object_property_get_int(OBJECT(dimm),
PC_DIMM_ADDR_PROP, &local_err);
+ if (local_err) {
+ error_propagate(&error_abort, local_err);
+ return false;
+ }
+ uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
-static void spapr_lmb_release(DeviceState *dev, void *opaque)
+ sPAPRDRConnector *drc;
+ int i = 0;
+ for (i = 0; i < nr_lmbs; i++) {
+ drc =
spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
+ addr / SPAPR_MEMORY_BLOCK_SIZE);
+ g_assert(drc);
+ if (drc->indicator_state !=
SPAPR_DR_INDICATOR_STATE_INACTIVE) {
+ return false;
+ }
+ addr += SPAPR_MEMORY_BLOCK_SIZE;
+ }
+ return true;
+}
+
+static void spapr_lmb_release(DeviceState *dev)
{
- sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque;
HotplugHandler *hotplug_ctrl;
- if (--ds->nr_lmbs) {
+ if (!spapr_all_lmbs_drcs_released(PC_DIMM(dev))) {
return;
}
I am concerned about the number of times we walk the DRC list
corresponding to each DIMM device. When a DIMM device is being
removed, spapr_lmb_release() will be invoked for each of the LMBs
of that DIMM. Now in this scheme, we end up walking through all
the DRC objects of the DIMM from every LMB's release function.
Hi Bharata,
I agree, this is definitely a poorer performance than simply
decrementing ds->nr_lmbs.
The reasons why I went on with it:
- hot unplug isn't an operation that happens too often, so it's
not terrible
to have a delay increase here;
- it didn't increased the unplug delay in an human noticeable way,
at least in
my tests;
- apart from migrating the information, there is nothing much we
can do in the
callback side about it. The callback isn't aware of the current
state of the DIMM
removal process, so the scanning is required every time.
All that said, assuming that the process of DIMM removal will
always go through
'spapr_del_lmbs', why do we need this callback? Can't we simply do
something
like this in spapr_del_lmbs?
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index cd42449..e443fea 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2734,6 +2734,20 @@ static void spapr_del_lmbs(DeviceState
*dev, uint64_t addr_start, uint64_t size,
addr += SPAPR_MEMORY_BLOCK_SIZE;
}
+ if (!spapr_all_lmbs_drcs_released(PC_DIMM(dev))) {
+ // something went wrong in the removal of the LMBs.
+ // propagate error and return
+ throw_error_code;
+ return;
+ }
spapr_del_lmbs() is called from ->unplug_request(). Here we notify
the guest about the unplug request. We have to wait till the guest
gives us a go ahead so that we can cleanup the DIMM device. The
cleanup is done as part of release callback (spapr_lmb_release) at
which point we are sure that the given LMB has been indeed removed
by the guest.
I wasn't clear enough in my last comment. Let me rephrase. Is there any
other use for
the 'spapr_lmb_release' callback function other than being called by the
spapr_del_lmbs()
in the flow you've stated above? Searching the master code now I've
found:
$ grep -R 'spapr_lmb_release' .
./spapr.c:static void spapr_lmb_release(DeviceState *dev, void *opaque)
./spapr.c: drck->detach(drc, dev, spapr_lmb_release, ds, errp);
Note that all the callback is doing is asserting that a nr_lmb counter
will be zero after
a decrement and, if true, execute the following:
hotplug_ctrl = qdev_get_hotplug_handler(dev);
hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
So, if the callback spapr_lmb_release is only being called in the
following for loop of spapr_del_lmbs()
to clean up each LMB DRC, can't we get rid of it and do the following
after this for loop?
for (i = 0; i < nr_lmbs; i++) {
drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
addr / SPAPR_MEMORY_BLOCK_SIZE);
g_assert(drc);
drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
drck->detach(drc, dev, ds, errp);
addr += SPAPR_MEMORY_BLOCK_SIZE;
}
if (!spapr_all_lmbs_drcs_released(PC_DIMM(dev))) {
// All LMBs were cleared, proceed with detach
hotplug_ctrl = qdev_get_hotplug_handler(dev);
hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort);
}
// proceed with spapr_del_lmbs code
Doesn't this code does exactly the same thing that the callback does
today? Note that we can
even use that conditional to block the remaining spapr_del_lmbs code
from executing if the
LMBs weren't properly cleansed - something that today isn't done.
If removing this callback is too problematic or can somehow cause
problems that I am unable to
foresee, then the alternative would be to either deal with the scanning
inside the callback
(as it is being done in this patch) or migrate the nr_lmbs information
for late retrieval in
the callback. I am fine with any alternative, we just need to agree on
what makes more
sense.
Daniel
Regards,
Bharata.