diff mbox

[RFC,v0,0/2] Increase max memslots to 512 for PowerPC

Message ID 201606020440.u524ZiVe027809@mx0a-001b2d01.pphosted.com
State New
Headers show

Commit Message

Bharata B Rao June 2, 2016, 4:39 a.m. UTC 
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(-)

Comments

Thomas Huth June 2, 2016, 7:03 a.m. UTC | #1 
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
diff mbox

Patch

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);
         }