[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] spapr: fix memory hot-unplugging
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH v2] spapr: fix memory hot-unplugging |
Date: |
Wed, 29 Mar 2017 11:35:32 +1100 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Tue, Mar 28, 2017 at 02:09:34PM +0200, Laurent Vivier wrote:
> If, once the kernel has booted, we try to remove a memory
> hotplugged while the kernel was not started, QEMU crashes on
> an assert:
>
> qemu-system-ppc64: hw/virtio/vhost.c:651:
> vhost_commit: Assertion `r >= 0' failed.
> ...
> #4 in vhost_commit
> #5 in memory_region_transaction_commit
> #6 in pc_dimm_memory_unplug
> #7 in spapr_memory_unplug
> #8 spapr_machine_device_unplug
> #9 in hotplug_handler_unplug
> #10 in spapr_lmb_release
> #11 in detach
> #12 in set_allocation_state
> #13 in rtas_set_indicator
> ...
>
> If we take a closer look to the guest kernel log, we can see when
> we try to unplug the memory:
>
> pseries-hotplug-mem: Attempting to hot-add 4 LMB(s)
>
> What happens:
>
> 1- The kernel has ignored the memory hotplug event because
> it was not started when it was generated.
>
> 2- When we hot-unplug the memory,
> QEMU starts to remove the memory,
> generates an hot-unplug event,
> and signals the kernel of the incoming new event
>
> 3- as the kernel is started, on the QEMU signal, it reads
> the event list, decodes the hotplug event and tries to
> finish the hotplugging.
>
> 4- QEMU receive the the hotplug notification while it
> is trying to hot-unplug the memory. This moves the memory
> DRC to an invalid state
>
> This patch prevents this by not allowing to set the allocation
> state to USABLE while the DRC is awaiting release.
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1432382
>
> Signed-off-by: Laurent Vivier <address@hidden>
Applied to ppc-for-2.9, thanks.
> ---
> v2: Add awaiting_allocation_skippable flag
> as suggested by Michael
>
> hw/ppc/spapr_drc.c | 20 +++++++++++++++++---
> include/hw/ppc/spapr_drc.h | 1 +
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 150f6bf..a1cdc87 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -135,6 +135,17 @@ static uint32_t set_allocation_state(sPAPRDRConnector
> *drc,
> if (!drc->dev) {
> return RTAS_OUT_NO_SUCH_INDICATOR;
> }
> + if (drc->awaiting_release && drc->awaiting_allocation) {
> + /* kernel is acknowledging a previous hotplug event
> + * while we are already removing it.
> + * it's safe to ignore awaiting_allocation here since we know the
> + * situation is predicated on the guest either already having
> done
> + * so (boot-time hotplug), or never being able to acquire in the
> + * first place (hotplug followed by immediate unplug).
> + */
> + drc->awaiting_allocation_skippable = true;
> + return RTAS_OUT_NO_SUCH_INDICATOR;
> + }
> }
>
> if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> @@ -436,9 +447,11 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d,
> }
>
> if (drc->awaiting_allocation) {
> - drc->awaiting_release = true;
> - trace_spapr_drc_awaiting_allocation(get_index(drc));
> - return;
> + if (!drc->awaiting_allocation_skippable) {
> + drc->awaiting_release = true;
> + trace_spapr_drc_awaiting_allocation(get_index(drc));
> + return;
> + }
> }
>
> drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
> @@ -448,6 +461,7 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d,
> }
>
> drc->awaiting_release = false;
> + drc->awaiting_allocation_skippable = false;
> g_free(drc->fdt);
> drc->fdt = NULL;
> drc->fdt_start_offset = 0;
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index fa531d5..5524247 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -154,6 +154,7 @@ typedef struct sPAPRDRConnector {
> bool awaiting_release;
> bool signalled;
> bool awaiting_allocation;
> + bool awaiting_allocation_skippable;
>
> /* device pointer, via link property */
> DeviceState *dev;
--
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