[Qemu-devel] [PATCH v2 0/1] Avoid LMB unplug if a previous unplug isn't

[Qemu-devel] [PATCH v2 0/1] Avoid LMB unplug if a previous unplug isn't

[Daniel Henrique Barboza]

v2:
- changed the approach: abort the LMB unplug if there is already
an unplug operation ongoing for the same DIMM.

v1 can be found at
http://lists.nongnu.org/archive/html/qemu-ppc/2017-10/msg00084.html

This fixes https://bugs.launchpad.net/qemu/+bug/1718118 .

Daniel Henrique Barboza (1):
  hw/ppc/spapr.c: abort unplug_request if previous unplug isn't done

 hw/ppc/spapr.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH v2 1/1] hw/ppc/spapr.c: abort unplug_request if previous unplug isn't done

[Daniel Henrique Barboza]

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 <danielhb@linux.vnet.ibm.com>
---
 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;
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2 1/1] hw/ppc/spapr.c: abort unplug_request if previous unplug isn't done

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4061 bytes --]


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 <danielhb@linux.vnet.ibm.com>

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]