[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH v2 1/1] hw/ppc/spapr.c: abort unplug_request if pr
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH v2 1/1] hw/ppc/spapr.c: abort unplug_request if previous unplug isn't done |
Date: |
Tue, 10 Oct 2017 12:36:44 +1100 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Mon, Oct 09, 2017 at 06:11:36PM -0300, Daniel Henrique Barboza wrote:
> LMB removal is completed only when the spapr_lmb_release callback
> is called after all DRCs of the dimm are detached. During this
> time, it is possible that a unplug request for the same dimm
> arrives, trying to detach DRCs that were detached by the guest
> in the first unplug_request.
>
> BQL doesn't help in this case - the lock will prevent any concurrent
> removal from happening until the end of spapr_memory_unplug_request
> only. What happens is that the second unplug_request ends up calling
> spapr_drc_detach in a DRC that were detached already, causing an
> assert error in spapr_drc_detach (e.g
> https://bugs.launchpad.net/qemu/+bug/1718118).
>
> spapr_lmb_release uses a structure called sPAPRDIMMState, stored in the
> spapr->pending_dimm_unplugs QTAIL, to track how many LMB DRCs are left
> to be detached by the guest. When there are no more DRCs left, this
> structure is deleted and the pc-dimm unplug handler is called to
> finish the process.
>
> This patch reuses the sPAPRDIMMState to allow unplug_request to know
> if there is an ongoing unplug process for a given dimm, aborting the
> unplug request in this case, by doing the following changes:
>
> - in spapr_lmb_release callback, move the dimm state removal to the
> end, after pc-dimm unplug handler. With this change we can check for
> the existence of the dimm state to see if the unplug process is
> done.
>
> - use spapr_pending_dimm_unplugs_find in spapr_memory_unplug_request
> to check if the dimm state exists. If positive, there is an unplug
> operation already in progress for this dimm, meaning that we should
> abort it and warn the user about it.
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1718118
> Signed-off-by: Daniel Henrique Barboza <address@hidden>
Thanks for the updated description. I've applied to ppc-for-2.11.
I'm not totally certain the first change is necessary - because each
of the pieces here should be called with the BQL, I think releasing
the DIMMState should be atomic with the rest of the stuff in
spapr_lmb_release(). But it makes it more obvious what's going on
here, so that's fine anyway.
> ---
> hw/ppc/spapr.c | 16 ++++++++++++++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ff87f155d5..7f9274d57d 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3054,14 +3054,13 @@ void spapr_lmb_release(DeviceState *dev)
> return;
> }
>
> - spapr_pending_dimm_unplugs_remove(spapr, ds);
> -
> /*
> * Now that all the LMBs have been removed by the guest, call the
> * pc-dimm unplug handler to cleanup up the pc-dimm device.
> */
> pc_dimm_memory_unplug(dev, &spapr->hotplug_memory, mr);
> object_unparent(OBJECT(dev));
> + spapr_pending_dimm_unplugs_remove(spapr, ds);
> }
>
> static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev,
> @@ -3090,6 +3089,19 @@ static void spapr_memory_unplug_request(HotplugHandler
> *hotplug_dev,
> goto out;
> }
>
> + /*
> + * An existing pending dimm state for this DIMM means that there is an
> + * unplug operation in progress, waiting for the spapr_lmb_release
> + * callback to complete the job (BQL can't cover that far). In this case,
> + * bail out to avoid detaching DRCs that were already released.
> + */
> + if (spapr_pending_dimm_unplugs_find(spapr, dimm)) {
> + error_setg(&local_err,
> + "Memory unplug already in progress for device %s",
> + dev->id);
> + goto out;
> + }
> +
> spapr_pending_dimm_unplugs_add(spapr, nr_lmbs, dimm);
>
> addr = addr_start;
--
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