Re: [PATCH v1 08/13] util/mmap-alloc: Prepare for resizable mmaps

[David Hildenbrand <david@redhat.com>]

On 06.02.20 00:00, Murilo Opsfelder Araújo wrote:
> Hello, David.
> 
> On Monday, February 3, 2020 3:31:20 PM -03 David Hildenbrand wrote:
>> When shrinking a mmap we want to re-reserve the already populated area.
>> When growing a memory region, we want to populate starting with a given
>> fd_offset. Prepare by allowing to pass these parameters.
>>
>> Also, let's make sure we always process full pages, to avoid
>> unmapping/remapping pages that are already in use when
>> growing/shrinking. (existing callers seem to guarantee this, but that's
>> not obvious)
>>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Greg Kurz <groug@kaod.org>
>> Cc: Murilo Opsfelder Araujo <muriloo@linux.ibm.com>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  util/mmap-alloc.c | 32 +++++++++++++++++++++-----------
>>  1 file changed, 21 insertions(+), 11 deletions(-)
>>
>> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
>> index f043ccb0ab..63ad6893b7 100644
>> --- a/util/mmap-alloc.c
>> +++ b/util/mmap-alloc.c
>> @@ -83,12 +83,12 @@ size_t qemu_mempath_getpagesize(const char *mem_path)
>>  }
>>
>>  /*
>> - * Reserve a new memory region of the requested size to be used for mapping
>> - * from the given fd (if any).
>> + * Reserve a new memory region of the requested size or re-reserve parts
>> + * of an existing region to be used for mapping from the given fd (if any).
>> */
>> -static void *mmap_reserve(size_t size, int fd)
>> +static void *mmap_reserve(void *ptr, size_t size, int fd)
>>  {
>> -    int flags = MAP_PRIVATE;
>> +    int flags = MAP_PRIVATE | (ptr ? MAP_FIXED : 0);
>>
>>  #if defined(__powerpc64__) && defined(__linux__)
>>      /*
>> @@ -111,19 +111,23 @@ static void *mmap_reserve(size_t size, int fd)
>>      flags |= MAP_ANONYMOUS;
>>  #endif
>>
>> -    return mmap(0, size, PROT_NONE, flags, fd, 0);
>> +    return mmap(ptr, size, PROT_NONE, flags, fd, 0);
>>  }
>>
>>  /*
>>   * Populate memory in a reserved region from the given fd (if any).
>>   */
>> -static void *mmap_populate(void *ptr, size_t size, int fd, bool shared,
>> -                           bool is_pmem)
>> +static void *mmap_populate(void *ptr, size_t size, int fd, size_t
>> fd_offset, +                           bool shared, bool is_pmem)
>>  {
>>      int map_sync_flags = 0;
>>      int flags = MAP_FIXED;
>>      void *new_ptr;
>>
>> +    if (fd == -1) {
>> +        fd_offset = 0;
>> +    }
>> +
>>      flags |= fd == -1 ? MAP_ANONYMOUS : 0;
>>      flags |= shared ? MAP_SHARED : MAP_PRIVATE;
>>      if (shared && is_pmem) {
>> @@ -131,7 +135,7 @@ static void *mmap_populate(void *ptr, size_t size, int
>> fd, bool shared, }
>>
>>      new_ptr = mmap(ptr, size, PROT_READ | PROT_WRITE, flags |
>> map_sync_flags, -                   fd, 0);
>> +                   fd, fd_offset);
>>      if (new_ptr == MAP_FAILED && map_sync_flags) {
>>          if (errno == ENOTSUP) {
>>              char *proc_link = g_strdup_printf("/proc/self/fd/%d", fd);
>> @@ -153,7 +157,7 @@ static void *mmap_populate(void *ptr, size_t size, int
>> fd, bool shared, * If mmap failed with MAP_SHARED_VALIDATE | MAP_SYNC, we
>> will try * again without these flags to handle backwards compatibility. */
>> -        new_ptr = mmap(ptr, size, PROT_READ | PROT_WRITE, flags, fd, 0);
>> +        new_ptr = mmap(ptr, size, PROT_READ | PROT_WRITE, flags, fd,
>> fd_offset); }
>>      return new_ptr;
>>  }
>> @@ -178,13 +182,16 @@ void *qemu_ram_mmap(int fd,
>>      size_t offset, total;
>>      void *ptr, *guardptr;
>>
>> +    /* we can only map whole pages */
>> +    size = QEMU_ALIGN_UP(size, pagesize);
>> +
> 
> Caller already rounds up size to block->page_size.
> 
> Why this QEMU_ALIGN_UP is necessary?
> 
>>      /*
>>       * Note: this always allocates at least one extra page of virtual
>> address * space, even if size is already aligned.
>>       */
>>      total = size + align;
> 
> If size was aligned above with pagesize boundary, why would this align be 
> necessary?
> 
> Can the pagesize differ from memory region align?

Sorry, skipped this comment.

Yes, e.g., we want to align ram blocks for KVM to hugepage size, to
allow for transparent huge pages. So the comment still holds.


-- 
Thanks,

David / dhildenb