[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/2] vhost: add used memslot number for vhost
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/2] vhost: add used memslot number for vhost-user and vhost-kernel separately |
Date: |
Thu, 28 Dec 2017 12:11:55 +0100 |
On Sat, 23 Dec 2017 07:09:51 +0000
"Zhoujian (jay)" <address@hidden> wrote:
> Hi Igor,
>
> > -----Original Message-----
> > From: Igor Mammedov [mailto:address@hidden
> > Sent: Saturday, December 23, 2017 3:03 AM
> > To: Zhoujian (jay) <address@hidden>
> > Cc: Huangweidong (C) <address@hidden>; address@hidden; wangxin
> > (U) <address@hidden>; address@hidden; Liuzhe (Cloud
> > Open Labs, NFV) <address@hidden>; address@hidden; Gonglei
> > (Arei) <address@hidden>
> > Subject: Re: [Qemu-devel] [PATCH v2 1/2] vhost: add used memslot number
> > for vhost-user and vhost-kernel separately
> >
> > On Fri, 22 Dec 2017 17:25:13 +0100
> > Igor Mammedov <address@hidden> wrote:
> >
> > > On Fri, 15 Dec 2017 16:45:54 +0800
> > > Jay Zhou <address@hidden> wrote:
> > [...]
> >
> > > > +static void vhost_user_set_used_memslots(struct vhost_dev *dev) {
> > > > + int counter = 0;
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < dev->mem->nregions; ++i) {
> > > > + struct vhost_memory_region *reg = dev->mem->regions + i;
> > > > + ram_addr_t offset;
> > > > + MemoryRegion *mr;
> > > > + int fd;
> > > > +
> > > > + assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
> > > > + mr = memory_region_from_host((void *)(uintptr_t)reg-
> > >userspace_addr,
> > > > + &offset);
> > > > + fd = memory_region_get_fd(mr);
> > > > + if (fd > 0) {
> > > > + counter++;
> > > > + }
> > > > + }
> > > vhost_user_set_mem_table() already does the same counting, there is no
> > > point in duplicating it, just drop vhost_set_used_memslots callback
> > >
> > > > + vhost_user_used_memslots = counter;
> > > and update this value from vhost_user_set_mem_table()
> > never mind, updating it from vhost_user_set_mem_table() as it's called
> > only when device is started which is not the case when backend is
> > initialized, so the way you did it should work for both cases
> >
>
> How about do it like this(not sure whether the best way, any idea?):
>
> Define a new function, e.g.
>
> static void vhost_user_set_used_memslots_and_msgs(struct vhost_dev *dev,
> VhostUserMsg *msg, size_t *fd_num, int *fds)
s/vhost_user_set_used_memslots_and_msgs/vhost_user_prepare_msg/
> {
> int num = 0;
> int i;
>
> for (i = 0; i < dev->mem->nregions; ++i) {
> struct vhost_memory_region *reg = dev->mem->regions + i;
> ram_addr_t offset;
> MemoryRegion *mr;
> int fd;
>
> assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
> mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
> &offset);
> fd = memory_region_get_fd(mr);
> if (fd > 0) {
> if (msg && fd_num && fds) {
> msg->payload.memory.regions[num].userspace_addr
> = reg->userspace_addr;
> msg->payload.memory.regions[num].memory_size
> = reg->memory_size;
> msg->payload.memory.regions[num].guest_phys_addr
> = reg->guest_phys_addr;
> msg->payload.memory.regions[num].mmap_offset = offset;
> assert(num < VHOST_MEMORY_MAX_NREGIONS);
there is no need to assert here function can return error.
> fds[num++] = fd;
Is num counting in wrong place?
if (msg && fd_num && fds) => false
is the case vhost_user_set_used_memslots(), so it won't count 'num'
which is later used to intialize vhost_user_used_memslots.
> }
> }
> }
> vhost_user_used_memslots = num;
add comment here as it won't be obvious to reader why vhost_user_used_memslots
is here
> if (fd_num)
> *fd_num = num;
msg.payload.memory.nregions can be used directly instead of num amd fd_num
and it's 1 less argument to pass out of function.
> }
>
> And call it when the backend is initialized and the device is started
> respectively,
>
> static void vhost_user_set_used_memslots(struct vhost_dev *dev)
static int
for return value
> {
> vhost_user_set_used_memslots_and_msgs(dev, NULL, NULL, NULL);
we can do here
vhost_user_set_used_memslots_and_msgs(dev, &msg, &fd_num, fds);
and discard/ignore results, that way it will be less conditionals inside of
called function
> }
>
> static int vhost_user_set_mem_table(struct vhost_dev *dev,
> struct vhost_memory *mem)
> {
> [...]
>
> vhost_user_set_used_memslots_and_msgs(dev, &msg, &fd_num, fds);
rc = vhost_user_set_used_memslots_and_msgs(dev, &msg, &fd_num, fds);
and return error from vhost_user_set_mem_table(), caller should be able
to coupe with error.
>
> [...]
> }
>
>
> Regards,
> Jay
>
> >
> > >
> > > > +}
> > > > +
> > [...]
>
[Qemu-devel] [PATCH v2 2/2] vhost: double check used memslots number, Jay Zhou, 2017/12/15
Re: [Qemu-devel] [PATCH v2 2/2] vhost: double check used memslots number, Zhoujian (jay), 2017/12/23
Re: [Qemu-devel] [PATCH v2 0/2] vhost: two fixes, no-reply, 2017/12/16