[Qemu-devel] [PATCH for 2.13 0/2] target/ppc: Support adding memory to initially memory-less NUMA nodes

[Qemu-devel] [PATCH for 2.13 0/2] target/ppc: Support adding memory to initially memory-less NUMA nodes

[Serhii Popovych]

Now PowerPC Linux kernel supports hot-add to NUMA nodes not populated
initially with memory we can enable such support in qemu. This requires
two changes:

  o Add device tree property "ibm,max-associativity-domains" to let
    guest kernel chance to find max possible NUMA node

  o Revert  commit b556854bd852 ("spapr: Don't allow memory hotplug to
    memory less nodes") to remove check for hot-add to memory-less node.

See description messges for individual changes for more details.

Serhii Popovych (2):
  Revert "spapr: Don't allow memory hotplug to memory less nodes"
  spapr: Add ibm,max-associativity-domains property

 hw/ppc/spapr.c | 33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH for 2.13 1/2] Revert "spapr: Don't allow memory hotplug to memory less nodes"

[Serhii Popovych]

This reverts commit b556854bd8524c26b8be98ab1bfdf0826831e793.

Leave change @node type from uint32_t to to int from reverted commit
because node < 0 is always false.

Signed-off-by: Serhii Popovych <spopovyc@redhat.com>
---
 hw/ppc/spapr.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 2c0be8c..3ad4545 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3477,28 +3477,6 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
             return;
         }
 
-        /*
-         * Currently PowerPC kernel doesn't allow hot-adding memory to
-         * memory-less node, but instead will silently add the memory
-         * to the first node that has some memory. This causes two
-         * unexpected behaviours for the user.
-         *
-         * - Memory gets hotplugged to a different node than what the user
-         *   specified.
-         * - Since pc-dimm subsystem in QEMU still thinks that memory belongs
-         *   to memory-less node, a reboot will set things accordingly
-         *   and the previously hotplugged memory now ends in the right node.
-         *   This appears as if some memory moved from one node to another.
-         *
-         * So until kernel starts supporting memory hotplug to memory-less
-         * nodes, just prevent such attempts upfront in QEMU.
-         */
-        if (nb_numa_nodes && !numa_info[node].node_mem) {
-            error_setg(errp, "Can't hotplug memory to memory-less node %d",
-                       node);
-            return;
-        }
-
         spapr_memory_plug(hotplug_dev, dev, node, errp);
     } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
         spapr_core_plug(hotplug_dev, dev, errp);
-- 
1.8.3.1

[Qemu-devel] [PATCH for 2.13 2/2] spapr: Add ibm, max-associativity-domains property

[Serhii Popovych]

Now recent kernels (i.e. since linux-stable commit a346137e9142
("powerpc/numa: Use ibm,max-associativity-domains to discover possible nodes")
support this property to mark initially memory-less NUMA nodes as "possible"
to allow further memory hot-add to them.

Advertise this property for pSeries machines to let guest kernels detect
maximum supported node configuration and benefit from kernel side change
when hot-add memory to specific, possibly empty before, NUMA node.

Signed-off-by: Serhii Popovych <spopovyc@redhat.com>
---
 hw/ppc/spapr.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 3ad4545..e02fc94 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -909,6 +909,14 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
         0, cpu_to_be32(SPAPR_MEMORY_BLOCK_SIZE),
         cpu_to_be32(max_cpus / smp_threads),
     };
+    uint32_t maxdomains[] = {
+        cpu_to_be32(5),
+        cpu_to_be32(0),
+        cpu_to_be32(0),
+        cpu_to_be32(0),
+        cpu_to_be32(nb_numa_nodes - 1),
+        cpu_to_be32(max_cpus - 1),
+    };
 
     _FDT(rtas = fdt_add_subnode(fdt, 0, "rtas"));
 
@@ -945,6 +953,9 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
     _FDT(fdt_setprop(fdt, rtas, "ibm,associativity-reference-points",
                      refpoints, sizeof(refpoints)));
 
