Re: [dpdk-dev] [PATCH] vhost: fix vhost_user_set_mem_table error

["Yang, Yi" <yi.y.yang@intel.com>]

On Fri, Sep 22, 2017 at 01:28:48PM +0800, Tan, Jianfeng wrote:
> 
> 
> > +				}
> > +				reg->guest_user_addr = 0;
> > +				j++;
> > +			} else {
> > +				i++;
> > +				j++;
> > +			}
> > +		}
> 
> The algorithm here could be problematic for hot plug. For example,
> 
> T0: we have two regions with address 1, 3.
> T1: we add another region, 2, which makes the regions as 1, 2, 3.
> 
> 1st iteration, 1 matches 1, i++, j++;
> 2nd iteration, 2 does not match 3, unmap 3, j++.

Jianfeng, thank you so much for your comments, but per my understanding
such hot add is impossible, this will result in discontinuous physical
address. Do you know how we can trigger such use case?

> 
> > +
> > +		/* munmap these regions because they have been hot
> > unplugged */
> > +		for (; j < dev->mem->nregions; j++) {
> > +			reg = &dev->mem->regions[j];
> > +			if (reg->host_user_addr) {
> > +				munmap(reg->mmap_addr, reg-
> > >mmap_size);
> > +				close(reg->fd);
> > +			}
> > +			reg->guest_user_addr = 0;
> > +		}
> >  	}
> > 
> > -	dev->nr_guest_pages = 0;
> >  	if (!dev->guest_pages) {
> > +		dev->nr_guest_pages = 0;
> 
> We cannot just move this line here, which results in a wrong nr_guest_pages if we do hot plug/unplug instead of initialization.

For initialization, dev->guest_pages is 0, for hot plug/unplug,
dev->guest_pages isn't 0, so it is ok.

> 
> >  		dev->max_guest_pages = 8;
> >  		dev->guest_pages = malloc(dev->max_guest_pages *
> >  						sizeof(struct guest_page));
> > @@ -532,7 +558,7 @@ vhost_user_set_mem_table(struct virtio_net *dev,
> > struct VhostUserMsg *pmsg)
> >  		}
> >  	}
> > 
> > -	dev->mem = rte_zmalloc("vhost-mem-table", sizeof(struct
> > rte_vhost_memory) +
> > +	dev->mem = rte_realloc(dev->mem, sizeof(struct
> > rte_vhost_memory) +
> >  		sizeof(struct rte_vhost_mem_region) * memory.nregions, 0);
> >  	if (dev->mem == NULL) {
> >  		RTE_LOG(ERR, VHOST_CONFIG,
> > @@ -546,6 +572,38 @@ vhost_user_set_mem_table(struct virtio_net *dev,
> > struct VhostUserMsg *pmsg)
> >  		fd  = pmsg->fds[i];
> >  		reg = &dev->mem->regions[i];
> > 
> > +		/* This region should be skipped if it is initialized before */
> > +		if (reg->guest_user_addr ==
> > memory.regions[i].userspace_addr) {
> 
> Also problematic for hot unplug. For example,
> T0: we have three regions, 1, 2, 3.
> T1: remove region 2, with only 1, 3 left.
> 
> 1st iteration, 1 matches 1, skip old region 1.
> 2nd iteration, 3 does not match 2, do the mmap.

It is ok, I have verified it, let us have a discussion in meeting.