Re: [PATCH RFC 08/21] ramblock: Cache the length to do file mmap() on ramblocks
On Mon, Jan 30, 2023 at 06:05:47AM +0100, Juan Quintela wrote:
> Peter Xu 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
>
> Reviewed-by: Juan Quintela
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. :(
--
Peter Xu
Re: [PATCH RFC 08/21] ramblock: Cache the length to do file mmap() on ramblocks
Peter Xu 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
Reviewed-by: Juan Quintela
> @@ -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:-)
Re: [PATCH RFC 08/21] ramblock: Cache the length to do file mmap() on ramblocks
On Mon, Jan 23, 2023 at 06:51:51PM +, Dr. David Alan Gilbert wrote: > * Peter Xu (pet...@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 > > Reviewed-by: Dr. David Alan Gilbert Thanks, -- Peter Xu
Re: [PATCH RFC 08/21] ramblock: Cache the length to do file mmap() on ramblocks
* Peter Xu (pet...@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
Reviewed-by: Dr. David Alan Gilbert
> ---
> 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
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[PATCH RFC 08/21] ramblock: Cache the length to do file mmap() on ramblocks
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
---
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