+    _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains",
+                     maxdomains, sizeof(maxdomains)));
+
     _FDT(fdt_setprop_cell(fdt, rtas, "rtas-error-log-max",
                           RTAS_ERROR_LOG_MAX));
     _FDT(fdt_setprop_cell(fdt, rtas, "rtas-event-scan-rate",
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH for 2.13 1/2] Revert "spapr: Don't allow memory hotplug to memory less nodes"

[Bharata B Rao]

On Thu, Apr 05, 2018 at 10:35:22AM -0400, Serhii Popovych wrote:
> This reverts commit b556854bd8524c26b8be98ab1bfdf0826831e793.
> 
> Leave change @node type from uint32_t to to int from reverted commit
> because node < 0 is always false.
> 
> Signed-off-by: Serhii Popovych <spopovyc@redhat.com>
> ---
>  hw/ppc/spapr.c | 22 ----------------------
>  1 file changed, 22 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 2c0be8c..3ad4545 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3477,28 +3477,6 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>              return;
>          }
> 
> -        /*
> -         * Currently PowerPC kernel doesn't allow hot-adding memory to
> -         * memory-less node, but instead will silently add the memory
> -         * to the first node that has some memory. This causes two
> -         * unexpected behaviours for the user.
> -         *
> -         * - Memory gets hotplugged to a different node than what the user
> -         *   specified.
> -         * - Since pc-dimm subsystem in QEMU still thinks that memory belongs
> -         *   to memory-less node, a reboot will set things accordingly
> -         *   and the previously hotplugged memory now ends in the right node.
> -         *   This appears as if some memory moved from one node to another.
> -         *
> -         * So until kernel starts supporting memory hotplug to memory-less
> -         * nodes, just prevent such attempts upfront in QEMU.
> -         */
> -        if (nb_numa_nodes && !numa_info[node].node_mem) {
> -            error_setg(errp, "Can't hotplug memory to memory-less node %d",
> -                       node);
> -            return;
> -        }
> -

If you remove this unconditionally, wouldn't it be a problem in case
of newer QEMU with older guest kernels ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH for 2.13 1/2] Revert "spapr: Don't allow memory hotplug to memory less nodes"

[Serhii Popovych]

[-- Attachment #1: Type: text/plain, Size: 2194 bytes --]

Bharata B Rao wrote:
> On Thu, Apr 05, 2018 at 10:35:22AM -0400, Serhii Popovych wrote:
>> This reverts commit b556854bd8524c26b8be98ab1bfdf0826831e793.
>>
>> Leave change @node type from uint32_t to to int from reverted commit
>> because node < 0 is always false.
>>
>> Signed-off-by: Serhii Popovych <spopovyc@redhat.com>
>> ---
>>  hw/ppc/spapr.c | 22 ----------------------
>>  1 file changed, 22 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 2c0be8c..3ad4545 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -3477,28 +3477,6 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
>>              return;
>>          }
>>
>> -        /*
>> -         * Currently PowerPC kernel doesn't allow hot-adding memory to
>> -         * memory-less node, but instead will silently add the memory
>> -         * to the first node that has some memory. This causes two
>> -         * unexpected behaviours for the user.
>> -         *
>> -         * - Memory gets hotplugged to a different node than what the user
>> -         *   specified.
>> -         * - Since pc-dimm subsystem in QEMU still thinks that memory belongs
>> -         *   to memory-less node, a reboot will set things accordingly
>> -         *   and the previously hotplugged memory now ends in the right node.
>> -         *   This appears as if some memory moved from one node to another.
>> -         *
>> -         * So until kernel starts supporting memory hotplug to memory-less
>> -         * nodes, just prevent such attempts upfront in QEMU.
>> -         */
>> -        if (nb_numa_nodes && !numa_info[node].node_mem) {
>> -            error_setg(errp, "Can't hotplug memory to memory-less node %d",
>> -                       node);
>> -            return;
>> -        }
>> -
> 
> If you remove this unconditionally, wouldn't it be a problem in case
> of newer QEMU with older guest kernels ?

Yes, that definitely would affect guest kernels without such support. We
probably need to add some capability to test for guest kernel
functionality presence.

