Re: [dpdk-dev] [PATCH 2/3] net/virtio-user: avoid parsing process mappings

[Tiwei Bie <tiwei.bie@intel.com>]

On Fri, Sep 07, 2018 at 01:21:35PM +0100, Burakov, Anatoly wrote:
> On 07-Sep-18 12:35 PM, Tiwei Bie wrote:
> > On Fri, Sep 07, 2018 at 10:39:16AM +0100, Burakov, Anatoly wrote:
> > > On 05-Sep-18 5:28 AM, Tiwei Bie wrote:
> > > > Recently some memory APIs were introduced to allow users to
> > > > get the file descriptor and offset for each memory segment.
> > > > We can leverage those APIs to get rid of the /proc magic on
> > > > memory table preparation in vhost-user backend.
> > > > 
> > > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > > ---
> > > 
> > > <snip>
> > > 
> > > > -	for (i = 0; i < num; ++i) {
> > > > -		mr = &msg->payload.memory.regions[i];
> > > > -		mr->guest_phys_addr = huges[i].addr; /* use vaddr! */
> > > > -		mr->userspace_addr = huges[i].addr;
> > > > -		mr->memory_size = huges[i].size;
> > > > -		mr->mmap_offset = 0;
> > > > -		fds[i] = open(huges[i].path, O_RDWR);
> > > > +	if (rte_memseg_get_fd_offset_thread_unsafe(ms, &offset) < 0) {
> > > > +		PMD_DRV_LOG(ERR, "Failed to get offset, ms=%p rte_errno=%d",
> > > > +			ms, rte_errno);
> > > > +		return -1;
> > > > +	}
> > > > +
> > > > +	start_addr = (uint64_t)(uintptr_t)ms->addr;
> > > > +	end_addr = start_addr + ms->len;
> > > > +
> > > > +	for (i = 0; i < wa->region_nr; i++) {
> > > 
> > > There has to be a better way than to run this loop on every segment. Maybe
> > > store last-used region, and only do a region look up if there's a mismatch?
> > > Generally, in single-file segments mode, you'll get lots of segments from
> > > one memseg list one after the other, so you will need to do a lookup once
> > > memseg list changes, instead of on each segment.
> > 
> > We may have holes in one memseg list due to dynamic free.
> > Do you mean we just need to do rte_memseg_contig_walk()
> > and we can assume that fds of the contiguous memegs will
> > be the same?
> 
> No, i didn't mean that.
> 
> Whether or not you are in single-file segments mode, you still need to scan
> each segment. However, you lose your state when you exit this function, and
> thus have to look up the appropriate memory region (that matches your fd)
> again, over and over. It would be good if you could store last-used memory
> region somewhere, so that next time you come back into this function, if the
> memseg has the same fd, you will not have to look it up.
> 
> Something like this:
> 
> struct walk_arg {
> 	*last_used;
> 	<snip>
> }
> 
> int walk_func() {
> 	<snip>
> 	cur_region = wa->last_used; // check if it matches
> 	if (cur->region->fd != fd) {
> 		// fd is different - we've changed the segment
> 		<snip>
> 		wa->last_used = cur_region
> 	}
> }

Thanks for the code. :)

> 
> So, cache last used region to not have to look it up again, because chances
> are, you won't have to. That way, you will still do region lookups, but only
> if you have to - not every time.

I can do it, but I'm not sure this optimization is really
necessary. Because this loop should be quite fast, as the
max number of regions permitted by vhost-user is quite
small. And actually we need to do that loop at least once
for each packet in vhost-user's dequeue and enqueue path,
i.e. the data path.


> 
> > 
> > > 
> > > > +		if (wa->fds[i] != fd)
> > > > +			continue;
> > > > +
> > > > +		mr = &wa->vm->regions[i];
> > > > +
> > > > +		if (mr->userspace_addr + mr->memory_size < end_addr)
> > > > +			mr->memory_size = end_addr - mr->userspace_addr;
> > > > +
> > > 
> > > <snip>
> > > 
> > > >    	int fds[VHOST_MEMORY_MAX_NREGIONS];
> > > >    	int fd_num = 0;
> > > > -	int i, len;
> > > > +	int len;
> > > >    	int vhostfd = dev->vhostfd;
> > > >    	RTE_SET_USED(m);
> > > > @@ -364,10 +337,6 @@ vhost_user_sock(struct virtio_user_dev *dev,
> > > >    		return -1;
> > > >    	}
> > > > -	if (req == VHOST_USER_SET_MEM_TABLE)
> > > > -		for (i = 0; i < fd_num; ++i)
> > > > -			close(fds[i]);
> > > > -
> > > 
> > > You're sharing fd's - presumably the other side of this is in a different
> > > process, so it's safe to close these fd's there?
> > 
> > Above code belongs to virtio-user, and it will close the
> > fds got from rte_memseg_get_fd_thread_unsafe().
> > 
> > Below is the code which will close these fds on the other
> > side (i.e. the vhost-user process):
> > 
> > https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L805
> > https://github.com/DPDK/dpdk/blob/3605968c2fa7/lib/librte_vhost/vhost_user.c#L97
> 
> OK, so not a problem then.
> 
> > 
> > 
> > > 
> > > >    	if (need_reply) {
> > > >    		if (vhost_user_read(vhostfd, &msg) < 0) {
> > > >    			PMD_DRV_LOG(ERR, "Received msg failed: %s",
> > > > 
> > > 
> > > 
> > > -- 
> > > Thanks,
> > > Anatoly
> > 
> 
> 
> -- 
> Thanks,
> Anatoly