Re: [Qemu-devel] [RFC PATCH v0 0/2] Increase max memslots to 512 for PowerPC

Re: [Qemu-devel] [RFC PATCH v0 0/2] Increase max memslots to 512 for PowerPC

[Thomas Huth]

On 01.06.2016 11:51, Bharata B Rao wrote:
> Recently the number of memory slots supported by KVM for PowerPC was changed
> from 32 to 512. QEMU was restricting the user specifiable hot-pluggable memory
> slots to 32. This patchset changes that to 512.
> 
> This allows more number of slots to be available for memory hotplugging.

It's certainly a good idea to increase the number of slots for
hot-pluggable memory. But should we really increase it to the full
maximum of 512 slots? The in-kernel slots are shared with other memory
regions, too, e.g. the memory slots for PCI cards. So if you allow the
users to plug all slots with DIMMs, they certainly will run into
problems there again later.

I think x86 is also using 256 DIMM slots only on purpose (see
https://lkml.org/lkml/2014/11/14/328 for example), so we should maybe
also limit the max. number of DIMM slots on spapr to
kvm_get_max_memslots() divided by two ?

 Thomas

Re: [Qemu-devel] [RFC PATCH v0 0/2] Increase max memslots to 512 for PowerPC

[David Gibson]

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

On Wed, Jun 01, 2016 at 12:18:22PM +0200, Thomas Huth wrote:
> On 01.06.2016 11:51, Bharata B Rao wrote:
> > Recently the number of memory slots supported by KVM for PowerPC was changed
> > from 32 to 512. QEMU was restricting the user specifiable hot-pluggable memory
> > slots to 32. This patchset changes that to 512.
> > 
> > This allows more number of slots to be available for memory hotplugging.
> 
> It's certainly a good idea to increase the number of slots for
> hot-pluggable memory. But should we really increase it to the full
> maximum of 512 slots? The in-kernel slots are shared with other memory
> regions, too, e.g. the memory slots for PCI cards. So if you allow the
> users to plug all slots with DIMMs, they certainly will run into
> problems there again later.
> 
> I think x86 is also using 256 DIMM slots only on purpose (see
> https://lkml.org/lkml/2014/11/14/328 for example), so we should maybe
> also limit the max. number of DIMM slots on spapr to
> kvm_get_max_memslots() divided by two ?

I tend to agree.  I've held off on merging 2/2 pending outcome of this
discussion.

-- 
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: 819 bytes --]

Re: [Qemu-devel] [RFC PATCH v0 0/2] Increase max memslots to 512 for PowerPC

[Bharata B Rao]

On Thu, Jun 02, 2016 at 10:42:23AM +1000, David Gibson wrote:
> On Wed, Jun 01, 2016 at 12:18:22PM +0200, Thomas Huth wrote:
> > On 01.06.2016 11:51, Bharata B Rao wrote:
> > > Recently the number of memory slots supported by KVM for PowerPC was changed
> > > from 32 to 512. QEMU was restricting the user specifiable hot-pluggable memory
> > > slots to 32. This patchset changes that to 512.
> > > 
> > > This allows more number of slots to be available for memory hotplugging.
> > 
> > It's certainly a good idea to increase the number of slots for
> > hot-pluggable memory. But should we really increase it to the full
> > maximum of 512 slots? The in-kernel slots are shared with other memory
> > regions, too, e.g. the memory slots for PCI cards. So if you allow the
> > users to plug all slots with DIMMs, they certainly will run into
> > problems there again later.
> > 
> > I think x86 is also using 256 DIMM slots only on purpose (see
> > https://lkml.org/lkml/2014/11/14/328 for example), so we should maybe
> > also limit the max. number of DIMM slots on spapr to
> > kvm_get_max_memslots() divided by two ?
> 
> I tend to agree.  I've held off on merging 2/2 pending outcome of this
> discussion.

Agreed. Here is the updated patch:

spapr: Increase hotpluggable memory slots to 256

From: Bharata B Rao <bharata@linux.vnet.ibm.com>

KVM now supports 512 memslots on PowerPC (earlier it was 32). Allow half
of it (256) to be used as hotpluggable memory slots.

Instead of hard coding the max value, use the KVM supplied value if KVM
is enabled. Otherwise resort to the default value of 32.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
---
 hw/ppc/spapr.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 44e401a..c82adef 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1816,11 +1816,22 @@ static void ppc_spapr_init(MachineState *machine)
     /* initialize hotplug memory address space */
     if (machine->ram_size < machine->maxram_size) {
         ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
+        /*
+         * Number of memslots supported by KVM on PowerPC was increased
+         * from 32 to 512. Let us limit the number of hotpluggable slots
+         * to half of that (256). However ensure that number of slots
+         * doesn't drop below 32 on older hosts.
+         */
+        int max_memslots = kvm_enabled() ? kvm_get_max_memslots() / 2 :
+                           SPAPR_MAX_RAM_SLOTS;
 
-        if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
+        if (max_memslots < SPAPR_MAX_RAM_SLOTS) {
+            max_memslots = SPAPR_MAX_RAM_SLOTS;
+        }
+        if (machine->ram_slots > max_memslots) {
             error_report("Specified number of memory slots %"
                          PRIu64" exceeds max supported %d",
-                         machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
+                         machine->ram_slots, max_memslots);
             exit(1);
         }
 

Re: [Qemu-devel] [RFC PATCH v0 0/2] Increase max memslots to 512 for PowerPC

[Thomas Huth]

On 02.06.2016 06:39, Bharata B Rao wrote:
...
> Agreed. Here is the updated patch:
> 
> spapr: Increase hotpluggable memory slots to 256
> 
> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> KVM now supports 512 memslots on PowerPC (earlier it was 32). Allow half
> of it (256) to be used as hotpluggable memory slots.
> 
> Instead of hard coding the max value, use the KVM supplied value if KVM
> is enabled. Otherwise resort to the default value of 32.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> ---
>  hw/ppc/spapr.c |   15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 44e401a..c82adef 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1816,11 +1816,22 @@ static void ppc_spapr_init(MachineState *machine)
>      /* initialize hotplug memory address space */
>      if (machine->ram_size < machine->maxram_size) {
>          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> +        /*
> +         * Number of memslots supported by KVM on PowerPC was increased
> +         * from 32 to 512. Let us limit the number of hotpluggable slots
> +         * to half of that (256). However ensure that number of slots
> +         * doesn't drop below 32 on older hosts.
> +         */

Using "hard-coded" information like "increased to 512" in comments is
true for the current state, but this has a risk of being out of date
soon. Once we change the memslots in the kernel, this comment is not
true anymore and might cause confusion. Better talk about leaving half
of the kernel memslots for PCI and other devices, or so.

> +        int max_memslots = kvm_enabled() ? kvm_get_max_memslots() / 2 :
> +                           SPAPR_MAX_RAM_SLOTS;
>  
> -        if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
> +        if (max_memslots < SPAPR_MAX_RAM_SLOTS) {
> +            max_memslots = SPAPR_MAX_RAM_SLOTS;
> +        }
> +        if (machine->ram_slots > max_memslots) {
>              error_report("Specified number of memory slots %"
>                           PRIu64" exceeds max supported %d",
> -                         machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
> +                         machine->ram_slots, max_memslots);
>              exit(1);
>          }

The changes to the code look fine to me. So once you've updated the
comment, feel free to add my "Reviewed-by".

 Thomas

Re: [Qemu-devel] [RFC PATCH v0 0/2] Increase max memslots to 512 for PowerPC

[Bharata B Rao]

On Thu, Jun 02, 2016 at 09:03:15AM +0200, Thomas Huth wrote:
> On 02.06.2016 06:39, Bharata B Rao wrote:
> ...
> > Agreed. Here is the updated patch:
> > 
> > spapr: Increase hotpluggable memory slots to 256
> > 
> > From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > 
> > KVM now supports 512 memslots on PowerPC (earlier it was 32). Allow half
> > of it (256) to be used as hotpluggable memory slots.
> > 
> > Instead of hard coding the max value, use the KVM supplied value if KVM
> > is enabled. Otherwise resort to the default value of 32.
> > 
> > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > ---
> >  hw/ppc/spapr.c |   15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 44e401a..c82adef 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1816,11 +1816,22 @@ static void ppc_spapr_init(MachineState *machine)
> >      /* initialize hotplug memory address space */
> >      if (machine->ram_size < machine->maxram_size) {
> >          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> > +        /*
> > +         * Number of memslots supported by KVM on PowerPC was increased
> > +         * from 32 to 512. Let us limit the number of hotpluggable slots
> > +         * to half of that (256). However ensure that number of slots
> > +         * doesn't drop below 32 on older hosts.
> > +         */
> 
> Using "hard-coded" information like "increased to 512" in comments is
> true for the current state, but this has a risk of being out of date
> soon. Once we change the memslots in the kernel, this comment is not
> true anymore and might cause confusion. Better talk about leaving half
> of the kernel memslots for PCI and other devices, or so.

Just want to note that even though we are limiting hotpluggable memory
slots to half of max, it is always possible for other devices to eat
into the memory hotplug slots, right ?

David - here is the patch updated with comments as suggested by Thomas.
If you need a separate post, let me know.

spapr: Increase hotpluggable memory slots to 256

From: Bharata B Rao <bharata@linux.vnet.ibm.com>

KVM now supports 512 memslots on PowerPC (earlier it was 32). Allow half
of it (256) to be used as hotpluggable memory slots.

Instead of hard coding the max value, use the KVM supplied value if KVM
is enabled. Otherwise resort to the default value of 32.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 44e401a..14cc6ae 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1816,11 +1816,21 @@ static void ppc_spapr_init(MachineState *machine)
     /* initialize hotplug memory address space */
     if (machine->ram_size < machine->maxram_size) {
         ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
+        /*
+         * Limit the number of hotpluggable memory slots to half the number
+         * slots that KVM supports, leaving the other half for PCI and other
+         * devices. However ensure that number of slots doesn't drop below 32.
+         */
+        int max_memslots = kvm_enabled() ? kvm_get_max_memslots() / 2 :
+                           SPAPR_MAX_RAM_SLOTS;
 
-        if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
+        if (max_memslots < SPAPR_MAX_RAM_SLOTS) {
+            max_memslots = SPAPR_MAX_RAM_SLOTS;
+        }
+        if (machine->ram_slots > max_memslots) {
             error_report("Specified number of memory slots %"
                          PRIu64" exceeds max supported %d",
-                         machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
+                         machine->ram_slots, max_memslots);
             exit(1);
         }
 

Re: [Qemu-devel] [RFC PATCH v0 0/2] Increase max memslots to 512 for PowerPC

[Thomas Huth]

On 02.06.2016 16:07, Bharata B Rao wrote:
> On Thu, Jun 02, 2016 at 09:03:15AM +0200, Thomas Huth wrote:
>> On 02.06.2016 06:39, Bharata B Rao wrote:
>> ...
>>> Agreed. Here is the updated patch:
>>>
>>> spapr: Increase hotpluggable memory slots to 256
>>>
>>> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
>>>
>>> KVM now supports 512 memslots on PowerPC (earlier it was 32). Allow half
>>> of it (256) to be used as hotpluggable memory slots.
>>>
>>> Instead of hard coding the max value, use the KVM supplied value if KVM
>>> is enabled. Otherwise resort to the default value of 32.
>>>
>>> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
>>> ---
>>>  hw/ppc/spapr.c |   15 +++++++++++++--
>>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 44e401a..c82adef 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -1816,11 +1816,22 @@ static void ppc_spapr_init(MachineState *machine)
>>>      /* initialize hotplug memory address space */
>>>      if (machine->ram_size < machine->maxram_size) {
>>>          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
>>> +        /*
>>> +         * Number of memslots supported by KVM on PowerPC was increased
>>> +         * from 32 to 512. Let us limit the number of hotpluggable slots
>>> +         * to half of that (256). However ensure that number of slots
>>> +         * doesn't drop below 32 on older hosts.
>>> +         */
>>
>> Using "hard-coded" information like "increased to 512" in comments is
>> true for the current state, but this has a risk of being out of date
>> soon. Once we change the memslots in the kernel, this comment is not
>> true anymore and might cause confusion. Better talk about leaving half
>> of the kernel memslots for PCI and other devices, or so.
> 
> Just want to note that even though we are limiting hotpluggable memory
> slots to half of max, it is always possible for other devices to eat
> into the memory hotplug slots, right ?

Right. But 256 slots for those other devices is already plenty, I think.
Let's hope that it is enough for a while - if it's not enough, we've got
to rework the kernel code again (and maybe switch to dynamic memslots
allocation instead).

 Thomas

Re: [Qemu-devel] [RFC PATCH v0 0/2] Increase max memslots to 512 for PowerPC

[David Gibson]

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

On Thu, Jun 02, 2016 at 07:37:37PM +0530, Bharata B Rao wrote:
> On Thu, Jun 02, 2016 at 09:03:15AM +0200, Thomas Huth wrote:
> > On 02.06.2016 06:39, Bharata B Rao wrote:
> > ...
> > > Agreed. Here is the updated patch:
> > > 
> > > spapr: Increase hotpluggable memory slots to 256
> > > 
> > > From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > 
> > > KVM now supports 512 memslots on PowerPC (earlier it was 32). Allow half
> > > of it (256) to be used as hotpluggable memory slots.
> > > 
> > > Instead of hard coding the max value, use the KVM supplied value if KVM
> > > is enabled. Otherwise resort to the default value of 32.
> > > 
> > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > > ---
> > >  hw/ppc/spapr.c |   15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 44e401a..c82adef 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1816,11 +1816,22 @@ static void ppc_spapr_init(MachineState *machine)
> > >      /* initialize hotplug memory address space */
> > >      if (machine->ram_size < machine->maxram_size) {
> > >          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> > > +        /*
> > > +         * Number of memslots supported by KVM on PowerPC was increased
> > > +         * from 32 to 512. Let us limit the number of hotpluggable slots
> > > +         * to half of that (256). However ensure that number of slots
> > > +         * doesn't drop below 32 on older hosts.
> > > +         */
> > 
> > Using "hard-coded" information like "increased to 512" in comments is
> > true for the current state, but this has a risk of being out of date
> > soon. Once we change the memslots in the kernel, this comment is not
> > true anymore and might cause confusion. Better talk about leaving half
> > of the kernel memslots for PCI and other devices, or so.
> 
> Just want to note that even though we are limiting hotpluggable memory
> slots to half of max, it is always possible for other devices to eat
> into the memory hotplug slots, right ?
> 
> David - here is the patch updated with comments as suggested by Thomas.
> If you need a separate post, let me know.

Applied to ppc-for-2.7, thanks.

> 
> spapr: Increase hotpluggable memory slots to 256
> 
> From: Bharata B Rao <bharata@linux.vnet.ibm.com>
> 
> KVM now supports 512 memslots on PowerPC (earlier it was 32). Allow half
> of it (256) to be used as hotpluggable memory slots.
> 
> Instead of hard coding the max value, use the KVM supplied value if KVM
> is enabled. Otherwise resort to the default value of 32.
> 
> Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/ppc/spapr.c |   14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 44e401a..14cc6ae 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1816,11 +1816,21 @@ static void ppc_spapr_init(MachineState *machine)
>      /* initialize hotplug memory address space */
>      if (machine->ram_size < machine->maxram_size) {
>          ram_addr_t hotplug_mem_size = machine->maxram_size - machine->ram_size;
> +        /*
> +         * Limit the number of hotpluggable memory slots to half the number
> +         * slots that KVM supports, leaving the other half for PCI and other
> +         * devices. However ensure that number of slots doesn't drop below 32.
> +         */
> +        int max_memslots = kvm_enabled() ? kvm_get_max_memslots() / 2 :
> +                           SPAPR_MAX_RAM_SLOTS;
>  
> -        if (machine->ram_slots > SPAPR_MAX_RAM_SLOTS) {
> +        if (max_memslots < SPAPR_MAX_RAM_SLOTS) {
> +            max_memslots = SPAPR_MAX_RAM_SLOTS;
> +        }
> +        if (machine->ram_slots > max_memslots) {
>              error_report("Specified number of memory slots %"
>                           PRIu64" exceeds max supported %d",
> -                         machine->ram_slots, SPAPR_MAX_RAM_SLOTS);
> +                         machine->ram_slots, max_memslots);
>              exit(1);
>          }
>  
> 

-- 
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: 819 bytes --]

[Qemu-devel] [RFC PATCH v0 0/2] Increase max memslots to 512 for PowerPC

[Bharata B Rao]

Recently the number of memory slots supported by KVM for PowerPC was changed
from 32 to 512. QEMU was restricting the user specifiable hot-pluggable memory
slots to 32. This patchset changes that to 512.

This allows more number of slots to be available for memory hotplugging.

Bharata B Rao (2):
  kvm: API to obtain max supported mem slots
  spapr: Increase max memslots to 512

 hw/ppc/spapr.c       | 6 ++++--
 include/sysemu/kvm.h | 1 +
 kvm-all.c            | 7 +++++++
 3 files changed, 12 insertions(+), 2 deletions(-)

-- 
2.1.0