[Qemu-devel] [PATCH] ppc: Huge page detection mechanism fixes - Episode III

[Qemu-devel] [PATCH] ppc: Huge page detection mechanism fixes - Episode III

[Thomas Huth]

After already fixing two issues with the huge page detection mechanism
(see commit 159d2e39a860 and 86b50f2e1bef), Greg Kurz noticed another
case that caused the guest to crash where QEMU announces huge pages
though they should not be available for the guest:

qemu-system-ppc64 -enable-kvm ... -mem-path /dev/hugepages \
 -m 1G,slots=4,maxmem=32G
 -object memory-backend-ram,policy=default,size=1G,id=mem-mem1 \
 -device pc-dimm,id=dimm-mem1,memdev=mem-mem1 -smp 2 \
 -numa node,nodeid=0 -numa node,nodeid=1

That means if there is a global mem-path option, we still have
to look at the memory-backend objects that have been specified
additionally and return their minimum page size if that value
is smaller than the page size of the main memory.

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target-ppc/kvm.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 7a8f555..97ab450 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -366,10 +366,13 @@ static int find_max_supported_pagesize(Object *obj, void *opaque)
 static long getrampagesize(void)
 {
     long hpsize = LONG_MAX;
+    long mainrampagesize;
     Object *memdev_root;
 
     if (mem_path) {
-        return gethugepagesize(mem_path);
+        mainrampagesize = gethugepagesize(mem_path);
+    } else {
+        mainrampagesize = getpagesize();
     }
 
     /* it's possible we have memory-backend objects with
@@ -383,28 +386,26 @@ static long getrampagesize(void)
      * backend isn't backed by hugepages.
      */
     memdev_root = object_resolve_path("/objects", NULL);
-    if (!memdev_root) {
-        return getpagesize();
+    if (memdev_root) {
+        object_child_foreach(memdev_root, find_max_supported_pagesize, &hpsize);
     }
-
-    object_child_foreach(memdev_root, find_max_supported_pagesize, &hpsize);
-
-    if (hpsize == LONG_MAX || hpsize == getpagesize()) {
-        return getpagesize();
+    if (hpsize == LONG_MAX) {
+        /* No additional memory regions found ==> Report main RAM page size */
+        return mainrampagesize;
     }
 
     /* If NUMA is disabled or the NUMA nodes are not backed with a
-     * memory-backend, then there is at least one node using "normal"
-     * RAM. And since normal RAM has not been configured with "-mem-path"
-     * (what we've checked earlier here already), we can not use huge pages!
+     * memory-backend, then there is at least one node using "normal" RAM,
+     * so if its page size is smaller we have got to report that size instead.
      */
-    if (nb_numa_nodes == 0 || numa_info[0].node_memdev == NULL) {
+    if (hpsize > mainrampagesize &&
+        (nb_numa_nodes == 0 || numa_info[0].node_memdev == NULL)) {
         static bool warned;
         if (!warned) {
             error_report("Huge page support disabled (n/a for main memory).");
             warned = true;
         }
-        return getpagesize();
+        return mainrampagesize;
     }
 
     return hpsize;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] ppc: Huge page detection mechanism fixes - Episode III

[Greg Kurz]

On Mon, 18 Jul 2016 15:19:04 +0200
Thomas Huth <thuth@redhat.com> wrote:

> After already fixing two issues with the huge page detection mechanism
> (see commit 159d2e39a860 and 86b50f2e1bef), Greg Kurz noticed another
> case that caused the guest to crash where QEMU announces huge pages
> though they should not be available for the guest:
> 
> qemu-system-ppc64 -enable-kvm ... -mem-path /dev/hugepages \
>  -m 1G,slots=4,maxmem=32G
>  -object memory-backend-ram,policy=default,size=1G,id=mem-mem1 \
>  -device pc-dimm,id=dimm-mem1,memdev=mem-mem1 -smp 2 \
>  -numa node,nodeid=0 -numa node,nodeid=1
> 
> That means if there is a global mem-path option, we still have
> to look at the memory-backend objects that have been specified
> additionally and return their minimum page size if that value
> is smaller than the page size of the main memory.
> 
> Reported-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---

Just one remark, see below, but apart from that:

Reviewed-by: Greg Kurz <groug@kaod.org>
Tested-by: Greg Kurz <groug@kaod.org>

>  target-ppc/kvm.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 7a8f555..97ab450 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -366,10 +366,13 @@ static int find_max_supported_pagesize(Object *obj, void *opaque)
>  static long getrampagesize(void)
>  {
>      long hpsize = LONG_MAX;
> +    long mainrampagesize;
>      Object *memdev_root;
>  
>      if (mem_path) {
> -        return gethugepagesize(mem_path);
> +        mainrampagesize = gethugepagesize(mem_path);
> +    } else {
> +        mainrampagesize = getpagesize();
>      }
>  
>      /* it's possible we have memory-backend objects with
> @@ -383,28 +386,26 @@ static long getrampagesize(void)
>       * backend isn't backed by hugepages.
>       */
>      memdev_root = object_resolve_path("/objects", NULL);
> -    if (!memdev_root) {
> -        return getpagesize();
> +    if (memdev_root) {
> +        object_child_foreach(memdev_root, find_max_supported_pagesize, &hpsize);
>      }
> -
> -    object_child_foreach(memdev_root, find_max_supported_pagesize, &hpsize);
> -
> -    if (hpsize == LONG_MAX || hpsize == getpagesize()) {
> -        return getpagesize();
> +    if (hpsize == LONG_MAX) {
> +        /* No additional memory regions found ==> Report main RAM page size */
> +        return mainrampagesize;
>      }
>  
>      /* If NUMA is disabled or the NUMA nodes are not backed with a
> -     * memory-backend, then there is at least one node using "normal"
> -     * RAM. And since normal RAM has not been configured with "-mem-path"
> -     * (what we've checked earlier here already), we can not use huge pages!
> +     * memory-backend, then there is at least one node using "normal" RAM,
> +     * so if its page size is smaller we have got to report that size instead.
>       */
> -    if (nb_numa_nodes == 0 || numa_info[0].node_memdev == NULL) {
> +    if (hpsize > mainrampagesize &&
> +        (nb_numa_nodes == 0 || numa_info[0].node_memdev == NULL)) {
>          static bool warned;
>          if (!warned) {
>              error_report("Huge page support disabled (n/a for main memory).");

Maybe update the error message since we have another condition ?

Something like:

"Huge page support disabled (at least one numa uses standard page size)"

>              warned = true;
>          }
> -        return getpagesize();
> +        return mainrampagesize;
>      }
>  
>      return hpsize;

Re: [Qemu-devel] [PATCH] ppc: Huge page detection mechanism fixes - Episode III

[David Gibson]

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

On Mon, Jul 18, 2016 at 03:19:04PM +0200, Thomas Huth wrote:
> After already fixing two issues with the huge page detection mechanism
> (see commit 159d2e39a860 and 86b50f2e1bef), Greg Kurz noticed another
> case that caused the guest to crash where QEMU announces huge pages
> though they should not be available for the guest:
> 
> qemu-system-ppc64 -enable-kvm ... -mem-path /dev/hugepages \
>  -m 1G,slots=4,maxmem=32G
>  -object memory-backend-ram,policy=default,size=1G,id=mem-mem1 \
>  -device pc-dimm,id=dimm-mem1,memdev=mem-mem1 -smp 2 \
>  -numa node,nodeid=0 -numa node,nodeid=1
> 
> That means if there is a global mem-path option, we still have
> to look at the memory-backend objects that have been specified
> additionally and return their minimum page size if that value
> is smaller than the page size of the main memory.
> 
> Reported-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Applied to ppc-for-2.7, thanks.

> ---
>  target-ppc/kvm.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 7a8f555..97ab450 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -366,10 +366,13 @@ static int find_max_supported_pagesize(Object *obj, void *opaque)
>  static long getrampagesize(void)
>  {
>      long hpsize = LONG_MAX;
> +    long mainrampagesize;
>      Object *memdev_root;
>  
>      if (mem_path) {
> -        return gethugepagesize(mem_path);
> +        mainrampagesize = gethugepagesize(mem_path);
> +    } else {
> +        mainrampagesize = getpagesize();
>      }
>  
>      /* it's possible we have memory-backend objects with
> @@ -383,28 +386,26 @@ static long getrampagesize(void)
>       * backend isn't backed by hugepages.
>       */
>      memdev_root = object_resolve_path("/objects", NULL);
> -    if (!memdev_root) {
> -        return getpagesize();
> +    if (memdev_root) {
> +        object_child_foreach(memdev_root, find_max_supported_pagesize, &hpsize);
>      }
> -
> -    object_child_foreach(memdev_root, find_max_supported_pagesize, &hpsize);
> -
> -    if (hpsize == LONG_MAX || hpsize == getpagesize()) {
> -        return getpagesize();
> +    if (hpsize == LONG_MAX) {
> +        /* No additional memory regions found ==> Report main RAM page size */
> +        return mainrampagesize;
>      }
>  
>      /* If NUMA is disabled or the NUMA nodes are not backed with a
> -     * memory-backend, then there is at least one node using "normal"
> -     * RAM. And since normal RAM has not been configured with "-mem-path"
> -     * (what we've checked earlier here already), we can not use huge pages!
> +     * memory-backend, then there is at least one node using "normal" RAM,
> +     * so if its page size is smaller we have got to report that size instead.
>       */
> -    if (nb_numa_nodes == 0 || numa_info[0].node_memdev == NULL) {
> +    if (hpsize > mainrampagesize &&
> +        (nb_numa_nodes == 0 || numa_info[0].node_memdev == NULL)) {
>          static bool warned;
>          if (!warned) {
>              error_report("Huge page support disabled (n/a for main memory).");
>              warned = true;
>          }
> -        return getpagesize();
> +        return mainrampagesize;
>      }
>  
>      return hpsize;

-- 
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] [PATCH] ppc: Huge page detection mechanism fixes - Episode III

[Thomas Huth]

On 18.07.2016 17:18, Greg Kurz wrote:
> On Mon, 18 Jul 2016 15:19:04 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> After already fixing two issues with the huge page detection mechanism
>> (see commit 159d2e39a860 and 86b50f2e1bef), Greg Kurz noticed another
>> case that caused the guest to crash where QEMU announces huge pages
>> though they should not be available for the guest:
>>
>> qemu-system-ppc64 -enable-kvm ... -mem-path /dev/hugepages \
>>  -m 1G,slots=4,maxmem=32G
>>  -object memory-backend-ram,policy=default,size=1G,id=mem-mem1 \
>>  -device pc-dimm,id=dimm-mem1,memdev=mem-mem1 -smp 2 \
>>  -numa node,nodeid=0 -numa node,nodeid=1
>>
>> That means if there is a global mem-path option, we still have
>> to look at the memory-backend objects that have been specified
>> additionally and return their minimum page size if that value
>> is smaller than the page size of the main memory.
>>
>> Reported-by: Greg Kurz <groug@kaod.org>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
> 
> Just one remark, see below, but apart from that:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Tested-by: Greg Kurz <groug@kaod.org>
> 
>>  target-ppc/kvm.c | 27 ++++++++++++++-------------
>>  1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index 7a8f555..97ab450 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -366,10 +366,13 @@ static int find_max_supported_pagesize(Object *obj, void *opaque)
>>  static long getrampagesize(void)
>>  {
>>      long hpsize = LONG_MAX;
>> +    long mainrampagesize;
>>      Object *memdev_root;
>>  
>>      if (mem_path) {
>> -        return gethugepagesize(mem_path);
>> +        mainrampagesize = gethugepagesize(mem_path);
>> +    } else {
>> +        mainrampagesize = getpagesize();
>>      }
>>  
>>      /* it's possible we have memory-backend objects with
>> @@ -383,28 +386,26 @@ static long getrampagesize(void)
>>       * backend isn't backed by hugepages.
>>       */
>>      memdev_root = object_resolve_path("/objects", NULL);
>> -    if (!memdev_root) {
>> -        return getpagesize();
>> +    if (memdev_root) {
>> +        object_child_foreach(memdev_root, find_max_supported_pagesize, &hpsize);
>>      }
>> -
>> -    object_child_foreach(memdev_root, find_max_supported_pagesize, &hpsize);
>> -
>> -    if (hpsize == LONG_MAX || hpsize == getpagesize()) {
>> -        return getpagesize();
>> +    if (hpsize == LONG_MAX) {
>> +        /* No additional memory regions found ==> Report main RAM page size */
>> +        return mainrampagesize;
>>      }
>>  
>>      /* If NUMA is disabled or the NUMA nodes are not backed with a
>> -     * memory-backend, then there is at least one node using "normal"
>> -     * RAM. And since normal RAM has not been configured with "-mem-path"
>> -     * (what we've checked earlier here already), we can not use huge pages!
>> +     * memory-backend, then there is at least one node using "normal" RAM,
>> +     * so if its page size is smaller we have got to report that size instead.
>>       */
>> -    if (nb_numa_nodes == 0 || numa_info[0].node_memdev == NULL) {
>> +    if (hpsize > mainrampagesize &&
>> +        (nb_numa_nodes == 0 || numa_info[0].node_memdev == NULL)) {
>>          static bool warned;
>>          if (!warned) {
>>              error_report("Huge page support disabled (n/a for main memory).");
> 
> Maybe update the error message since we have another condition ?
> 
> Something like:
> 
> "Huge page support disabled (at least one numa uses standard page size)"

That sounds also a little bit confusing since the error message could
occur when there is no numa configured at all. I think refering to "main
memory" is better here so that the users have a chance to know that they
might need to specify the global "-mem-path" parameter here, too.

 Thomas

Re: [Qemu-devel] [PATCH] ppc: Huge page detection mechanism fixes - Episode III

[Greg Kurz]

On Tue, 19 Jul 2016 08:23:46 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 18.07.2016 17:18, Greg Kurz wrote:
> > On Mon, 18 Jul 2016 15:19:04 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> After already fixing two issues with the huge page detection mechanism
> >> (see commit 159d2e39a860 and 86b50f2e1bef), Greg Kurz noticed another
> >> case that caused the guest to crash where QEMU announces huge pages
> >> though they should not be available for the guest:
> >>
> >> qemu-system-ppc64 -enable-kvm ... -mem-path /dev/hugepages \
> >>  -m 1G,slots=4,maxmem=32G
> >>  -object memory-backend-ram,policy=default,size=1G,id=mem-mem1 \
> >>  -device pc-dimm,id=dimm-mem1,memdev=mem-mem1 -smp 2 \
> >>  -numa node,nodeid=0 -numa node,nodeid=1
> >>
> >> That means if there is a global mem-path option, we still have
> >> to look at the memory-backend objects that have been specified
> >> additionally and return their minimum page size if that value
> >> is smaller than the page size of the main memory.
> >>
> >> Reported-by: Greg Kurz <groug@kaod.org>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---  
> > 
> > Just one remark, see below, but apart from that:
> > 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> > Tested-by: Greg Kurz <groug@kaod.org>
> >   
> >>  target-ppc/kvm.c | 27 ++++++++++++++-------------
> >>  1 file changed, 14 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> >> index 7a8f555..97ab450 100644
> >> --- a/target-ppc/kvm.c
> >> +++ b/target-ppc/kvm.c
> >> @@ -366,10 +366,13 @@ static int find_max_supported_pagesize(Object *obj, void *opaque)
> >>  static long getrampagesize(void)
> >>  {
> >>      long hpsize = LONG_MAX;
> >> +    long mainrampagesize;
> >>      Object *memdev_root;
> >>  
> >>      if (mem_path) {
> >> -        return gethugepagesize(mem_path);
> >> +        mainrampagesize = gethugepagesize(mem_path);
> >> +    } else {
> >> +        mainrampagesize = getpagesize();
> >>      }
> >>  
> >>      /* it's possible we have memory-backend objects with
> >> @@ -383,28 +386,26 @@ static long getrampagesize(void)
> >>       * backend isn't backed by hugepages.
> >>       */
> >>      memdev_root = object_resolve_path("/objects", NULL);
> >> -    if (!memdev_root) {
> >> -        return getpagesize();
> >> +    if (memdev_root) {
> >> +        object_child_foreach(memdev_root, find_max_supported_pagesize, &hpsize);
> >>      }
> >> -
> >> -    object_child_foreach(memdev_root, find_max_supported_pagesize, &hpsize);
> >> -
> >> -    if (hpsize == LONG_MAX || hpsize == getpagesize()) {
> >> -        return getpagesize();
> >> +    if (hpsize == LONG_MAX) {
> >> +        /* No additional memory regions found ==> Report main RAM page size */
> >> +        return mainrampagesize;
> >>      }
> >>  
> >>      /* If NUMA is disabled or the NUMA nodes are not backed with a
> >> -     * memory-backend, then there is at least one node using "normal"
> >> -     * RAM. And since normal RAM has not been configured with "-mem-path"
> >> -     * (what we've checked earlier here already), we can not use huge pages!
> >> +     * memory-backend, then there is at least one node using "normal" RAM,
> >> +     * so if its page size is smaller we have got to report that size instead.
> >>       */
> >> -    if (nb_numa_nodes == 0 || numa_info[0].node_memdev == NULL) {
> >> +    if (hpsize > mainrampagesize &&
> >> +        (nb_numa_nodes == 0 || numa_info[0].node_memdev == NULL)) {
> >>          static bool warned;
> >>          if (!warned) {
> >>              error_report("Huge page support disabled (n/a for main memory).");  
> > 
> > Maybe update the error message since we have another condition ?
> > 
> > Something like:
> > 
> > "Huge page support disabled (at least one numa uses standard page size)"  
> 
> That sounds also a little bit confusing since the error message could
> occur when there is no numa configured at all. I think refering to "main
> memory" is better here so that the users have a chance to know that they
> might need to specify the global "-mem-path" parameter here, too.
> 
>  Thomas
> 

Fair enough. And anyway, David has already applied the patch.

Cheers.

--
Greg