Re: [Qemu-devel] [PATCH v2 2/2] Revert "spapr: fix memory hot-unplugging"

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Jun 08, 2017 at 07:27:43PM +0200, Laurent Vivier wrote:
> This reverts commit fe6824d12642b005c69123ecf8631f9b13553f8b.
> 
> Conflicts hw/ppc/spapr_drc.c, because get_index() has been renamed
> spapr_get_index().
> 
> This didn't fix the problem. Once the hotplug has been started
> some memory is allocated and some structures are allocated.
> We don't free it when we ignore the unplug, and we can't because
> they can be in use by the kernel.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Heh.  I've just been looking at awaiting_allocation_skippable and
trying - but failing - to work out what it accomplishes.  Applied to
ppc-for-2.10.

> ---
>  hw/ppc/spapr_drc.c         | 20 +++-----------------
>  include/hw/ppc/spapr_drc.h |  1 -
>  2 files changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 39e7f30..7605977 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -140,17 +140,6 @@ 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 (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> @@ -401,11 +390,9 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
>      }
>  
>      if (drc->awaiting_allocation) {
> -        if (!drc->awaiting_allocation_skippable) {
> -            drc->awaiting_release = true;
> -            trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
> -            return;
> -        }
> +        drc->awaiting_release = true;
> +        trace_spapr_drc_awaiting_allocation(spapr_drc_index(drc));
> +        return;
>      }
>  
>      drc->indicator_state = SPAPR_DR_INDICATOR_STATE_INACTIVE;
> @@ -428,7 +415,6 @@ static void detach(sPAPRDRConnector *drc, DeviceState *d, Error **errp)
>      }
>  
>      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 c88e1be..84b58f0 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -200,7 +200,6 @@ 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

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