[PATCH v7 0/4] powerpc/pseries: Implement indexed-count memory hotplug

Indexed-count memory hotplug allows addition and removal of contiguous
lmb blocks with a single command. This update will help with memory
DLPAR for Linux KVM guests and Qemu. The management of memory in Qemu
on a per-DIMM basis currently requires that DLPAR operations are done
in a series of DLPAR request to remove an entire range of LMBs. With
the indexed-count capability a single request can be made to remove or
add a contigious block of LMBs.

This patch set also reverts a previous patch to move pseries to use the
auto-online capabilities. This has proven problematic as the device
struct for a memory block does not get updated to indicate the memory
block is online when it is added, event though it is online.

Additionally there is a patch to correct an error when building with
the MEMORY_HOTREMOVE config option not set.

-Nathan
---

Updates from v6:
Patch 1/4: New
Patch 2/4: New
Patch 3/4: Re-based to latest kernel
Patch 4/4: Re-based to latest kernel
---

Nathan Fontenot (2):
      powerpc/pseries: dlpar_memory_readd_by_index() when MEMORY_HOTREMOVE not defined
      powerpc/pseries: Revert 'Auto-online hotplugged memory'

Sahil Mehta (2):
      powerpc/pseries: Implement indexed-count hotplug memory add
      powerpc/pseries: Implement indexed-count hotplug memory remove


 arch/powerpc/configs/pseries_defconfig          |    1 
 arch/powerpc/include/asm/rtas.h                 |    2 
 arch/powerpc/platforms/pseries/dlpar.c          |   38 +++
 arch/powerpc/platforms/pseries/hotplug-memory.c |  273 +++++++++++++++++++++--
 4 files changed, 286 insertions(+), 28 deletions(-)

Add a definition for dlpar_memory_readd_by_index() that returns
-EOPNOTSUPP to correct a build error when the config option
MEMORY_HOTREMOVE is not set.

Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/platforms/pseries/hotplug-memory.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 3381c20..f971ea0 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c

@@ -629,6 +629,11 @@ static int dlpar_memory_remove_by_index(u32 drc_index, struct property *prop)
 	return -EOPNOTSUPP;
 }
 
+static int dlpar_memory_readd_by_index(u32 drc_index, struct property *prop)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
 static int dlpar_add_lmb(struct of_drconf_cell *lmb)

Revert the patch patch to auto-online hotplugged memory, commit
id ec999072442a. Using the auto-online acpability does online added
memory but does not update the associated device struct to
indicate that the memory is online. The result of this is that
memoryXX/online file in sysfs still reports the memory as being offline.

Additionally, not updating the device struct to indicate the memory
is online will cause the pseries memory DLPAR code to fail when trying
to remove a LMB that was previously removed and added back. This happens
when validating that the LMB is removable.

Looking further into the auto-online memory, it seems that this
capability is meant more for something like a balloon driver rather than
general memory hotplug.

This patch reverts to the previous behavior of calling device_online()
to online the LMB when it is DLPAR added and moves the lmb_to_memblock()
routine out of CONFIG_MEMORY_HOTREMOVE now that we call it for add.

Signed-off-by: Nathan Fontenot <redacted>
---
 arch/powerpc/configs/pseries_defconfig          |    1 
 arch/powerpc/platforms/pseries/hotplug-memory.c |   52 ++++++++++++++++-------
 2 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/configs/pseries_defconfig b/arch/powerpc/configs/pseries_defconfig
index d99734f..28fc0c9 100644
--- a/arch/powerpc/configs/pseries_defconfig
+++ b/arch/powerpc/configs/pseries_defconfig

@@ -56,7 +56,6 @@ CONFIG_KEXEC_FILE=y
 CONFIG_IRQ_ALL_CPUS=y
 CONFIG_MEMORY_HOTPLUG=y
 CONFIG_MEMORY_HOTREMOVE=y
-CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE=y
 CONFIG_KSM=y
 CONFIG_TRANSPARENT_HUGEPAGE=y
 CONFIG_PPC_64K_PAGES=y

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index f971ea0..9609a72 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c

@@ -320,6 +320,19 @@ static int dlpar_remove_device_tree_lmb(struct of_drconf_cell *lmb)
 	return dlpar_update_device_tree_lmb(lmb);
 }
 
+static struct memory_block *lmb_to_memblock(struct of_drconf_cell *lmb)
+{
+	unsigned long section_nr;
+	struct mem_section *mem_sect;
+	struct memory_block *mem_block;
+
+	section_nr = pfn_to_section_nr(PFN_DOWN(lmb->base_addr));
+	mem_sect = __nr_to_section(section_nr);
+
+	mem_block = find_memory_block(mem_sect);
+	return mem_block;
+}
+
 #ifdef CONFIG_MEMORY_HOTREMOVE
 static int pseries_remove_memblock(unsigned long base, unsigned int memblock_size)
 {

@@ -407,19 +420,6 @@ static bool lmb_is_removable(struct of_drconf_cell *lmb)
 
 static int dlpar_add_lmb(struct of_drconf_cell *);
 
-static struct memory_block *lmb_to_memblock(struct of_drconf_cell *lmb)
-{
-	unsigned long section_nr;
-	struct mem_section *mem_sect;
-	struct memory_block *mem_block;
-
-	section_nr = pfn_to_section_nr(PFN_DOWN(lmb->base_addr));
-	mem_sect = __nr_to_section(section_nr);
-
-	mem_block = find_memory_block(mem_sect);
-	return mem_block;
-}
-
 static int dlpar_remove_lmb(struct of_drconf_cell *lmb)
 {
 	struct memory_block *mem_block;

@@ -636,6 +636,20 @@ static int dlpar_memory_readd_by_index(u32 drc_index, struct property *prop)
 
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
+static int dlpar_online_lmb(struct of_drconf_cell *lmb)
+{
+	struct memory_block *mem_block;
+	int rc;
+
+	mem_block = lmb_to_memblock(lmb);
+	if (!mem_block)
+		return -EINVAL;
+
+	rc = device_online(&mem_block->dev);
+	put_device(&mem_block->dev);
+	return rc;
+}
+
 static int dlpar_add_lmb(struct of_drconf_cell *lmb)
 {
 	unsigned long block_sz;

@@ -659,10 +673,18 @@ static int dlpar_add_lmb(struct of_drconf_cell *lmb)
 
 	/* Add the memory */
 	rc = add_memory(nid, lmb->base_addr, block_sz);
-	if (rc)
+	if (rc) {
 		dlpar_remove_device_tree_lmb(lmb);
-	else
+		return rc;
+	}
+
+	rc = dlpar_online_lmb(lmb);
+	if (rc) {
+		remove_memory(nid, lmb->base_addr, block_sz);
+		dlpar_remove_device_tree_lmb(lmb);
+	} else {
 		lmb->flags |= DRCONF_MEM_ASSIGNED;
+	}
 
 	return rc;
 }

From: Sahil Mehta <redacted>

Indexed-count add for memory hotplug guarantees that a contiguous block
of <count> lmbs beginning at a specified <drc index> will be assigned,
any LMBs in this range that are not already assigned will be DLPAR added.
Because of Qemu's per-DIMM memory management, the addition of a contiguous
block of memory currently requires a series of individual calls to add
each LMB in the block. Indexed-count add reduces this series of calls to
a single call for the entire block.

Signed-off-by: Sahil Mehta <redacted>
Signed-off-by: Nathan Fontenot <redacted>
---
Updates from v6: Re-based to latest kernel

 arch/powerpc/include/asm/rtas.h                 |    2 
 arch/powerpc/platforms/pseries/dlpar.c          |   38 +++++++
 arch/powerpc/platforms/pseries/hotplug-memory.c |  118 +++++++++++++++++++++--
 3 files changed, 146 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 076b892..ec9dd79 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h

@@ -307,6 +307,7 @@ struct pseries_hp_errorlog {
 	union {
 		__be32	drc_index;
 		__be32	drc_count;
+		struct { __be32 count, index; } ic;
 		char	drc_name[1];
 	} _drc_u;
 };

@@ -323,6 +324,7 @@ struct pseries_hp_errorlog {
 #define PSERIES_HP_ELOG_ID_DRC_NAME	1
 #define PSERIES_HP_ELOG_ID_DRC_INDEX	2
 #define PSERIES_HP_ELOG_ID_DRC_COUNT	3
+#define PSERIES_HP_ELOG_ID_DRC_IC	4
 
 struct pseries_errorlog *get_pseries_errorlog(struct rtas_error_log *log,
 					      uint16_t section_id);

diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index d3a81e7..193e052 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c

@@ -354,11 +354,17 @@ static int handle_dlpar_errorlog(struct pseries_hp_errorlog *hp_elog)
 	switch (hp_elog->id_type) {
 	case PSERIES_HP_ELOG_ID_DRC_COUNT:
 		hp_elog->_drc_u.drc_count =
-					be32_to_cpu(hp_elog->_drc_u.drc_count);
+				be32_to_cpu(hp_elog->_drc_u.drc_count);
 		break;
 	case PSERIES_HP_ELOG_ID_DRC_INDEX:
 		hp_elog->_drc_u.drc_index =
-					be32_to_cpu(hp_elog->_drc_u.drc_index);
+				be32_to_cpu(hp_elog->_drc_u.drc_index);
+		break;
+	case PSERIES_HP_ELOG_ID_DRC_IC:
+		hp_elog->_drc_u.ic.count =
+				be32_to_cpu(hp_elog->_drc_u.ic.count);
+		hp_elog->_drc_u.ic.index =
+				be32_to_cpu(hp_elog->_drc_u.ic.index);
 	}
 
 	switch (hp_elog->resource) {

@@ -467,7 +473,33 @@ static int dlpar_parse_id_type(char **cmd, struct pseries_hp_errorlog *hp_elog)
 	if (!arg)
 		return -EINVAL;
 
-	if (sysfs_streq(arg, "index")) {
+	if (sysfs_streq(arg, "indexed-count")) {
+		hp_elog->id_type = PSERIES_HP_ELOG_ID_DRC_IC;
+		arg = strsep(cmd, " ");
+		if (!arg) {
+			pr_err("No DRC count specified.\n");
+			return -EINVAL;
+		}
+
+		if (kstrtou32(arg, 0, &count)) {
+			pr_err("Invalid DRC count specified.\n");
+			return -EINVAL;
+		}
+
+		arg = strsep(cmd, " ");
+		if (!arg) {
+			pr_err("No DRC Index specified.\n");
+			return -EINVAL;
+		}
+
+		if (kstrtou32(arg, 0, &index)) {
+			pr_err("Invalid DRC Index specified.\n");
+			return -EINVAL;
+		}
+
+		hp_elog->_drc_u.ic.count = cpu_to_be32(count);
+		hp_elog->_drc_u.ic.index = cpu_to_be32(index);
+	} else if (sysfs_streq(arg, "index")) {
 		hp_elog->id_type = PSERIES_HP_ELOG_ID_DRC_INDEX;
 		arg = strsep(cmd, " ");
 		if (!arg) {

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 9609a72..0d1aa77 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c

@@ -803,6 +803,97 @@ static int dlpar_memory_add_by_index(u32 drc_index, struct property *prop)
 	return rc;
 }
 
+static int dlpar_memory_add_by_ic(u32 lmbs_to_add, u32 drc_index,
+				  struct property *prop)
+{
+	struct of_drconf_cell *lmbs;
+	u32 num_lmbs, *p;
+	int i, rc, start_lmb_found;
+	int lmbs_available = 0, start_index = 0, end_index;
+
+	pr_info("Attempting to hot-add %u LMB(s) at index %x\n",
+		lmbs_to_add, drc_index);
+
+	if (lmbs_to_add == 0)
+		return -EINVAL;
+
+	p = prop->value;
+	num_lmbs = *p++;
+	lmbs = (struct of_drconf_cell *)p;
+	start_lmb_found = 0;
+
+	/* Navigate to drc_index */
+	while (start_index < num_lmbs) {
+		if (lmbs[start_index].drc_index == drc_index) {
+			start_lmb_found = 1;
+			break;
+		}
+
+		start_index++;
+	}
+
+	if (!start_lmb_found)
+		return -EINVAL;
+
+	end_index = start_index + lmbs_to_add;
+
+	/* Validate that the LMBs in this range are not reserved */
+	for (i = start_index; i < end_index; i++) {
+		if (lmbs[i].flags & DRCONF_MEM_RESERVED)
+			break;
+
+		lmbs_available++;
+	}
+
+	if (lmbs_available < lmbs_to_add)
+		return -EINVAL;
+
+	for (i = start_index; i < end_index; i++) {
+		if (lmbs[i].flags & DRCONF_MEM_ASSIGNED)
+			continue;
+
+		rc = dlpar_acquire_drc(lmbs[i].drc_index);
+		if (rc)
+			break;
+
+		rc = dlpar_add_lmb(&lmbs[i]);
+		if (rc) {
+			dlpar_release_drc(lmbs[i].drc_index);
+			break;
+		}
+
+		lmbs[i].reserved = 1;
+	}
+
+	if (rc) {
+		pr_err("Memory indexed-count-add failed, removing any added LMBs\n");
+
+		for (i = start_index; i < end_index; i++) {
+			if (!lmbs[i].reserved)
+				continue;
+
+			rc = dlpar_remove_lmb(&lmbs[i]);
+			if (rc)
+				pr_err("Failed to remove LMB, drc index %x\n",
+				       be32_to_cpu(lmbs[i].drc_index));
+			else
+				dlpar_release_drc(lmbs[i].drc_index);
+		}
+		rc = -EINVAL;
+	} else {
+		for (i = start_index; i < end_index; i++) {
+			if (!lmbs[i].reserved)
+				continue;
+
+			pr_info("Memory at %llx (drc index %x) was hot-added\n",
+				lmbs[i].base_addr, lmbs[i].drc_index);
+			lmbs[i].reserved = 0;
+		}
+	}
+
+	return rc;
+}
+
 int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
 {
 	struct device_node *dn;

@@ -810,9 +901,6 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
 	u32 count, drc_index;
 	int rc;
 
-	count = hp_elog->_drc_u.drc_count;
-	drc_index = hp_elog->_drc_u.drc_index;
-
 	lock_device_hotplug();
 
 	dn = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");

@@ -829,20 +917,32 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
 
 	switch (hp_elog->action) {
 	case PSERIES_HP_ELOG_ACTION_ADD:
-		if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT)
+		if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) {
+			count = hp_elog->_drc_u.drc_count;
 			rc = dlpar_memory_add_by_count(count, prop);
-		else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX)
+		} else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) {
+			drc_index = hp_elog->_drc_u.drc_index;
 			rc = dlpar_memory_add_by_index(drc_index, prop);
-		else
+		} else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) {
+			count = hp_elog->_drc_u.ic.count;
+			drc_index = hp_elog->_drc_u.ic.index;
+			rc = dlpar_memory_add_by_ic(count, drc_index, prop);
+		} else {
 			rc = -EINVAL;
+		}
+
 		break;
 	case PSERIES_HP_ELOG_ACTION_REMOVE:
-		if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT)
+		if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_COUNT) {
+			count = hp_elog->_drc_u.drc_count;
 			rc = dlpar_memory_remove_by_count(count, prop);
-		else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX)
+		} else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) {
+			drc_index = hp_elog->_drc_u.drc_index;
 			rc = dlpar_memory_remove_by_index(drc_index, prop);
-		else
+		} else {
 			rc = -EINVAL;
+		}
+
 		break;
 	case PSERIES_HP_ELOG_ACTION_READD:
 		rc = dlpar_memory_readd_by_index(drc_index, prop);

