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

[David Hildenbrand]

On 24.02.20 15:25, Murilo Opsfelder Araújo wrote:
> 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 <address@hidden>
>>>>>> Acked-by: Murilo Opsfelder Araujo <address@hidden>
>>>>>> Reviewed-by: Richard Henderson <address@hidden>
>>>>>> Cc: "Michael S. Tsirkin" <address@hidden>
>>>>>> Cc: Murilo Opsfelder Araujo <address@hidden>
>>>>>> Cc: Greg Kurz <address@hidden>
>>>>>> Cc: Eduardo Habkost <address@hidden>
>>>>>> Cc: "Dr. David Alan Gilbert" <address@hidden>
>>>>>> Cc: Igor Mammedov <address@hidden>
>>>>>> Signed-off-by: David Hildenbrand <address@hidden>
>>>>>> ---
>>>>>>
>>>>>>  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?

At least under Linux it won't be an issue I guess. The same is probably
true for other OSs as well - the memory is not even readable, so why
should it consume memory. So it will only consume space in the virtual
address space.

Anyone reading along wants to object?

-- 
Thanks,

David / dhildenb