Re: [PATCH v2 fixed 08/16] util/mmap-alloc: Factor out calculation of pagesize to mmap_pagesize()

[Peter Xu <peterx@redhat.com>]

On Wed, Feb 12, 2020 at 02:42:46PM +0100, David Hildenbrand wrote:
> Factor it out and add a comment.
> 
> Reviewed-by: Igor Kotrasinski <i.kotrasinsk@partner.samsung.com>
> Acked-by: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
> Cc: Greg Kurz <groug@kaod.org>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  util/mmap-alloc.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> index 27dcccd8ec..82f02a2cec 100644
> --- a/util/mmap-alloc.c
> +++ b/util/mmap-alloc.c
> @@ -82,17 +82,27 @@ size_t qemu_mempath_getpagesize(const char *mem_path)
>      return qemu_real_host_page_size;
>  }
>  
> +static inline size_t mmap_pagesize(int fd)
> +{
> +#if defined(__powerpc64__) && defined(__linux__)
> +    /* Mappings in the same segment must share the same page size */
> +    return qemu_fd_getpagesize(fd);
> +#else
> +    return qemu_real_host_page_size;
> +#endif
> +}

Pure question: This will return 4K even for huge pages on x86, is this
what we want?

This is of course not related to this specific patch which still
follows the old code, but I'm thinking whether it was intended or not
even in the old code (or is there anything to do with the
MAP_NORESERVE fix for ppc64 huge pages?).  Do you know the answer?

Thanks,

> +
>  void *qemu_ram_mmap(int fd,
>                      size_t size,
>                      size_t align,
>                      bool shared,
>                      bool is_pmem)
>  {
> +    const size_t pagesize = mmap_pagesize(fd);
>      int flags;
>      int map_sync_flags = 0;
>      int guardfd;
>      size_t offset;
> -    size_t pagesize;
>      size_t total;
>      void *guardptr;
>      void *ptr;
> @@ -113,7 +123,6 @@ void *qemu_ram_mmap(int fd,
>       * anonymous memory is OK.
>       */
>      flags = MAP_PRIVATE;
> -    pagesize = qemu_fd_getpagesize(fd);
>      if (fd == -1 || pagesize == qemu_real_host_page_size) {
>          guardfd = -1;
>          flags |= MAP_ANONYMOUS;
> @@ -123,7 +132,6 @@ void *qemu_ram_mmap(int fd,
>      }
>  #else
>      guardfd = -1;
> -    pagesize = qemu_real_host_page_size;
>      flags = MAP_PRIVATE | MAP_ANONYMOUS;
>  #endif
>  
> @@ -198,15 +206,10 @@ void *qemu_ram_mmap(int fd,
>  
>  void qemu_ram_munmap(int fd, void *ptr, size_t size)
>  {
> -    size_t pagesize;
> +    const size_t pagesize = mmap_pagesize(fd);
>  
>      if (ptr) {
>          /* Unmap both the RAM block and the guard page */
> -#if defined(__powerpc64__) && defined(__linux__)
> -        pagesize = qemu_fd_getpagesize(fd);
> -#else
> -        pagesize = qemu_real_host_page_size;
> -#endif
>          munmap(ptr, size + pagesize);
>      }
>  }
> -- 
> 2.24.1
> 
> 

-- 
Peter Xu