From: Sahil Mehta <redacted>

Indexed-count remove for memory hotplug guarantees that a contiguous block
of <count> lmbs beginning at a specified <index> will be unassigned (NOT
that <count> lmbs will be removed). Because of Qemu's per-DIMM memory
management, the removal of a contiguous block of memory currently
requires a series of individual calls. Indexed-count remove reduces
this series into a single call.

Signed-off-by: Sahil Mehta <redacted>
Signed-off-by: Nathan Fontenot <redacted>
---
Updates for v7: Re-based to latest kernel

 arch/powerpc/platforms/pseries/hotplug-memory.c |   98 +++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 0d1aa77..efef10a 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c

@@ -601,6 +601,94 @@ static int dlpar_memory_readd_by_index(u32 drc_index, struct property *prop)
 
 	return rc;
 }
+
+static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index,
+				     struct property *prop)
+{
+	struct of_drconf_cell *lmbs;
+	u32 num_lmbs, *p;
+	int i, rc, start_lmb_found;
+	int lmbs_available = 0, start_index = 0, end_index;
+
+	pr_info("Attempting to hot-remove %u LMB(s) at %x\n",
+		lmbs_to_remove, drc_index);
+
+	if (lmbs_to_remove == 0)
+		return -EINVAL;
+
+	p = prop->value;
+	num_lmbs = *p++;
+	lmbs = (struct of_drconf_cell *)p;
+	start_lmb_found = 0;
+
+	/* Navigate to drc_index */
+	while (start_index < num_lmbs) {
+		if (lmbs[start_index].drc_index == drc_index) {
+			start_lmb_found = 1;
+			break;
+		}
+
+		start_index++;
+	}
+
+	if (!start_lmb_found)
+		return -EINVAL;
+
+	end_index = start_index + lmbs_to_remove;
+
+	/* Validate that there are enough LMBs to satisfy the request */
+	for (i = start_index; i < end_index; i++) {
+		if (lmbs[i].flags & DRCONF_MEM_RESERVED)
+			break;
+
+		lmbs_available++;
+	}
+
+	if (lmbs_available < lmbs_to_remove)
+		return -EINVAL;
+
+	for (i = start_index; i < end_index; i++) {
+		if (!(lmbs[i].flags & DRCONF_MEM_ASSIGNED))
+			continue;
+
+		rc = dlpar_remove_lmb(&lmbs[i]);
+		if (rc)
+			break;
+
+		lmbs[i].reserved = 1;
+	}
+
+	if (rc) {
+		pr_err("Memory indexed-count-remove failed, adding any removed LMBs\n");
+
+		for (i = start_index; i < end_index; i++) {
+			if (!lmbs[i].reserved)
+				continue;
+
+			rc = dlpar_add_lmb(&lmbs[i]);
+			if (rc)
+				pr_err("Failed to add LMB, drc index %x\n",
+				       be32_to_cpu(lmbs[i].drc_index));
+
+			lmbs[i].reserved = 0;
+		}
+		rc = -EINVAL;
+	} else {
+		for (i = start_index; i < end_index; i++) {
+			if (!lmbs[i].reserved)
+				continue;
+
+			dlpar_release_drc(lmbs[i].drc_index);
+			pr_info("Memory at %llx (drc index %x) was hot-removed\n",
+				lmbs[i].base_addr, lmbs[i].drc_index);
+
+			lmbs[i].reserved = 0;
+		}
+	}
+
+	return rc;
+}
+
 #else
 static inline int pseries_remove_memblock(unsigned long base,
 					  unsigned int memblock_size)

