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/ > { > 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 > > > > > > > > > > +} > > > > + > > [...] >
