diff mbox

[v6,4/4] of/fdt: mark hotpluggable memory

Message ID 1478562276-25539-5-git-send-email-arbab@linux.vnet.ibm.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Reza Arbab Nov. 7, 2016, 11:44 p.m. UTC
When movable nodes are enabled, any node containing only hotpluggable
memory is made movable at boot time.

On x86, hotpluggable memory is discovered by parsing the ACPI SRAT,
making corresponding calls to memblock_mark_hotplug().

If we introduce a dt property to describe memory as hotpluggable,
configs supporting early fdt may then also do this marking and use
movable nodes.

Signed-off-by: Reza Arbab <arbab@linux.vnet.ibm.com>
---
 drivers/of/fdt.c | 6 ++++++
 mm/Kconfig       | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

kernel test robot Nov. 8, 2016, 1:59 a.m. UTC | #1
Hi Reza,

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.9-rc4 next-20161028]
[cannot apply to mmotm/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Reza-Arbab/enable-movable-nodes-on-non-x86-configs/20161108-081711
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: tile-allmodconfig (attached as .config)
compiler: tilegx-linux-gcc (GCC) 4.6.2
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=tile 

All errors (new ones prefixed by >>):

   drivers/of/fdt.c: In function 'early_init_dt_scan_memory':
>> drivers/of/fdt.c:1064:3: error: implicit declaration of function 'memblock_mark_hotplug'
   cc1: some warnings being treated as errors

vim +/memblock_mark_hotplug +1064 drivers/of/fdt.c

  1058				continue;
  1059			pr_debug(" - %llx ,  %llx\n", (unsigned long long)base,
  1060			    (unsigned long long)size);
  1061	
  1062			early_init_dt_add_memory_arch(base, size);
  1063	
> 1064			if (hotpluggable && memblock_mark_hotplug(base, size))
  1065				pr_warn("failed to mark hotplug range 0x%llx - 0x%llx\n",
  1066					base, base + size);
  1067		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Reza Arbab Nov. 8, 2016, 7:59 p.m. UTC | #2
On Tue, Nov 08, 2016 at 09:59:26AM +0800, kbuild test robot wrote:
>All errors (new ones prefixed by >>):
>
>   drivers/of/fdt.c: In function 'early_init_dt_scan_memory':
>>> drivers/of/fdt.c:1064:3: error: implicit declaration of function 'memblock_mark_hotplug'
>   cc1: some warnings being treated as errors
>
>vim +/memblock_mark_hotplug +1064 drivers/of/fdt.c
>
>  1058				continue;
>  1059			pr_debug(" - %llx ,  %llx\n", (unsigned long long)base,
>  1060			    (unsigned long long)size);
>  1061	
>  1062			early_init_dt_add_memory_arch(base, size);
>  1063	
>> 1064			if (hotpluggable && memblock_mark_hotplug(base, size))
>  1065				pr_warn("failed to mark hotplug range 0x%llx - 0x%llx\n",
>  1066					base, base + size);
>  1067		}

