> -----Original Message-----
> From: Igor Mammedov [mailto:imamm...@redhat.com]
> Sent: Thursday, December 28, 2017 7:12 PM
> To: Zhoujian (jay) <jianjay.z...@huawei.com>
> Cc: Huangweidong (C) <weidong.hu...@huawei.com>; m...@redhat.com; wangxin
> (U) <wangxinxin.w...@huawei.com>; dgilb...@redhat.com; Liuzhe (Cloud Open
> Labs, NFV) <gary.liu...@huawei.com>; qemu-devel@nongnu.org; Gonglei (Arei)
> <arei.gong...@huawei.com>
> Subject: Re: [Qemu-devel] [PATCH v2 1/2] vhost: add used memslot number
> for vhost-user and vhost-kernel separately
>
> On Sat, 23 Dec 2017 07:09:51 +0000
> "Zhoujian (jay)" <jianjay.z...@huawei.com> wrote:
>
> > Hi Igor,
> >
> > > -----Original Message-----
> > > From: Igor Mammedov [mailto:imamm...@redhat.com]
> > > Sent: Saturday, December 23, 2017 3:03 AM
> > > To: Zhoujian (jay) <jianjay.z...@huawei.com>
> > > Cc: Huangweidong (C) <weidong.hu...@huawei.com>; m...@redhat.com;
> > > wangxin
> > > (U) <wangxinxin.w...@huawei.com>; qemu-devel@nongnu.org; Liuzhe
> > > (Cloud Open Labs, NFV) <gary.liu...@huawei.com>;
> > > dgilb...@redhat.com; Gonglei
> > > (Arei) <arei.gong...@huawei.com>
> > > 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 <imamm...@redhat.com> wrote:
> > >
> > > > On Fri, 15 Dec 2017 16:45:54 +0800 Jay Zhou
> > > > <jianjay.z...@huawei.com> 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/
Ok
>
> > {
> > 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.
Ok
>
> > 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.
My bad, will fix.
> > }
> > }
> > }
> > vhost_user_used_memslots = num;
> add comment here as it won't be obvious to reader why
> vhost_user_used_memslots is here
Will do.
>
> > 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.
>
Will fix all of them in the next version.
Regards,
Jay