> 
> Regards,
> Bharata.
> 
> 


-- 
Thanks,
Serhii


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH for 2.13 0/2] target/ppc: Support adding memory to initially memory-less NUMA nodes

[Greg Kurz]

On Thu,  5 Apr 2018 10:35:21 -0400
Serhii Popovych <spopovyc@redhat.com> wrote:

> Now PowerPC Linux kernel supports hot-add to NUMA nodes not populated
> initially with memory we can enable such support in qemu. This requires
> two changes:
> 
>   o Add device tree property "ibm,max-associativity-domains" to let
>     guest kernel chance to find max possible NUMA node
> 
>   o Revert  commit b556854bd852 ("spapr: Don't allow memory hotplug to
>     memory less nodes") to remove check for hot-add to memory-less node.
> 

But the series do the changes in the opposite order... 

IIUC correctly a recent kernel will mis-behave as before linux commit
a346137e9142 if the property is not present... ie, patch 2 should come
first.

> See description messges for individual changes for more details.
> 
> Serhii Popovych (2):
>   Revert "spapr: Don't allow memory hotplug to memory less nodes"
>   spapr: Add ibm,max-associativity-domains property
> 
>  hw/ppc/spapr.c | 33 +++++++++++----------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 

Re: [Qemu-devel] [PATCH for 2.13 1/2] Revert "spapr: Don't allow memory hotplug to memory less nodes"

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3081 bytes --]

On Fri, Apr 06, 2018 at 08:48:55AM +0300, Serhii Popovych wrote:
> Bharata B Rao wrote:
> > On Thu, Apr 05, 2018 at 10:35:22AM -0400, Serhii Popovych wrote:
> >> This reverts commit b556854bd8524c26b8be98ab1bfdf0826831e793.
> >>
> >> Leave change @node type from uint32_t to to int from reverted commit
> >> because node < 0 is always false.
> >>
> >> Signed-off-by: Serhii Popovych <spopovyc@redhat.com>
> >> ---
> >>  hw/ppc/spapr.c | 22 ----------------------
> >>  1 file changed, 22 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 2c0be8c..3ad4545 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3477,28 +3477,6 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev,
> >>              return;
> >>          }
> >>
> >> -        /*
> >> -         * Currently PowerPC kernel doesn't allow hot-adding memory to
> >> -         * memory-less node, but instead will silently add the memory
> >> -         * to the first node that has some memory. This causes two
> >> -         * unexpected behaviours for the user.
> >> -         *
> >> -         * - Memory gets hotplugged to a different node than what the user
> >> -         *   specified.
> >> -         * - Since pc-dimm subsystem in QEMU still thinks that memory belongs
> >> -         *   to memory-less node, a reboot will set things accordingly
> >> -         *   and the previously hotplugged memory now ends in the right node.
> >> -         *   This appears as if some memory moved from one node to another.
> >> -         *
> >> -         * So until kernel starts supporting memory hotplug to memory-less
> >> -         * nodes, just prevent such attempts upfront in QEMU.
> >> -         */
> >> -        if (nb_numa_nodes && !numa_info[node].node_mem) {
> >> -            error_setg(errp, "Can't hotplug memory to memory-less node %d",
> >> -                       node);
> >> -            return;
> >> -        }
> >> -
> > 
> > If you remove this unconditionally, wouldn't it be a problem in case
> > of newer QEMU with older guest kernels ?
> 
> Yes, that definitely would affect guest kernels without such support. We
> probably need to add some capability to test for guest kernel
> functionality presence.

Hm, maybe.

So first, we should check when the guest side support came in.  If
it's old enough we might not care.

PAPR does include a mechanism for negotiating guest/host
capabilities.  However, I don't think it has a bit for this specific
feature, so I can't really see a way to do this cleanly.

I don't think we necessarily have to handle that case: it's not like
we can reasonably workaround *every* possible guest bug/limitation
from the host side.  If you want to create a system with a memory-less
node you need an OS that can handle that, nothing really special there.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH for 2.13 0/2] target/ppc: Support adding memory to initially memory-less NUMA nodes

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1493 bytes --]