@@ -634,6 +722,12 @@ static int dlpar_memory_readd_by_index(u32 drc_index, struct property *prop)
 	return -EOPNOTSUPP;
 }
 
+static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index,
+				     struct property *prop)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 
 static int dlpar_online_lmb(struct of_drconf_cell *lmb)

@@ -939,6 +1033,10 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
 		} else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_INDEX) {
 			drc_index = hp_elog->_drc_u.drc_index;
 			rc = dlpar_memory_remove_by_index(drc_index, prop);
+		} else if (hp_elog->id_type == PSERIES_HP_ELOG_ID_DRC_IC) {
+			count = hp_elog->_drc_u.ic.count;
+			drc_index = hp_elog->_drc_u.ic.index;
+			rc = dlpar_memory_remove_by_ic(count, drc_index, prop);
 		} else {
 			rc = -EINVAL;
 		}

On Wed, Feb 15, 2017 at 01:45:22PM -0500, Nathan Fontenot wrote:

Add a definition for dlpar_memory_readd_by_index() that returns
-EOPNOTSUPP to correct a build error when the config option
MEMORY_HOTREMOVE is not set.

Signed-off-by: Nathan Fontenot <redacted>

Reviewed-by: David Gibson <redacted>

-- 
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

Nathan Fontenot [off-list ref] writes:

Revert the patch patch to auto-online hotplugged memory, commit
id ec999072442a. Using the auto-online acpability does online added
memory but does not update the associated device struct to
indicate that the memory is online. The result of this is that
memoryXX/online file in sysfs still reports the memory as being offline.

Isn't that just a bug in the auto-online code?

If I'm reading it right it's calling online_memory_block(). If that
doesn't cause the memory_block to be online that would puzzle me.


Also commit ec999072442a went into v4.8, so is memory hotplug broken
since then? If so we need to backport this or whatever fix we come up.

cheers

Nathan Fontenot [off-list ref] writes:

pc/platforms/pseries/hotplug-memory.c

        ^^^

This fails to build for me because of the above removal, ...

g)

=20=20

...

 		break;
 	case PSERIES_HP_ELOG_ACTION_READD:
 		rc =3D dlpar_memory_readd_by_index(drc_index, prop);

						 ^^^
And the existing usage here.


Which leads to:


  arch/powerpc/platforms/pseries/hotplug-memory.c: In function =E2=80=98dlp=
ar_memory=E2=80=99:
  arch/powerpc/platforms/pseries/hotplug-memory.c:581:6: error: =E2=80=98dr=
c_index=E2=80=99 may be used uninitialized in this function [-Werror=3Dmayb=
e-uninitialized]
  if (lmbs[i].drc_index =3D=3D drc_index) {
  ^

Presumably you're using an old compiler that didn't pick that up? If so
please update to a more modern compiler.


I did the obvious fix, hopefully it is correct:

diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc=

/platforms/pseries/hotplug-memory.c
index bc73546587e7..e28abfa013e5 100644

--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c

@@ -1019,6 +1019,7 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)

