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

["Murilo Opsfelder Araújo" <muriloo@linux.ibm.com>]

On Monday, February 24, 2020 11:16:16 AM -03 Murilo Opsfelder Araújo wrote:
> On Monday, February 24, 2020 7:57:03 AM -03 David Hildenbrand wrote:
> > On 24.02.20 11:50, David Hildenbrand wrote:
> > > On 19.02.20 23:46, Peter Xu wrote:
> > >> 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?
> > >
> > > (was asking myself the same question) I *think* it's intended. It's
> > > mainly only used to allocate one additional guard page. The callers of
> > > qemu_ram_mmap() make sure that the size is properly aligned (e.g., to
> > > huge pages).
> > >
> > > Of course, a 4k guard page is sufficient - unless we can't use that
> > > (special case for ppc64 here).
> > >
> > > Thanks!
> >
> > We could rename the function to mmap_guard_pagesize(), thoughts?
>
> The existing qemu_fd_getpagesize() already returns qemu_real_host_page_size
> for non-anonymous mappings (when fd == -1).  I think this new
> mmap_pagesize() could be dropped in favor of qemu_fd_getpagesize().

s/non-//

I mean "for anonymous mappings".

>
> A side effect of this change would be guard page using a bit more memory for
> non-anonymous mapping.  Could that be a problem?
>
> What do you think?
>
> --
> Murilo


--
Murilo