On Fri, Apr 06, 2018 at 10:21:36AM +0200, Greg Kurz wrote:
> On Thu,  5 Apr 2018 10:35:21 -0400
> Serhii Popovych <spopovyc@redhat.com> wrote:
> 
> > Now PowerPC Linux kernel supports hot-add to NUMA nodes not populated
> > initially with memory we can enable such support in qemu. This requires
> > two changes:
> > 
> >   o Add device tree property "ibm,max-associativity-domains" to let
> >     guest kernel chance to find max possible NUMA node
> > 
> >   o Revert  commit b556854bd852 ("spapr: Don't allow memory hotplug to
> >     memory less nodes") to remove check for hot-add to memory-less node.
> > 
> 
> But the series do the changes in the opposite order... 

Right, the patches should go in the reverse order to avoid breaking
bisections.

> IIUC correctly a recent kernel will mis-behave as before linux commit
> a346137e9142 if the property is not present... ie, patch 2 should come
> first.
> 
> > See description messges for individual changes for more details.
> > 
> > Serhii Popovych (2):
> >   Revert "spapr: Don't allow memory hotplug to memory less nodes"
> >   spapr: Add ibm,max-associativity-domains property
> > 
> >  hw/ppc/spapr.c | 33 +++++++++++----------------------
> >  1 file changed, 11 insertions(+), 22 deletions(-)
> > 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH for 2.13 2/2] spapr: Add ibm, max-associativity-domains property

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2713 bytes --]

On Thu, Apr 05, 2018 at 10:35:23AM -0400, Serhii Popovych wrote:
> Now recent kernels (i.e. since linux-stable commit a346137e9142
> ("powerpc/numa: Use ibm,max-associativity-domains to discover possible nodes")
> support this property to mark initially memory-less NUMA nodes as "possible"
> to allow further memory hot-add to them.
> 
> Advertise this property for pSeries machines to let guest kernels detect
> maximum supported node configuration and benefit from kernel side change
> when hot-add memory to specific, possibly empty before, NUMA node.
> 
> Signed-off-by: Serhii Popovych <spopovyc@redhat.com>
> ---
>  hw/ppc/spapr.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 3ad4545..e02fc94 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -909,6 +909,14 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
>          0, cpu_to_be32(SPAPR_MEMORY_BLOCK_SIZE),
>          cpu_to_be32(max_cpus / smp_threads),
>      };
> +    uint32_t maxdomains[] = {
> +        cpu_to_be32(5),
> +        cpu_to_be32(0),
> +        cpu_to_be32(0),
> +        cpu_to_be32(0),
> +        cpu_to_be32(nb_numa_nodes - 1),
> +        cpu_to_be32(max_cpus - 1),
> +    };

There's a minor problem here, which I didn't think of when I was
discussing this with you earlier.

(max_cpus - 1) in the last slot isn't quite right, because we need the
maximum vcpu id here.  Because of (complicated, historical) reasons we
don't allocate the vcpu ids contiguously in all cases, so it could be
larger than max_cpus - 1.

But, we don't actually need to handle that case.  We give 5 levels of
associativity on cpus, but only 4 on memory.  Having a quick look at
the NUMA code on the guest side, I'm pretty sure it's ok if we only
give maximum values to depth 4 here, so we can just drop the last cell
here.

>  
>      _FDT(rtas = fdt_add_subnode(fdt, 0, "rtas"));
>  
> @@ -945,6 +953,9 @@ static void spapr_dt_rtas(sPAPRMachineState *spapr, void *fdt)
>      _FDT(fdt_setprop(fdt, rtas, "ibm,associativity-reference-points",
>                       refpoints, sizeof(refpoints)));
>  
> +    _FDT(fdt_setprop(fdt, rtas, "ibm,max-associativity-domains",
> +                     maxdomains, sizeof(maxdomains)));
> +
>      _FDT(fdt_setprop_cell(fdt, rtas, "rtas-error-log-max",
>                            RTAS_ERROR_LOG_MAX));
>      _FDT(fdt_setprop_cell(fdt, rtas, "rtas-event-scan-rate",

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]