Re: [PATCH V1 13/26] physmem: ram_block_create

[Steven Sistare]

On 5/13/2024 2:37 PM, Fabiano Rosas wrote:
> Steve Sistare <steven.sistare@oracle.com> writes:
>
> > Create a common subroutine to allocate a RAMBlock, de-duping the code to
> > populate its common fields.  Add a trace point for good measure.
> > No functional change.
> >
> > Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> > ---
> >   system/physmem.c    | 47 ++++++++++++++++++++++++++---------------------
> >   system/trace-events |  3 +++
> >   2 files changed, 29 insertions(+), 21 deletions(-)
> >
> > diff --git a/system/physmem.c b/system/physmem.c
> > index c3d04ca..6216b14 100644
> > --- a/system/physmem.c
> > +++ b/system/physmem.c
> > @@ -52,6 +52,7 @@
> >   #include "sysemu/hw_accel.h"
> >   #include "sysemu/xen-mapcache.h"
> >   #include "trace/trace-root.h"
> > +#include "trace.h"
> >   
> >   #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> >   #include <linux/falloc.h>
> > @@ -1918,11 +1919,29 @@ out_free:
> >       }
> >   }
> >   
> > +static RAMBlock *ram_block_create(MemoryRegion *mr, ram_addr_t size,
> > +                                  ram_addr_t max_size, uint32_t ram_flags)
> > +{
> > +    RAMBlock *rb = g_malloc0(sizeof(*rb));
> > +
> > +    rb->used_length = size;
> > +    rb->max_length = max_size;
> > +    rb->fd = -1;
> > +    rb->flags = ram_flags;
> > +    rb->page_size = qemu_real_host_page_size();
> > +    rb->mr = mr;
> > +    rb->guest_memfd = -1;
> > +    trace_ram_block_create(rb->idstr, rb->flags, rb->fd, rb->used_length,
>
> There's no idstr at this point, is there? I think this needs to be
> memory_region_name(mr).

Thanks, will fix. That is a bug in my patch factoring.  I add the call to
qemu_ram_set_idstr in patch "physmem: set ram block idstr earlier".

- Steve

> > +                           rb->max_length, mr->align);
> > +    return rb;
> > +}
> > +
> >   #ifdef CONFIG_POSIX
> >   RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
> >                                    uint32_t ram_flags, int fd, off_t offset,
> >                                    Error **errp)
> >   {
> > +    void *host;
> >       RAMBlock *new_block;
> >       Error *local_err = NULL;
> >       int64_t file_size, file_align;
> > @@ -1962,19 +1981,14 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, 
> > MemoryRegion *mr,
> >           return NULL;
> >       }
> >   
> > -    new_block = g_malloc0(sizeof(*new_block));
> > -    new_block->mr = mr;
> > -    new_block->used_length = size;
> > -    new_block->max_length = size;
> > -    new_block->flags = ram_flags;
> > -    new_block->guest_memfd = -1;
> > -    new_block->host = file_ram_alloc(new_block, size, fd, !file_size, offset,
> > -                                     errp);
> > -    if (!new_block->host) {
> > +    new_block = ram_block_create(mr, size, size, ram_flags);
> > +    host = file_ram_alloc(new_block, size, fd, !file_size, offset, errp);
> > +    if (!host) {
> >           g_free(new_block);
> >           return NULL;
> >       }
> >   
> > +    new_block->host = host;
> >       ram_block_add(new_block, &local_err);
> >       if (local_err) {
> >           g_free(new_block);
> > @@ -1982,7 +1996,6 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, 
> > MemoryRegion *mr,
> >           return NULL;
> >       }
> >       return new_block;
> > -
> >   }
> >   
> >   
> > @@ -2054,18 +2067,10 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, ram_addr_t max_size,
> >       align = MAX(align, TARGET_PAGE_SIZE);
> >       size = ROUND_UP(size, align);
> >       max_size = ROUND_UP(max_size, align);
> > -
> > -    new_block = g_malloc0(sizeof(*new_block));
> > -    new_block->mr = mr;
> > -    new_block->resized = resized;
> > -    new_block->used_length = size;
> > -    new_block->max_length = max_size;
> >       assert(max_size >= size);
> > -    new_block->fd = -1;
> > -    new_block->guest_memfd = -1;
> > -    new_block->page_size = qemu_real_host_page_size();
> > -    new_block->host = host;
> > -    new_block->flags = ram_flags;
> > +    new_block = ram_block_create(mr, size, max_size, ram_flags);
> > +    new_block->resized = resized;
> > +
> >       ram_block_add(new_block, &local_err);
> >       if (local_err) {
> >           g_free(new_block);
> > diff --git a/system/trace-events b/system/trace-events
> > index 69c9044..f0a80ba 100644
> > --- a/system/trace-events
> > +++ b/system/trace-events
> > @@ -38,3 +38,6 @@ dirtylimit_state_finalize(void)
> >   dirtylimit_throttle_pct(int cpu_index, uint64_t pct, int64_t time_us) "CPU[%d] throttle percent: 
> > %" PRIu64 ", throttle adjust time %"PRIi64 " us"
> >   dirtylimit_set_vcpu(int cpu_index, uint64_t quota) "CPU[%d] set dirty page rate 
> > limit %"PRIu64
> >   dirtylimit_vcpu_execute(int cpu_index, int64_t sleep_time_us) "CPU[%d] sleep %"PRIi64 
> > " us"
> > +
> > +# physmem.c
> > +ram_block_create(const char *name, uint32_t flags, int fd, size_t used_length, size_t 
> > max_length, size_t align) "%s, flags %u, fd %d, len %lu, maxlen %lu, align %lu"