All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Gibson <david@gibson.dropbear.id.au>
To: peter.maydell@linaro.org, groug@kaod.org
Cc: Daniel Henrique Barboza <danielhb413@gmail.com>,
	qemu-ppc@nongnu.org, qemu-devel@nongnu.org,
	David Gibson <david@gibson.dropbear.id.au>
Subject: [PULL 12/20] spapr_drc.c: use DRC reconfiguration to cleanup DIMM unplug state
Date: Wed, 10 Mar 2021 15:09:54 +1100	[thread overview]
Message-ID: <20210310041002.333813-13-david@gibson.dropbear.id.au> (raw)
In-Reply-To: <20210310041002.333813-1-david@gibson.dropbear.id.au>

From: Daniel Henrique Barboza <danielhb413@gmail.com>

Handling errors in memory hotunplug in the pSeries machine is more
complex than any other device type, because there are all the
complications that other devices has, and more.

For instance, determining a timeout for a DIMM hotunplug must consider
if it's a Hash-MMU or a Radix-MMU guest, because Hash guests takes
longer to hotunplug DIMMs. The size of the DIMM is also a factor, given
that longer DIMMs naturally takes longer to be hotunplugged from the
kernel. And there's also the guest memory usage to be considered: if
there's a process that is consuming memory that would be lost by the
DIMM unplug, the kernel will postpone the unplug process until the
process finishes, and then initiate the regular hotunplug process. The
first two considerations are manageable, but the last one is a deal
breaker.

There is no sane way for the pSeries machine to determine the memory
load in the guest when attempting a DIMM hotunplug - and even if there
was a way, the guest can start using all the RAM in the middle of the
unplug process and invalidate our previous assumptions - and in result
we can't even begin to calculate a timeout for the operation. This means
that we can't implement a viable timeout mechanism for memory unplug in
pSeries.

Going back to why we would consider an unplug timeout, the reason is
that we can't know if the kernel is giving up the unplug. Turns out
that, sometimes, we can. Consider a failed memory hotunplug attempt
where the kernel will error out with the following message:

'pseries-hotplug-mem: Memory indexed-count-remove failed, adding any
removed LMBs'

This happens when there is a LMB that the kernel gave up in removing,
and the LMBs previously marked for removal are now being added back.
This happens in the pseries kernel in [1], dlpar_memory_remove_by_ic()
into dlpar_add_lmb(), and after that update_lmb_associativity_index().
In this function, the kernel is configuring the LMB DRC connector again.
Note that this is a valid usage in LOPAR, as stated in section
"ibm,configure-connector RTAS Call":

'A subsequent sequence of calls to ibm,configure-connector with the same
entry from the “ibm,drc-indexes” or “ibm,drc-info” property will restart
the configuration of devices which were not completely configured.'

We can use this kernel behavior in our favor. If a DRC connector
reconfiguration for a LMB that we marked as unplug pending happens, this
indicates that the kernel changed its mind about the unplug and is
reasserting that it will keep using all the LMBs of the DIMM. In this
case, it's safe to assume that the whole DIMM device unplug was
cancelled.

This patch hops into rtas_ibm_configure_connector() and, in the scenario
described above, clear the unplug state for the DIMM device. This will
not solve all the problems we still have with memory unplug, but it will
cover this case where the kernel reconfigures LMBs after a failed
unplug. We are a bit more resilient, without using an unreliable
timeout, and we didn't make the remaining error cases any worse.

[1] arch/powerpc/platforms/pseries/hotplug-memory.c

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Message-Id: <20210222194531.62717-6-danielhb413@gmail.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c         | 43 ++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/spapr_drc.c     | 10 ++++++++++
 include/hw/ppc/spapr.h |  2 ++
 3 files changed, 55 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index ecce8abf14..6eaddb12cb 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3575,6 +3575,49 @@ static SpaprDimmState *spapr_recover_pending_dimm_state(SpaprMachineState *ms,
     return spapr_pending_dimm_unplugs_add(ms, avail_lmbs, dimm);
 }
 
