Pingfan Liu [off-list ref] writes:

On Wed, Jul 22, 2020 at 12:57 PM Michael Ellerman [off-list ref] wrote:

quoted

Pingfan Liu [off-list ref] writes:

quoted

A bug is observed on pseries by taking the following steps on rhel:

                                                                ^
                                                                RHEL

I assume it happens on mainline too?

Yes, it does.

quoted

[...]

quoted

quoted

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 1a3ac3b..def8cb3f 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c

@@ -372,6 +372,7 @@ static int dlpar_remove_lmb(struct drmem_lmb *lmb)
      invalidate_lmb_associativity_index(lmb);
      lmb_clear_nid(lmb);
      lmb->flags &= ~DRCONF_MEM_ASSIGNED;
+     drmem_update_dt();

No error checking?

Hmm, here should be a more careful design. Please see the comment at the end.

quoted

quoted

      __remove_memory(nid, base_addr, block_sz);

@@ -607,6 +608,7 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)

      lmb_set_nid(lmb);
      lmb->flags |= DRCONF_MEM_ASSIGNED;
+     drmem_update_dt();

And here ..

quoted

      block_sz = memory_block_size_bytes();

@@ -625,6 +627,7 @@ static int dlpar_add_lmb(struct drmem_lmb *lmb)
              invalidate_lmb_associativity_index(lmb);
              lmb_clear_nid(lmb);
              lmb->flags &= ~DRCONF_MEM_ASSIGNED;
+             drmem_update_dt();

And here ..

quoted

              __remove_memory(nid, base_addr, block_sz);
      }

@@ -877,9 +880,6 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
              break;
      }

-     if (!rc)
-             rc = drmem_update_dt();
-
      unlock_device_hotplug();
      return rc;

Whereas previously we did check it.

drmem_update_dt() fails iff allocating memory fail.

That's true currently, but it might change in future.

And in the failed case, even the original code does not roll back the
effect of __add_memory()/__remove_memory().

Yeah hard to know what the desired behaviour is. If something fails we
at least need to print a message though, not silently swallow it.

And I plan to do the following in V4: if drmem_update_dt() fails in
dlpar_add_lmb(), then bails out immediately.

That sounds reasonable.

cheers