Ah, I need to adjust for !CONFIG_HAVE_MEMBLOCK. Will correct in v7.
Rob Herring Nov. 9, 2016, 6:12 p.m. UTC | #3
On Mon, Nov 7, 2016 at 5:44 PM, Reza Arbab <arbab@linux.vnet.ibm.com> wrote:
> When movable nodes are enabled, any node containing only hotpluggable
> memory is made movable at boot time.
>
> On x86, hotpluggable memory is discovered by parsing the ACPI SRAT,
> making corresponding calls to memblock_mark_hotplug().
>
> If we introduce a dt property to describe memory as hotpluggable,
> configs supporting early fdt may then also do this marking and use
> movable nodes.
>
> Signed-off-by: Reza Arbab <arbab@linux.vnet.ibm.com>
> ---
>  drivers/of/fdt.c | 6 ++++++
>  mm/Kconfig       | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index c89d5d2..2cf1d66 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -1015,6 +1015,7 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
>         const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
>         const __be32 *reg, *endp;
>         int l;
> +       bool hotpluggable;
>
>         /* We are scanning "memory" nodes only */
>         if (type == NULL) {
> @@ -1034,6 +1035,7 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
>                 return 0;
>
>         endp = reg + (l / sizeof(__be32));
> +       hotpluggable = of_get_flat_dt_prop(node, "linux,hotpluggable", NULL);

Memory being hotpluggable doesn't seem like a linux property to me.
I'd drop the linux prefix. Also, this needs to be documented.

Rob
Reza Arbab Nov. 9, 2016, 8:15 p.m. UTC | #4
On Wed, Nov 09, 2016 at 12:12:55PM -0600, Rob Herring wrote:
>On Mon, Nov 7, 2016 at 5:44 PM, Reza Arbab <arbab@linux.vnet.ibm.com> wrote:
>> +       hotpluggable = of_get_flat_dt_prop(node, "linux,hotpluggable", NULL);
>
>Memory being hotpluggable doesn't seem like a linux property to me.
>I'd drop the linux prefix. Also, this needs to be documented.

Sure, that makes sense. I'll do both in v7.
Balbir Singh Nov. 10, 2016, 12:56 a.m. UTC | #5
On 08/11/16 10:44, Reza Arbab wrote:
> When movable nodes are enabled, any node containing only hotpluggable
> memory is made movable at boot time.
> 
> On x86, hotpluggable memory is discovered by parsing the ACPI SRAT,
> making corresponding calls to memblock_mark_hotplug().
> 
> If we introduce a dt property to describe memory as hotpluggable,
> configs supporting early fdt may then also do this marking and use
> movable nodes.

This looks much better, like the other comments pointed out

We need documentation around the changes. One quick question

Have you tested this across all combinations of skiboot/kexec/SLOF boots?

Balbir Singh.
Reza Arbab Nov. 10, 2016, 8:52 p.m. UTC | #6
On Thu, Nov 10, 2016 at 11:56:02AM +1100, Balbir Singh wrote:
>Have you tested this across all combinations of skiboot/kexec/SLOF 
>boots?

I've tested it under qemu/grub, simics/skiboot, and via kexec.
Balbir Singh Nov. 11, 2016, 1:17 a.m. UTC | #7
On 08/11/16 10:44, Reza Arbab wrote:
> When movable nodes are enabled, any node containing only hotpluggable
> memory is made movable at boot time.
> 
> On x86, hotpluggable memory is discovered by parsing the ACPI SRAT,
> making corresponding calls to memblock_mark_hotplug().
> 
> If we introduce a dt property to describe memory as hotpluggable,
> configs supporting early fdt may then also do this marking and use
> movable nodes.
> 
> Signed-off-by: Reza Arbab <arbab@linux.vnet.ibm.com>
> ---

Tested-by: Balbir Singh <bsingharora@gmail.com>

I tested this with a custom device tree and it worked quite well for me.
It also means that the guest and bare-metal have two different mechanisms
of marking something as hotpluggable. But given that your patch enables
all architectures using OF, it might be worth it.

Balbir Singh.
Michael Ellerman Nov. 14, 2016, 11:59 a.m. UTC | #8
Reza Arbab <arbab@linux.vnet.ibm.com> writes:

> When movable nodes are enabled, any node containing only hotpluggable
> memory is made movable at boot time.
>
> On x86, hotpluggable memory is discovered by parsing the ACPI SRAT,
> making corresponding calls to memblock_mark_hotplug().
>
> If we introduce a dt property to describe memory as hotpluggable,
> configs supporting early fdt may then also do this marking and use
> movable nodes.

So I'm not opposed to this, but it is a little vague.

What does the "hotpluggable" property really mean?

Is it just a hint to the operating system? (which may or may not be
Linux).

Or is it a direction, "this memory must be able to be hotunplugged"?

I think you're intending the former, ie. a hint, which is probably OK.
But it needs to be documented clearly.

cheers
Reza Arbab Nov. 14, 2016, 7:34 p.m. UTC | #9
On Mon, Nov 14, 2016 at 10:59:43PM +1100, Michael Ellerman wrote:
>So I'm not opposed to this, but it is a little vague.
>
>What does the "hotpluggable" property really mean?
>
>Is it just a hint to the operating system? (which may or may not be
>Linux).
>
>Or is it a direction, "this memory must be able to be hotunplugged"?
>
>I think you're intending the former, ie. a hint, which is probably OK.
>But it needs to be documented clearly.

Yes, you've got it right. It's just a hint, not a mandate.

I'm about to send v7 which adds a description of "hotpluggable" in the 
documentation. Hopefully I've explained it well enough there.
diff mbox

Patch

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index c89d5d2..2cf1d66 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1015,6 +1015,7 @@  int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
 	const char *type = of_get_flat_dt_prop(node, "device_type", NULL);
 	const __be32 *reg, *endp;
 	int l;
+	bool hotpluggable;
 
 	/* We are scanning "memory" nodes only */
 	if (type == NULL) {
@@ -1034,6 +1035,7 @@  int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
 		return 0;
 
 	endp = reg + (l / sizeof(__be32));
+	hotpluggable = of_get_flat_dt_prop(node, "linux,hotpluggable", NULL);
 
 	pr_debug("memory scan node %s, reg size %d,\n", uname, l);
 
@@ -1049,6 +1051,10 @@  int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
 		    (unsigned long long)size);
 
 		early_init_dt_add_memory_arch(base, size);
+
+		if (hotpluggable && memblock_mark_hotplug(base, size))
+			pr_warn("failed to mark hotplug range 0x%llx - 0x%llx\n",
+				base, base + size);
 	}
 
 	return 0;
diff --git a/mm/Kconfig b/mm/Kconfig
index 061b46b..33a9b06 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -153,7 +153,7 @@  config MOVABLE_NODE
 	bool "Enable to assign a node which has only movable memory"
 	depends on HAVE_MEMBLOCK
 	depends on NO_BOOTMEM
-	depends on X86_64 || MEMORY_HOTPLUG
+	depends on X86_64 || OF_EARLY_FLATTREE || MEMORY_HOTPLUG
 	depends on NUMA
 	default n
 	help