+void spapr_clear_pending_dimm_unplug_state(SpaprMachineState *spapr,
+                                           DeviceState *dev)
+{
+    SpaprDimmState *ds;
+    PCDIMMDevice *dimm;
+    SpaprDrc *drc;
+    uint32_t nr_lmbs;
+    uint64_t size, addr_start, addr;
+    int i;
+
+    if (!dev) {
+        return;
+    }
+
+    dimm = PC_DIMM(dev);
+    ds = spapr_pending_dimm_unplugs_find(spapr, dimm);
+
+    /*
+     * 'ds == NULL' would mean that the DIMM doesn't have a pending
+     * unplug state, but one of its DRC is marked as unplug_requested.
+     * This is bad and weird enough to g_assert() out.
+     */
+    g_assert(ds);
+
+    spapr_pending_dimm_unplugs_remove(spapr, ds);
+
+    size = memory_device_get_region_size(MEMORY_DEVICE(dimm), &error_abort);
+    nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE;
+
+    addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP,
+                                          &error_abort);
+
+    addr = addr_start;
+    for (i = 0; i < nr_lmbs; i++) {
+        drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
+                              addr / SPAPR_MEMORY_BLOCK_SIZE);
+        g_assert(drc);
+
+        drc->unplug_requested = false;
+        addr += SPAPR_MEMORY_BLOCK_SIZE;
+    }
+}
+
 /* Callback to be called during DRC release. */
 void spapr_lmb_release(DeviceState *dev)
 {
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index fd2e45640f..8c4997d795 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -1230,6 +1230,16 @@ static void rtas_ibm_configure_connector(PowerPCCPU *cpu,
 
     drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
 
+    /*
+     * This indicates that the kernel is reconfiguring a LMB due to
+     * a failed hotunplug. Clear the pending unplug state for the whole
+     * DIMM.
+     */
+    if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB &&
+        drc->unplug_requested) {
+        spapr_clear_pending_dimm_unplug_state(spapr, drc->dev);
+    }
+
     if (!drc->fdt) {
         void *fdt;
         int fdt_size;
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index ccbeeca1de..d6edeaaaff 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -847,6 +847,8 @@ int spapr_hpt_shift_for_ramsize(uint64_t ramsize);
 int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp);
 void spapr_clear_pending_events(SpaprMachineState *spapr);
 void spapr_clear_pending_hotplug_events(SpaprMachineState *spapr);
+void spapr_clear_pending_dimm_unplug_state(SpaprMachineState *spapr,
+                                           DeviceState *dev);
 int spapr_max_server_number(SpaprMachineState *spapr);
 void spapr_store_hpte(PowerPCCPU *cpu, hwaddr ptex,
                       uint64_t pte0, uint64_t pte1);
-- 
2.29.2



  parent reply	other threads:[~2021-03-10  4:20 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-10  4:09 [PULL 00/20] ppc-for-6.0 queue 20210310 David Gibson
2021-03-10  4:09 ` [PULL 01/20] hw/display/sm501: Remove dead code for non-32-bit RGB surfaces David Gibson
2021-03-10  4:09 ` [PULL 02/20] hw/display/sm501: Expand out macros in template header David Gibson
2021-03-10  4:09 ` [PULL 03/20] hw/display/sm501: Inline template header into C file David Gibson
2021-03-10  4:09 ` [PULL 04/20] spapr_drc.c: do not call spapr_drc_detach() in drc_isolate_logical() David Gibson
2021-03-10  4:09 ` [PULL 05/20] pseries: Update SLOF firmware image David Gibson
2021-03-10  4:09 ` [PULL 06/20] spapr_drc.c: use spapr_drc_release() in isolate_physical/set_unusable David Gibson
2021-03-10  4:09 ` [PULL 07/20] spapr: rename spapr_drc_detach() to spapr_drc_unplug_request() David Gibson
2021-03-10  4:09 ` [PULL 08/20] docs/system: Extend PPC section David Gibson
2021-03-10  4:09 ` [PULL 09/20] target/ppc: Fix bcdsub. emulation when result overflows David Gibson
2021-03-10  4:09 ` [PULL 10/20] spapr_drc.c: introduce unplug_timeout_timer David Gibson
2021-03-10  4:09 ` [PULL 11/20] spapr_drc.c: add hotunplug timeout for CPUs David Gibson
2021-03-10  4:09 ` David Gibson [this message]
2021-03-10  4:09 ` [PULL 13/20] hw/net: fsl_etsec: Fix build error when HEX_DUMP is on David Gibson
2021-03-10  4:09 ` [PULL 14/20] hw/ppc: e500: Add missing <ranges> in the eTSEC node David Gibson
2021-03-10  4:09 ` [PULL 15/20] spapr.c: add 'unplug already in progress' message for PHB unplug David Gibson
2021-03-10  4:09 ` [PULL 16/20] spapr_pci.c: add 'unplug already in progress' message for PCI unplug David Gibson
2021-03-10  4:09 ` [PULL 17/20] qemu_timer.c: add timer_deadline_ms() helper David Gibson
2021-03-10  4:10 ` [PULL 18/20] target/ppc: fix icount support on Book-e vms accessing SPRs David Gibson
2021-03-10  4:10 ` [PULL 19/20] spapr.c: remove duplicated assert in spapr_memory_unplug_request() David Gibson
2021-03-10  4:10 ` [PULL 20/20] spapr.c: send QAPI event when memory hotunplug fails David Gibson
2021-03-10  4:43 ` [PULL 00/20] ppc-for-6.0 queue 20210310 Bin Meng
2021-03-10  6:00   ` David Gibson
2021-03-11  1:26     ` Bin Meng
2021-03-10 14:09 ` Ivan Warren
2021-03-11  1:47   ` David Gibson
2021-03-11  3:22     ` Ivan Warren
2021-03-11  4:56       ` David Gibson
2021-03-11 13:31         ` Richard Henderson
2021-03-11 15:54           ` Greg Kurz
2021-03-11 18:02             ` Greg Kurz
2021-03-12 13:53 ` Peter Maydell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210310041002.333813-13-david@gibson.dropbear.id.au \
    --to=david@gibson.dropbear.id.au \
    --cc=danielhb413@gmail.com \
    --cc=groug@kaod.org \
    --cc=peter.maydell@linaro.org \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.