Re: [dpdk-dev] [RFC] net/virtio-user: avoid parsing process mappings

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

On Tue, Aug 28, 2018 at 09:12:38AM +0100, Burakov, Anatoly wrote:
> On 28-Aug-18 8:53 AM, Tiwei Bie wrote:
[...]
> >   
> > -	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);
> > +	start_addr = (uint64_t)(uintptr_t)ms->addr;
> > +	end_addr = start_addr + ms->len;
> > +
> > +	/*
> > +	 * XXX workaround!
> > +	 *
> > +	 * For --single-file-segments case, offset should be:
> > +	 * offset = rte_fbarray_find_idx(&msl->memseg_arr, ms) * msl->page_sz;
> > +	 *
> > +	 * But it's not true for non-single-file-segments case.
> > +	 *
> > +	 * This is a temporary workaround which assumes the file will
> > +	 * also be mapped from the beginning in --single-file-segments
> > +	 * case. It should be replaced when we have a memory API to
> > +	 * get the offset.
> 
> Yes, this is an unfortunate consequence of having split personalities in 
> memory subsystem. A good solution would be to just deprecate 
> non-single-file segments mode and always use it, but we cannot do that 
> because we support kernel versions that do not support fallocate() on 
> hugetlbfs (and it still leaves legacy mem mode, which effectively 
> behaves like non-single-file segments as far as virtio is concerned).
> 
> How about we add this API in v2 for seg fd patchset?

Sure, thanks!

Besides, I saw another minor issue. When --legacy-mem
and --single-file-segments are both specified, the EAL
init doesn't fail, but fd can't be got successfully.

> 
> > +	 */
> > +	offset = 0;
> > +
[...]
> > @@ -109,9 +111,24 @@ is_vhost_user_by_type(const char *path)
> >   int
> >   virtio_user_start_device(struct virtio_user_dev *dev)
> >   {
> > +	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
> >   	uint64_t features;
> >   	int ret;
> >   
> > +	/*
> > +	 * XXX workaround!
> > +	 *
> > +	 * We need to make sure that the locks will be
> > +	 * taken in the correct order to avoid deadlocks.
> > +	 *
> > +	 * Before releasing this lock, this thread should
> > +	 * not trigger any memory hotplug events.
> > +	 *
> > +	 * This is a temporary workaround, and should be
> > +	 * replaced when we get proper supports from the
> > +	 * memory subsystem in the future.
> > +	 */
> > +	rte_rwlock_read_lock(&mcfg->memory_hotplug_lock);
> 
> This feels like it should be a separate patch, because it fixes the 
> issue that exists independently of whether we have segment fd API or not.

Yeah, I'll put it in a separate patch.

> 
> >   	pthread_mutex_lock(&dev->mutex);
> >   
> >   	if (is_vhost_user_by_type(dev->path) && dev->vhostfd < 0)
> > @@ -152,10 +169,12 @@ virtio_user_start_device(struct virtio_user_dev *dev)
> >   
> >   	dev->started = true;
> >   	pthread_mutex_unlock(&dev->mutex);
> > +	rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
> >   
> >   	return 0;
> >   error:
> >   	pthread_mutex_unlock(&dev->mutex);
> > +	rte_rwlock_read_unlock(&mcfg->memory_hotplug_lock);
> >   	/* TODO: free resource here or caller to check */
> >   	return -1;
> >   }
> > 
> 
> 
> -- 
> Thanks,
> Anatoly