Re: [Qemu-devel] [PATCH 3/3] memory: pass RAMBlock as a agrument

[Gonglei (Arei)]

> From: Fam Zheng [mailto:address@hidden
> Sent: Friday, April 29, 2016 4:07 PM
> To: Gonglei (Arei)
> Cc: address@hidden; address@hidden; Huangpeng (Peter)
> Subject: Re: [PATCH 3/3] memory: pass RAMBlock as a agrument
> 
> On Wed, 04/20 11:34, Gonglei wrote:
> > In this way, we can avoid to invoke qemu_get_ram_block()
> > at most time, which can save cpu cycle.
> >
> > Signed-off-by: Gonglei <address@hidden>
> > ---
> >  exec.c                  | 48
> ++++++++++++++++++++++++++++++------------------
> >  hw/misc/ivshmem.c       |  6 ++++--
> >  hw/virtio/vhost-user.c  | 11 ++++++-----
> >  include/exec/ram_addr.h |  6 +++---
> >  memory.c                |  3 ++-
> >  5 files changed, 45 insertions(+), 29 deletions(-)
> >
> > diff --git a/exec.c b/exec.c
> > index b0ecbcd..f3348aa 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1813,37 +1813,49 @@ void qemu_ram_remap(ram_addr_t addr,
> ram_addr_t length)
> >  }
> >  #endif /* !_WIN32 */
> >
> > -int qemu_get_ram_fd(ram_addr_t addr)
> > +int qemu_get_ram_fd(RAMBlock *ram_block, ram_addr_t addr)
> >  {
> > -    RAMBlock *block;
> > +    RAMBlock *block = ram_block;
> >      int fd;
> >
> > -    rcu_read_lock();
> > -    block = qemu_get_ram_block(addr);
> > -    fd = block->fd;
> > -    rcu_read_unlock();
> > +    if (block == NULL) {
> 
> Is this branch even used? It seems no caller passes in literal NULL in this
> patch. Can the variables be NULL?
> 
I'm not sure. But currently the variables never be NULL.

> If not, I think we can simply drop addr.
> 
Yes, add assert(block) at the front of those functions.

Regards,
-Gonglei

> > +        rcu_read_lock();
> > +        block = qemu_get_ram_block(addr);
> > +        fd = block->fd;
> > +        rcu_read_unlock();
> > +    } else {
> > +        fd = block->fd;
> > +    }
> >      return fd;
> >  }
> >
> > -void qemu_set_ram_fd(ram_addr_t addr, int fd)
> > +void qemu_set_ram_fd(RAMBlock *ram_block, ram_addr_t addr, int fd)
> >  {
> > -    RAMBlock *block;
> > +    RAMBlock *block = ram_block;
> >
> > -    rcu_read_lock();
> > -    block = qemu_get_ram_block(addr);
> > -    block->fd = fd;
> > -    rcu_read_unlock();
> > +    if (block == NULL) {
> 
> Same here...
> 
> > +        rcu_read_lock();
> > +        block = qemu_get_ram_block(addr);
> > +        block->fd = fd;
> > +        rcu_read_unlock();
> > +    } else {
> > +        block->fd = fd;
> > +    }
> >  }
> >
> > -void *qemu_get_ram_block_host_ptr(ram_addr_t addr)
> > +void *qemu_get_ram_block_host_ptr(RAMBlock *ram_block, ram_addr_t
> addr)
> >  {
> > -    RAMBlock *block;
> > +    RAMBlock *block = ram_block;
> >      void *ptr;
> >
> > -    rcu_read_lock();
> > -    block = qemu_get_ram_block(addr);
> > -    ptr = ramblock_ptr(block, 0);
> > -    rcu_read_unlock();
> > +    if (block == NULL) {
> 
> And here.
> 
> > +        rcu_read_lock();
> > +        block = qemu_get_ram_block(addr);
> > +        ptr = ramblock_ptr(block, 0);
> > +        rcu_read_unlock();
> > +    } else {
> > +        ptr = ramblock_ptr(block, 0);
> > +    }
> >      return ptr;
> >  }
> >
> > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> > index e40f23b..5ba0af7 100644
> > --- a/hw/misc/ivshmem.c
> > +++ b/hw/misc/ivshmem.c
> > @@ -533,7 +533,8 @@ static void process_msg_shmem(IVShmemState *s,
> int fd, Error **errp)
> >      }
> >      memory_region_init_ram_ptr(&s->server_bar2, OBJECT(s),
> >                                 "ivshmem.bar2", size, ptr);
> > -    qemu_set_ram_fd(memory_region_get_ram_addr(&s->server_bar2),
> fd);
> > +    qemu_set_ram_fd(s->server_bar2.ram_block,
> > +                    memory_region_get_ram_addr(&s->server_bar2),
> fd);
> >      s->ivshmem_bar2 = &s->server_bar2;
> >  }
> >
> > @@ -940,7 +941,8 @@ static void ivshmem_exit(PCIDevice *dev)
> >                               strerror(errno));
> >              }
> >
> > -            fd =
> qemu_get_ram_fd(memory_region_get_ram_addr(s->ivshmem_bar2));
> > +            fd = qemu_get_ram_fd(s->ivshmem_bar2->ram_block,
> > +
> memory_region_get_ram_addr(s->ivshmem_bar2));
> >              close(fd);
> >          }
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 5914e85..4b93533 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -248,17 +248,18 @@ static int vhost_user_set_mem_table(struct
> vhost_dev *dev,
> >      for (i = 0; i < dev->mem->nregions; ++i) {
> >          struct vhost_memory_region *reg = dev->mem->regions + i;
> >          ram_addr_t ram_addr;
> > +        MemoryRegion *mr;
> >
> >          assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
> > -        qemu_ram_addr_from_host((void
> *)(uintptr_t)reg->userspace_addr,
> > +        mr = qemu_ram_addr_from_host((void
> *)(uintptr_t)reg->userspace_addr,
> >                                  &ram_addr);
> > -        fd = qemu_get_ram_fd(ram_addr);
> > +        fd = qemu_get_ram_fd(mr->ram_block, ram_addr);
> >          if (fd > 0) {
> >              msg.payload.memory.regions[fd_num].userspace_addr =
> reg->userspace_addr;
> >              msg.payload.memory.regions[fd_num].memory_size  =
> reg->memory_size;
> >              msg.payload.memory.regions[fd_num].guest_phys_addr =
> reg->guest_phys_addr;
> >              msg.payload.memory.regions[fd_num].mmap_offset =
> reg->userspace_addr -
> > -                (uintptr_t) qemu_get_ram_block_host_ptr(ram_addr);
> > +                (uintptr_t)
> qemu_get_ram_block_host_ptr(mr->ram_block, ram_addr);
> >              assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
> >              fds[fd_num++] = fd;
> >          }
> > @@ -622,11 +623,11 @@ static bool vhost_user_can_merge(struct
> vhost_dev *dev,
> >
> >      mr = qemu_ram_addr_from_host((void *)(uintptr_t)start1,
> &ram_addr);
> >      assert(mr);
> > -    mfd = qemu_get_ram_fd(ram_addr);
> > +    mfd = qemu_get_ram_fd(mr->ram_block, ram_addr);
> >
> >      mr = qemu_ram_addr_from_host((void *)(uintptr_t)start2,
> &ram_addr);
> >      assert(mr);
> > -    rfd = qemu_get_ram_fd(ram_addr);
> > +    rfd = qemu_get_ram_fd(mr->ram_block, ram_addr);
> >
> >      return mfd == rfd;
> >  }
> > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> > index 5b6e1b8..4e1e5c3 100644
> > --- a/include/exec/ram_addr.h
> > +++ b/include/exec/ram_addr.h
> > @@ -105,9 +105,9 @@ RAMBlock *qemu_ram_alloc_resizeable(ram_addr_t
> size, ram_addr_t max_size,
> >                                                      uint64_t
> length,
> >                                                      void
> *host),
> >                                      MemoryRegion *mr, Error
> **errp);
> > -int qemu_get_ram_fd(ram_addr_t addr);
> > -void qemu_set_ram_fd(ram_addr_t addr, int fd);
> > -void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
> > +int qemu_get_ram_fd(RAMBlock *ram_block, ram_addr_t addr);
> > +void qemu_set_ram_fd(RAMBlock *ram_block, ram_addr_t addr, int fd);
> > +void *qemu_get_ram_block_host_ptr(RAMBlock *ram_block, ram_addr_t
> addr);
> >  void qemu_ram_free(RAMBlock *block);
> >
> >  int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error
> **errp);
> > diff --git a/memory.c b/memory.c
> > index 239e6da..fa2b6e2 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -1641,7 +1641,8 @@ int memory_region_get_fd(MemoryRegion *mr)
> >
> >      assert(mr->ram_block);
> >
> > -    return qemu_get_ram_fd(memory_region_get_ram_addr(mr) &
> TARGET_PAGE_MASK);
> > +    return qemu_get_ram_fd(mr->ram_block,
> > +                      memory_region_get_ram_addr(mr) &
> TARGET_PAGE_MASK);
> >  }
> >
> >  void *memory_region_get_ram_ptr(MemoryRegion *mr)
> > --
> > 1.7.12.4
> >
> >