=20
 		break;
 	case PSERIES_HP_ELOG_ACTION_READD:
+		drc_index =3D hp_elog->_drc_u.drc_index;
 		rc =3D dlpar_memory_readd_by_index(drc_index, prop);
 		break;
 	default:

cheers

On Wed, 2017-02-15 at 18:45:56 UTC, Nathan Fontenot wrote:

From: Sahil Mehta <redacted>

Indexed-count add for memory hotplug guarantees that a contiguous block
of <count> lmbs beginning at a specified <drc index> will be assigned,
any LMBs in this range that are not already assigned will be DLPAR added.
Because of Qemu's per-DIMM memory management, the addition of a contiguous
block of memory currently requires a series of individual calls to add
each LMB in the block. Indexed-count add reduces this series of calls to
a single call for the entire block.

Signed-off-by: Sahil Mehta <redacted>
Signed-off-by: Nathan Fontenot <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/333f7b76865bec24c66710cf352f89

cheers

On Wed, 2017-02-15 at 18:46:18 UTC, Nathan Fontenot wrote:

From: Sahil Mehta <redacted>

Indexed-count remove for memory hotplug guarantees that a contiguous block
of <count> lmbs beginning at a specified <index> will be unassigned (NOT
that <count> lmbs will be removed). Because of Qemu's per-DIMM memory
management, the removal of a contiguous block of memory currently
requires a series of individual calls. Indexed-count remove reduces
this series into a single call.

