[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/5] spapr.c: check unplug_request flag in spapr_memory_unplu

From: Daniel Henrique Barboza
Subject: Re: [PATCH 2/5] spapr.c: check unplug_request flag in spapr_memory_unplug_request()
Date: Tue, 2 Mar 2021 07:39:17 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0

On 3/1/21 11:03 PM, David Gibson wrote:
On Fri, Feb 26, 2021 at 01:32:58PM -0300, Daniel Henrique Barboza wrote:
Now that we're asserting the first DRC LMB earlier, use it to query if
the DRC is already pending unplug and, in this case, issue the same
error we already do.

The previous check was introduced in commit 2a129767ebb1 and it works,
but it's easier to check the unplug_requested  flag instead of looking
for the existence of the sPAPRDIMMState. It's also compliant with what
is already done in other unplug_request functions for other devices.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

I'm having some trouble completely convincing myself this is right.

What about this situation:
      1. We initiate a DIMM unplug
        - unplug_request is set on all the LMBs
        - all the LMBs go on the pending_unplug list
      2. The guest encounters no problems, and starts issuing set
         indicator calls to mark the LMBs unusable, starting from the
        lowest address
      3. On drc_set_unusable() for the first LMB, we see that unplug is
         requested and call spapr_drc_release()
      4. spapr_drc_release() on the first LMB clears unplug_requested
      5. At this point, but before this is done on *all* of the DIMM's
         LMBs, the user attempts another unplug triggering the code

AFAICT this will now skip the error, since the first LMB is no longer
in unplug_requested state, but there *are* still pending unplugs for
some of the remaining LMBs, so the old code would have tripped the

Good point. Checking the existence of the sPAPRDIMMState struct at this
point is the same as checking for drc->unplug_requested for all the DRCs
of the DIMM.

I could check for drc->unplug_requested inside the loop where we instantiate
each DRC, but there is no gain in doing that instead of what we already have.

I'll drop this patch and change patch 1 to just remove the duplicated assert.



  hw/ppc/spapr.c | 8 +-------
  1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 74e046b522..149dc2113f 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3681,13 +3681,7 @@ static void spapr_memory_unplug_request(HotplugHandler 
                                  addr_start / SPAPR_MEMORY_BLOCK_SIZE);
- /*
-     * 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)) {
+    if (spapr_drc_unplug_requested(drc_start)) {
          error_setg(errp, "Memory unplug already in progress for device %s",

reply via email to

[Prev in Thread] Current Thread [Next in Thread]