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 <address@hidden> 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 <address@hidden>
>> Signed-off-by: Thomas Huth <address@hidden>
>> ---
> 
> Just one remark, see below, but apart from that:
> 
> Reviewed-by: Greg Kurz <address@hidden>
> Tested-by: Greg Kurz <address@hidden>
> 
>>  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