On 5/3/21 10:02 PM, David Gibson wrote:
On Fri, Apr 30, 2021 at 09:09:16AM -0300, Daniel Henrique Barboza wrote:
dlpar_memory_remove_by_ic() validates the amount of LMBs to be removed
by checking !DRCONF_MEM_RESERVED, and in the following loop before
dlpar_remove_lmb() a check for DRCONF_MEM_ASSIGNED is made before
removing it. This means that a LMB that is both !DRCONF_MEM_RESERVED and
!DRCONF_MEM_ASSIGNED will be counted as valid, but then not being
removed. The function will end up not removing all 'lmbs_to_remove'
LMBs while also not reporting any errors.
Comparing it to dlpar_memory_remove_by_count(), the validation is done
via lmb_is_removable(), which checks for DRCONF_MEM_ASSIGNED and fadump
constraints. No additional check is made afterwards, and
DRCONF_MEM_RESERVED is never checked before dlpar_remove_lmb(). The
function doesn't have the same 'check A for validation, then B for
removal' issue as remove_by_ic(), but it's not checking if the LMB is
reserved.
There is no reason for these functions to validate the same operation in
two different manners.
Actually, I think there is: remove_by_ic() is handling a request to
remove a specific range of LMBs. If any are reserved, they can't be
removed and so this needs to fail. But if they are !ASSIGNED, that
essentially means they're *already* removed (or never added), so
"removing" them is, correctly, a no-op.
I guess that makes sense. Although I am not aware of any situation, at least
thinking about how QEMU adds/removes LMBs, where some LMBs would be removed
'ad-hoc' in the middle of a LMB range that maps to a QEMU DIMM, I can't say
that this wouldn't never happen either. It is sensible to make remove_by_ic()
resilient to this situation.
I'll re-send this patch just with the remove_by_count() change.
Thanks,
Daniel
remove_by_count(), in contrast, is being asked to remove a fixed
number of LMBs from wherever they can be found, and for that it needs
to find LMBs that haven't already been removed.
Basically remove_by_ic() is an absolute request: "make this set of
LMBs be not-plugged", whereas remove_by_count() is a relative request
"make N less LMBs be plugged".
So I think remove_by_ic()s existing handling is correct. I'm less
sure if remove_by_count() ignoring RESERVED is correct - I couldn't
quickly find under what circumstances RESERVED gets set.
This patch addresses that by changing
lmb_is_removable() to also check for DRCONF_MEM_RESERVED to tell if a
lmb is removable, making dlpar_memory_remove_by_count() take the
reservation state into account when counting the LMBs.
lmb_is_removable() is then used in the validation step of
dlpar_memory_remove_by_ic(), which is already checking for both states
but in different stages, to avoid counting a LMB that is not assigned as
eligible for removal. We can then skip the check before
dlpar_remove_lmb() since we're validating all LMBs beforehand.
Signed-off-by: Daniel Henrique Barboza <danielhb...@gmail.com>
---
arch/powerpc/platforms/pseries/hotplug-memory.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c
b/arch/powerpc/platforms/pseries/hotplug-memory.c
index bb98574a84a2..4e6d162c3f1a 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -348,7 +348,8 @@ static int pseries_remove_mem_node(struct device_node *np)
static bool lmb_is_removable(struct drmem_lmb *lmb)
{
- if (!(lmb->flags & DRCONF_MEM_ASSIGNED))
+ if ((lmb->flags & DRCONF_MEM_RESERVED) ||
+ !(lmb->flags & DRCONF_MEM_ASSIGNED))
return false;
#ifdef CONFIG_FA_DUMP
@@ -523,7 +524,7 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove,
u32 drc_index)
/* Validate that there are enough LMBs to satisfy the request */
for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
- if (lmb->flags & DRCONF_MEM_RESERVED)
+ if (!lmb_is_removable(lmb))
break;
lmbs_available++;
@@ -533,9 +534,6 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove,
u32 drc_index)
return -EINVAL;
for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
- if (!(lmb->flags & DRCONF_MEM_ASSIGNED))
- continue;
-
rc = dlpar_remove_lmb(lmb);
if (rc)
break;