Signed-off-by: Sahil Mehta <redacted>
Signed-off-by: Nathan Fontenot <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/753843471cbbaeca25a5cab51981ee

cheers

On 02/15/2017 10:34 PM, Michael Ellerman wrote:

Nathan Fontenot [off-list ref] writes:

quoted

Revert the patch patch to auto-online hotplugged memory, commit
id ec999072442a. Using the auto-online acpability does online added
memory but does not update the associated device struct to
indicate that the memory is online. The result of this is that
memoryXX/online file in sysfs still reports the memory as being offline.

Isn't that just a bug in the auto-online code?

After digging through the code some more and reading some of the email
chain when the auto-online feature was submitted I can't decide if this
is a bug or if this is by design. The fact that they only other users
of this appear to be balloon drivers (hv and xen) makes me think this
may be by design.

Changing the auto-online capability to call device_offline() instead
would appear to also require changes to the hv and xen balloon
drivers for the new behavior.

If I'm reading it right it's calling online_memory_block(). If that
doesn't cause the memory_block to be online that would puzzle me.

The memory is online and usuable when the dlpar operation completes. I
was mistaken in my original note though, the state file in sysfs does report
the memory as being online. The underlying issue is that the device struct
does not get updated (dev->offline) when using the auto-online capability.
The result is that trying to remove a LMB a second time fails when we call
device_offline() which checks the dev->offline flag and returns failure.

I think reverting the patch to use the auto-online capability may be the
way to go. This would restore the code so that we call device_online and
device_offline for add and remove respectively, and not rely on what the 
auto-online code is doing.

Thoughts?

Also commit ec999072442a went into v4.8, so is memory hotplug broken
since then? If so we need to backport this or whatever fix we come up.

Yes, we need to backport whatever fix we do.

-Nathan

cheers

Nathan Fontenot [off-list ref] writes:

On 02/15/2017 10:34 PM, Michael Ellerman wrote:

quoted

Nathan Fontenot [off-list ref] writes:

quoted

Revert the patch patch to auto-online hotplugged memory, commit
id ec999072442a. Using the auto-online acpability does online added
memory but does not update the associated device struct to
indicate that the memory is online. The result of this is that
memoryXX/online file in sysfs still reports the memory as being offline.

Isn't that just a bug in the auto-online code?

After digging through the code some more and reading some of the email
chain when the auto-online feature was submitted I can't decide if this
is a bug or if this is by design. The fact that they only other users
of this appear to be balloon drivers (hv and xen) makes me think this
may be by design.

Have we asked the original authors? I don't see them on Cc?

Changing the auto-online capability to call device_offline() instead
would appear to also require changes to the hv and xen balloon
drivers for the new behavior.

OK, if that's the case then that's going to make life tricky.
 

quoted

If I'm reading it right it's calling online_memory_block(). If that
doesn't cause the memory_block to be online that would puzzle me.

The memory is online and usuable when the dlpar operation completes. I
was mistaken in my original note though, the state file in sysfs does report
the memory as being online. The underlying issue is that the device struct
does not get updated (dev->offline) when using the auto-online capability.
The result is that trying to remove a LMB a second time fails when we call
device_offline() which checks the dev->offline flag and returns failure.

That still sounds like a bug to me. We asked the core to "auto online"
the added memory, but the dev is still offline? But maybe there's some
subtlety.

