diff mbox series

[RFC,08/21] ramblock: Cache the length to do file mmap() on ramblocks

Message ID 20230117220914.2062125-9-peterx@redhat.com (mailing list archive)
State New, archived
Headers show
Series migration: Support hugetlb doublemaps | expand

Commit Message

Peter Xu Jan. 17, 2023, 10:09 p.m. UTC
We do proper page size alignment for file backed mmap()s for ramblocks.
Even if it's as simple as that, cache the value because it'll be used in
multiple places.

Since at it, drop size for file_ram_alloc() and just use max_length because
that's always true for file-backed ramblocks.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/exec/ramblock.h |  2 ++
 softmmu/physmem.c       | 14 +++++++-------
 2 files changed, 9 insertions(+), 7 deletions(-)

Comments

Dr. David Alan Gilbert Jan. 23, 2023, 6:51 p.m. UTC | #1
* Peter Xu (peterx@redhat.com) wrote:
> We do proper page size alignment for file backed mmap()s for ramblocks.
> Even if it's as simple as that, cache the value because it'll be used in
> multiple places.
> 
> Since at it, drop size for file_ram_alloc() and just use max_length because
> that's always true for file-backed ramblocks.

Having a length previously called 'memory' was a bit odd!

> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  include/exec/ramblock.h |  2 ++
>  softmmu/physmem.c       | 14 +++++++-------
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
> index 76cd0812c8..3f31ce1591 100644
> --- a/include/exec/ramblock.h
> +++ b/include/exec/ramblock.h
> @@ -32,6 +32,8 @@ struct RAMBlock {
>      ram_addr_t offset;
>      ram_addr_t used_length;
>      ram_addr_t max_length;
> +    /* Only used for file-backed ramblocks */
> +    ram_addr_t mmap_length;
>      void (*resized)(const char*, uint64_t length, void *host);
>      uint32_t flags;
>      /* Protected by iothread lock.  */
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index aa1a7466e5..b5be02f1cb 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -1533,7 +1533,6 @@ static int file_ram_open(const char *path,
>  }
>  
>  static void *file_ram_alloc(RAMBlock *block,
> -                            ram_addr_t memory,
>                              int fd,
>                              bool readonly,
>                              bool truncate,
> @@ -1563,14 +1562,14 @@ static void *file_ram_alloc(RAMBlock *block,
>      }
>  #endif
>  
> -    if (memory < block->page_size) {
> +    if (block->max_length < block->page_size) {
>          error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
>                     "or larger than page size 0x%zx",
> -                   memory, block->page_size);
> +                   block->max_length, block->page_size);
>          return NULL;
>      }
>  
> -    memory = ROUND_UP(memory, block->page_size);
> +    block->mmap_length = ROUND_UP(block->max_length, block->page_size);
>  
>      /*
>       * ftruncate is not supported by hugetlbfs in older
> @@ -1586,7 +1585,7 @@ static void *file_ram_alloc(RAMBlock *block,
>       * those labels. Therefore, extending the non-empty backend file
>       * is disabled as well.
>       */
> -    if (truncate && ftruncate(fd, memory)) {
> +    if (truncate && ftruncate(fd, block->mmap_length)) {
>          perror("ftruncate");
>      }
>  
> @@ -1594,7 +1593,8 @@ static void *file_ram_alloc(RAMBlock *block,
>      qemu_map_flags |= (block->flags & RAM_SHARED) ? QEMU_MAP_SHARED : 0;
>      qemu_map_flags |= (block->flags & RAM_PMEM) ? QEMU_MAP_SYNC : 0;
>      qemu_map_flags |= (block->flags & RAM_NORESERVE) ? QEMU_MAP_NORESERVE : 0;
> -    area = qemu_ram_mmap(fd, memory, block->mr->align, qemu_map_flags, offset);
> +    area = qemu_ram_mmap(fd, block->mmap_length, block->mr->align,
> +                         qemu_map_flags, offset);
>      if (area == MAP_FAILED) {
>          error_setg_errno(errp, errno,
>                           "unable to map backing store for guest RAM");
> @@ -2100,7 +2100,7 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
>      new_block->used_length = size;
>      new_block->max_length = size;
>      new_block->flags = ram_flags;
> -    new_block->host = file_ram_alloc(new_block, size, fd, readonly,
> +    new_block->host = file_ram_alloc(new_block, fd, readonly,
>                                       !file_size, offset, errp);
>      if (!new_block->host) {
>          g_free(new_block);
> -- 
> 2.37.3
>
Peter Xu Jan. 24, 2023, 8:28 p.m. UTC | #2
On Mon, Jan 23, 2023 at 06:51:51PM +0000, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > We do proper page size alignment for file backed mmap()s for ramblocks.
> > Even if it's as simple as that, cache the value because it'll be used in
> > multiple places.
> > 
> > Since at it, drop size for file_ram_alloc() and just use max_length because
> > that's always true for file-backed ramblocks.
> 
> Having a length previously called 'memory' was a bit odd!

:-D

> 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Thanks,
Juan Quintela Jan. 30, 2023, 5:05 a.m. UTC | #3
Peter Xu <peterx@redhat.com> wrote:
> We do proper page size alignment for file backed mmap()s for ramblocks.
> Even if it's as simple as that, cache the value because it'll be used in
> multiple places.
>
> Since at it, drop size for file_ram_alloc() and just use max_length because
> that's always true for file-backed ramblocks.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>


> @@ -2100,7 +2100,7 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
>      new_block->used_length = size;
>      new_block->max_length = size;
>      new_block->flags = ram_flags;
> -    new_block->host = file_ram_alloc(new_block, size, fd, readonly,
> +    new_block->host = file_ram_alloc(new_block, fd, readonly,
>                                       !file_size, offset, errp);
>      if (!new_block->host) {
>          g_free(new_block);

Passing "size" in three places, not bad at all O:-)
Peter Xu Jan. 30, 2023, 10:07 p.m. UTC | #4
On Mon, Jan 30, 2023 at 06:05:47AM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > We do proper page size alignment for file backed mmap()s for ramblocks.
> > Even if it's as simple as that, cache the value because it'll be used in
> > multiple places.
> >
> > Since at it, drop size for file_ram_alloc() and just use max_length because
> > that's always true for file-backed ramblocks.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Thanks for reviewing the set!

> 
> > @@ -2100,7 +2100,7 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
> >      new_block->used_length = size;
> >      new_block->max_length = size;
> >      new_block->flags = ram_flags;
> > -    new_block->host = file_ram_alloc(new_block, size, fd, readonly,
> > +    new_block->host = file_ram_alloc(new_block, fd, readonly,
> >                                       !file_size, offset, errp);
> >      if (!new_block->host) {
> >          g_free(new_block);
> 
> Passing "size" in three places, not bad at all O:-)

Yes it's a bit unfortunate. :(
diff mbox series

Patch

diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
index 76cd0812c8..3f31ce1591 100644
--- a/include/exec/ramblock.h
+++ b/include/exec/ramblock.h
@@ -32,6 +32,8 @@  struct RAMBlock {
     ram_addr_t offset;
     ram_addr_t used_length;
     ram_addr_t max_length;
+    /* Only used for file-backed ramblocks */
+    ram_addr_t mmap_length;
     void (*resized)(const char*, uint64_t length, void *host);
     uint32_t flags;
     /* Protected by iothread lock.  */
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index aa1a7466e5..b5be02f1cb 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -1533,7 +1533,6 @@  static int file_ram_open(const char *path,
 }
 
 static void *file_ram_alloc(RAMBlock *block,
-                            ram_addr_t memory,
                             int fd,
                             bool readonly,
                             bool truncate,
@@ -1563,14 +1562,14 @@  static void *file_ram_alloc(RAMBlock *block,
     }
 #endif
 
-    if (memory < block->page_size) {
+    if (block->max_length < block->page_size) {
         error_setg(errp, "memory size 0x" RAM_ADDR_FMT " must be equal to "
                    "or larger than page size 0x%zx",
-                   memory, block->page_size);
+                   block->max_length, block->page_size);
         return NULL;
     }
 
-    memory = ROUND_UP(memory, block->page_size);
+    block->mmap_length = ROUND_UP(block->max_length, block->page_size);
 
     /*
      * ftruncate is not supported by hugetlbfs in older
@@ -1586,7 +1585,7 @@  static void *file_ram_alloc(RAMBlock *block,
      * those labels. Therefore, extending the non-empty backend file
      * is disabled as well.
      */
-    if (truncate && ftruncate(fd, memory)) {
+    if (truncate && ftruncate(fd, block->mmap_length)) {
         perror("ftruncate");
     }
 
@@ -1594,7 +1593,8 @@  static void *file_ram_alloc(RAMBlock *block,
     qemu_map_flags |= (block->flags & RAM_SHARED) ? QEMU_MAP_SHARED : 0;
     qemu_map_flags |= (block->flags & RAM_PMEM) ? QEMU_MAP_SYNC : 0;
     qemu_map_flags |= (block->flags & RAM_NORESERVE) ? QEMU_MAP_NORESERVE : 0;
-    area = qemu_ram_mmap(fd, memory, block->mr->align, qemu_map_flags, offset);
+    area = qemu_ram_mmap(fd, block->mmap_length, block->mr->align,
+                         qemu_map_flags, offset);
     if (area == MAP_FAILED) {
         error_setg_errno(errp, errno,
                          "unable to map backing store for guest RAM");
@@ -2100,7 +2100,7 @@  RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
     new_block->used_length = size;
     new_block->max_length = size;
     new_block->flags = ram_flags;
-    new_block->host = file_ram_alloc(new_block, size, fd, readonly,
+    new_block->host = file_ram_alloc(new_block, fd, readonly,
                                      !file_size, offset, errp);
     if (!new_block->host) {
         g_free(new_block);