virfile: relax checks for hugetlbfs

[libvirt] [PATCH] virfile: relax checks for hugetlbfs

Posted by David Vrabel 6 years, 4 months ago

There are use cases where it is useful to use the support in libvirt
for file-backed guest memory, but without using hugetlbfs but tmpfs
instead (for example, to run tests on hosts that do not have hugepages
configured, or to use Linux's idle page tracking to monitor guest
memory accesses at a 4k page granularity.).

Drop the check for hugetlbfs when querying the huge page size, but
move it to the loop that's searching for a suitable mount to use.

Change-Id: I2c9589191e14653724725b944171689553ee6bae
---
 src/util/virfile.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 82cb36dbc..24ff5e208 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -3438,19 +3438,23 @@ virFileGetHugepageSize(const char *path,
         goto cleanup;
     }
 
-    if (fs.f_type != HUGETLBFS_MAGIC) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("not a hugetlbfs mount: '%s'"),
-                       path);
-        goto cleanup;
-    }
-
     *size = fs.f_bsize / 1024; /* we are storing size in KiB */
     ret = 0;
  cleanup:
     return ret;
 }
 
+static bool
+virFileIsHugeTLBFS(const char *path)
+{
+    struct statfs fs;
+
+    if (statfs(path, &fs) < 0) {
+        return false;
+    }
+    return fs.f_type == HUGETLBFS_MAGIC;
+}
+
 # define PROC_MEMINFO "/proc/meminfo"
 # define HUGEPAGESIZE_STR "Hugepagesize:"
 
@@ -3517,6 +3521,9 @@ virFileFindHugeTLBFS(virHugeTLBFSPtr *ret_fs,
         if (STRNEQ(mb.mnt_type, "hugetlbfs"))
             continue;
 
+        if (!virFileIsHugeTLBFS(mb.mnt_dir))
+            continue;
+
         if (VIR_EXPAND_N(fs, nfs, 1) < 0)
              goto cleanup;
 
-- 
2.11.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virfile: relax checks for hugetlbfs

Posted by Michal Privoznik 6 years, 4 months ago

On 12/04/2017 04:21 PM, David Vrabel wrote:
> There are use cases where it is useful to use the support in libvirt
> for file-backed guest memory, but without using hugetlbfs but tmpfs
> instead (for example, to run tests on hosts that do not have hugepages
> configured, or to use Linux's idle page tracking to monitor guest
> memory accesses at a 4k page granularity.).
> 
> Drop the check for hugetlbfs when querying the huge page size, but
> move it to the loop that's searching for a suitable mount to use.
> 
> Change-Id: I2c9589191e14653724725b944171689553ee6bae
> ---
>  src/util/virfile.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> > diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 82cb36dbc..24ff5e208 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -3438,19 +3438,23 @@ virFileGetHugepageSize(const char *path,
>          goto cleanup;
>      }
>  
> -    if (fs.f_type != HUGETLBFS_MAGIC) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("not a hugetlbfs mount: '%s'"),
> -                       path);
> -        goto cleanup;
> -    }
> -
>      *size = fs.f_bsize / 1024; /* we are storing size in KiB */
>      ret = 0;
>   cleanup:
>      return ret;
>  }
>  
> +static bool
> +virFileIsHugeTLBFS(const char *path)
> +{
> +    struct statfs fs;
> +
> +    if (statfs(path, &fs) < 0) {
> +        return false;
> +    }


Note that we don't put curly braces around one line bodies.

> +    return fs.f_type == HUGETLBFS_MAGIC;
> +}
> +
>  # define PROC_MEMINFO "/proc/meminfo"
>  # define HUGEPAGESIZE_STR "Hugepagesize:"
>  
> @@ -3517,6 +3521,9 @@ virFileFindHugeTLBFS(virHugeTLBFSPtr *ret_fs,
>          if (STRNEQ(mb.mnt_type, "hugetlbfs"))
>              continue;
>  
> +        if (!virFileIsHugeTLBFS(mb.mnt_dir))
> +            continue;
> +
>          if (VIR_EXPAND_N(fs, nfs, 1) < 0)
>               goto cleanup;
>  
> 

Frankly, I'm not a fan of this patch. Currently, when people have
<hugepages/> in their domain XML we guarantee that hugepages are used to
back the guest memory. With this patch users might be mislead as it
enables *any* other filesystem. All that's needed is to put mount points
into qemu.conf. I know you want it for testing, but is it an upstream
material? IOW, I'm not nacking this, I just want some conversation to
happen first.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] virfile: relax checks for hugetlbfs

