Re: [PATCH v7 07/35] util: introduce qemu_file_get_page_size() - Vladimir Sementsov-Ogievskiy

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
To: Xiao Guangrong <guangrong.xiao@linux.intel.com>,
	<pbonzini@redhat.com>, <imammedo@redhat.com>
Cc: <gleb@kernel.org>, <mtosatti@redhat.com>, <stefanha@redhat.com>,
	<mst@redhat.com>, <rth@twiddle.net>, <ehabkost@redhat.com>,
	<dan.j.williams@intel.com>, <kvm@vger.kernel.org>,
	<qemu-devel@nongnu.org>, <eblake@redhat.com>
Subject: Re: [PATCH v7 07/35] util: introduce qemu_file_get_page_size()
Date: Mon, 2 Nov 2015 16:56:35 +0300	[thread overview]
Message-ID: <56376B93.3080104@virtuozzo.com> (raw)
In-Reply-To: <1446455617-129562-8-git-send-email-guangrong.xiao@linux.intel.com>

On 02.11.2015 12:13, Xiao Guangrong wrote:
> There are three places use the some logic to get the page size on
> the file path or file fd
>
> Windows did not support file hugepage, so it will return normal page
> for this case. And this interface has not been used on windows so far
>   
> This patch introduces qemu_file_get_page_size() to unify the code
>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
>   exec.c               | 33 ++++++---------------------------
>   include/qemu/osdep.h |  1 +
>   target-ppc/kvm.c     | 23 +++++------------------
>   util/oslib-posix.c   | 37 +++++++++++++++++++++++++++++++++----
>   util/oslib-win32.c   |  5 +++++
>   5 files changed, 50 insertions(+), 49 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 8af2570..9de38be 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1174,32 +1174,6 @@ void qemu_mutex_unlock_ramlist(void)
>   }
>   
>   #ifdef __linux__
> -
> -#include <sys/vfs.h>
> -
> -#define HUGETLBFS_MAGIC       0x958458f6
> -
> -static long gethugepagesize(const char *path, Error **errp)
> -{
> -    struct statfs fs;
> -    int ret;
> -
> -    do {
> -        ret = statfs(path, &fs);
> -    } while (ret != 0 && errno == EINTR);
> -
> -    if (ret != 0) {
> -        error_setg_errno(errp, errno, "failed to get page size of file %s",
> -                         path);
> -        return 0;
> -    }
> -
> -    if (fs.f_type != HUGETLBFS_MAGIC)
> -        fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path);
> -
> -    return fs.f_bsize;
> -}
> -
>   static void *file_ram_alloc(RAMBlock *block,
>                               ram_addr_t memory,
>                               const char *path,
> @@ -1213,11 +1187,16 @@ static void *file_ram_alloc(RAMBlock *block,
>       uint64_t hpagesize;
>       Error *local_err = NULL;
>   
> -    hpagesize = gethugepagesize(path, &local_err);
> +    hpagesize = qemu_file_get_page_size(path, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
>           goto error;
>       }
> +
> +    if (hpagesize == getpagesize()) {
> +        fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path);
> +    }
> +
>       block->mr->align = hpagesize;
>   
>       if (memory < hpagesize) {
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index b568424..dbc17dc 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -302,4 +302,5 @@ int qemu_read_password(char *buf, int buf_size);
>    */
>   pid_t qemu_fork(Error **errp);
>   
> +size_t qemu_file_get_page_size(const char *mem_path, Error **errp);
>   #endif
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index ac70f08..d8760ea 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -308,28 +308,15 @@ static void kvm_get_smmu_info(PowerPCCPU *cpu, struct kvm_ppc_smmu_info *info)
>   
>   static long gethugepagesize(const char *mem_path)
>   {
> -    struct statfs fs;
> -    int ret;
> -
> -    do {
> -        ret = statfs(mem_path, &fs);
> -    } while (ret != 0 && errno == EINTR);
> +    Error *local_err = NULL;
> +    long size = qemu_file_get_page_size(mem_path, local_err);
>   
> -    if (ret != 0) {
> -        fprintf(stderr, "Couldn't statfs() memory path: %s\n",
> -                strerror(errno));
> +    if (local_err) {
> +        error_report_err(local_err);
>           exit(1);
>       }
>   
> -#define HUGETLBFS_MAGIC       0x958458f6
> -
> -    if (fs.f_type != HUGETLBFS_MAGIC) {
> -        /* Explicit mempath, but it's ordinary pages */
> -        return getpagesize();
> -    }
> -
> -    /* It's hugepage, return the huge page size */
> -    return fs.f_bsize;
> +    return size;
>   }
>   
>   static int find_max_supported_pagesize(Object *obj, void *opaque)
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 914cef5..51437ff 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -340,7 +340,7 @@ static void sigbus_handler(int signal)
>       siglongjmp(sigjump, 1);
>   }
>   
> -static size_t fd_getpagesize(int fd)
> +static size_t fd_getpagesize(int fd, Error **errp)
>   {
>   #ifdef CONFIG_LINUX
>       struct statfs fs;
> @@ -351,7 +351,12 @@ static size_t fd_getpagesize(int fd)
>               ret = fstatfs(fd, &fs);
>           } while (ret != 0 && errno == EINTR);
>   
> -        if (ret == 0 && fs.f_type == HUGETLBFS_MAGIC) {
> +        if (ret) {
> +            error_setg_errno(errp, errno, "fstatfs is failed");
> +            return 0;
> +        }
> +
> +        if (fs.f_type == HUGETLBFS_MAGIC) {
>               return fs.f_bsize;
>           }
>       }
> @@ -360,6 +365,22 @@ static size_t fd_getpagesize(int fd)
>       return getpagesize();
>   }
>   
> +size_t qemu_file_get_page_size(const char *path, Error **errp)
> +{
> +    size_t size = 0;
> +    int fd = qemu_open(path, O_RDONLY);
> +
> +    if (fd < 0) {
> +        error_setg_file_open(errp, errno, path);
> +        goto exit;
> +    }
> +
> +    size = fd_getpagesize(fd, errp);
> +    qemu_close(fd);
> +exit:
> +    return size;
> +}
> +
>   void os_mem_prealloc(int fd, char *area, size_t memory)
>   {
>       int ret;
> @@ -387,8 +408,16 @@ void os_mem_prealloc(int fd, char *area, size_t memory)
>           exit(1);
>       } else {
>           int i;
> -        size_t hpagesize = fd_getpagesize(fd);
> -        size_t numpages = DIV_ROUND_UP(memory, hpagesize);
> +        Error *local_err = NULL;
> +        size_t hpagesize = fd_getpagesize(fd, &local_err);
> +        size_t numpages;
> +
> +        if (local_err) {
> +            error_report_err(local_err);
> +            exit(1);
> +        }
> +
> +        numpages = DIV_ROUND_UP(memory, hpagesize);
>   
>           /* MAP_POPULATE silently ignores failures */
>           for (i = 0; i < numpages; i++) {
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index 09f9e98..dada6b6 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -462,6 +462,11 @@ size_t getpagesize(void)
>       return system_info.dwPageSize;
>   }
>   
> +size_t qemu_file_get_page_size(const char *path, Error **errp)
> +{
> +    return getpagesize();
> +}
> +
>   void os_mem_prealloc(int fd, char *area, size_t memory)
>   {
>       int i;

Ok for me. The only thing: some functions about pagesize return size_t, 
when others return long. long is more common practice here, but this 
doesn't really matter, so with or without size_t <-> long changes:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.