I think reverting the patch to use the auto-online capability may be the
way to go. This would restore the code so that we call device_online and
device_offline for add and remove respectively, and not rely on what the 
auto-online code is doing.

Thoughts?

It's not great, but given we need to backport it to v4.8, yeah I think
we'll have to go with a revert.

But we should also pursue fixing the auto online logic.

quoted

Also commit ec999072442a went into v4.8, so is memory hotplug broken
since then? If so we need to backport this or whatever fix we come up.

Yes, we need to backport whatever fix we do.

Right. I'll queue it up.

cheers

On 02/20/2017 07:02 PM, Michael Ellerman wrote:

Nathan Fontenot [off-list ref] writes:

quoted

On 02/15/2017 10:34 PM, Michael Ellerman wrote:

quoted

Nathan Fontenot [off-list ref] writes:

quoted

Revert the patch patch to auto-online hotplugged memory, commit
id ec999072442a. Using the auto-online acpability does online added
memory but does not update the associated device struct to
indicate that the memory is online. The result of this is that
memoryXX/online file in sysfs still reports the memory as being offline.

Isn't that just a bug in the auto-online code?

After digging through the code some more and reading some of the email
chain when the auto-online feature was submitted I can't decide if this
is a bug or if this is by design. The fact that they only other users
of this appear to be balloon drivers (hv and xen) makes me think this
may be by design.

Have we asked the original authors? I don't see them on Cc?

Not yet. I'm working on a patch to use device_online() for doing
auto online of memory. I will send this out as an RFC with an explanation
of what I'm seeing and ask why it was done the way it was.

quoted

Changing the auto-online capability to call device_offline() instead
would appear to also require changes to the hv and xen balloon
drivers for the new behavior.

OK, if that's the case then that's going to make life tricky.

Yep. I'll ask about this with the RFC I send out.

-Nathan

quoted

quoted

If I'm reading it right it's calling online_memory_block(). If that
doesn't cause the memory_block to be online that would puzzle me.

The memory is online and usuable when the dlpar operation completes. I
was mistaken in my original note though, the state file in sysfs does report
the memory as being online. The underlying issue is that the device struct
does not get updated (dev->offline) when using the auto-online capability.
The result is that trying to remove a LMB a second time fails when we call
device_offline() which checks the dev->offline flag and returns failure.

That still sounds like a bug to me. We asked the core to "auto online"
the added memory, but the dev is still offline? But maybe there's some
subtlety.

quoted

I think reverting the patch to use the auto-online capability may be the
way to go. This would restore the code so that we call device_online and
device_offline for add and remove respectively, and not rely on what the 
auto-online code is doing.

Thoughts?

It's not great, but given we need to backport it to v4.8, yeah I think
we'll have to go with a revert.

But we should also pursue fixing the auto online logic.

quoted

quoted

Also commit ec999072442a went into v4.8, so is memory hotplug broken
since then? If so we need to backport this or whatever fix we come up.

Yes, we need to backport whatever fix we do.

Right. I'll queue it up.

cheers

On Wed, 2017-02-15 at 18:45:30 UTC, Nathan Fontenot wrote:

Revert the patch patch to auto-online hotplugged memory, commit
id ec999072442a. Using the auto-online acpability does online added
memory but does not update the associated device struct to
indicate that the memory is online. The result of this is that
memoryXX/online file in sysfs still reports the memory as being offline.

Additionally, not updating the device struct to indicate the memory
is online will cause the pseries memory DLPAR code to fail when trying
to remove a LMB that was previously removed and added back. This happens
when validating that the LMB is removable.

Looking further into the auto-online memory, it seems that this
capability is meant more for something like a balloon driver rather than
general memory hotplug.

This patch reverts to the previous behavior of calling device_online()
to online the LMB when it is DLPAR added and moves the lmb_to_memblock()
routine out of CONFIG_MEMORY_HOTREMOVE now that we call it for add.

Signed-off-by: Nathan Fontenot <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/943db62c316c578f8e2cc6fb81a5f6

cheers