Posted by David Vrabel 6 years, 4 months ago

On 06/12/17 14:55, Michal Privoznik wrote:
> On 12/04/2017 04:21 PM, David Vrabel wrote:
>> There are use cases where it is useful to use the support in libvirt
>> for file-backed guest memory, but without using hugetlbfs but tmpfs
>> instead (for example, to run tests on hosts that do not have hugepages
>> configured, or to use Linux's idle page tracking to monitor guest
>> memory accesses at a 4k page granularity.).
>>
>> Drop the check for hugetlbfs when querying the huge page size, but
>> move it to the loop that's searching for a suitable mount to use.
>>
>> Change-Id: I2c9589191e14653724725b944171689553ee6bae
>> ---
>>  src/util/virfile.c | 21 ++++++++++++++-------
>>  1 file changed, 14 insertions(+), 7 deletions(-)
>>> diff --git a/src/util/virfile.c b/src/util/virfile.c
>> index 82cb36dbc..24ff5e208 100644
>> --- a/src/util/virfile.c
>> +++ b/src/util/virfile.c
>> @@ -3438,19 +3438,23 @@ virFileGetHugepageSize(const char *path,
>>          goto cleanup;
>>      }
>>  
>> -    if (fs.f_type != HUGETLBFS_MAGIC) {
>> -        virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                       _("not a hugetlbfs mount: '%s'"),
>> -                       path);
>> -        goto cleanup;
>> -    }
>> -
>>      *size = fs.f_bsize / 1024; /* we are storing size in KiB */
>>      ret = 0;
>>   cleanup:
>>      return ret;
>>  }
>>  
>> +static bool
>> +virFileIsHugeTLBFS(const char *path)
>> +{
>> +    struct statfs fs;
>> +
>> +    if (statfs(path, &fs) < 0) {
>> +        return false;
>> +    }
> 
> 
> Note that we don't put curly braces around one line bodies.
> 
>> +    return fs.f_type == HUGETLBFS_MAGIC;
>> +}
>> +
>>  # define PROC_MEMINFO "/proc/meminfo"
>>  # define HUGEPAGESIZE_STR "Hugepagesize:"
>>  
>> @@ -3517,6 +3521,9 @@ virFileFindHugeTLBFS(virHugeTLBFSPtr *ret_fs,
>>          if (STRNEQ(mb.mnt_type, "hugetlbfs"))
>>              continue;
>>  
>> +        if (!virFileIsHugeTLBFS(mb.mnt_dir))
>> +            continue;
>> +
>>          if (VIR_EXPAND_N(fs, nfs, 1) < 0)
>>               goto cleanup;
>>  
>>
> 
> Frankly, I'm not a fan of this patch. Currently, when people have
> <hugepages/> in their domain XML we guarantee that hugepages are used to
> back the guest memory. With this patch users might be mislead as it
> enables *any* other filesystem. All that's needed is to put mount points
> into qemu.conf. I know you want it for testing, but is it an upstream
> material? IOW, I'm not nacking this, I just want some conversation to
> happen first.

I think this is a fair point.  If a system inadvertently loses its
/dev/hugepages mount, the VMs shouldn't start memory backed by the
filesystem in /dev.  This could result in hard-to-diagnose problems.

Not sure I'll be able to find time to rework this patch to add new
configuration options, though.

David

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list