Re: [dpdk-dev] [PATCH v7 3/8] vhost: vring queue setup for multiple queue support

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Thu, Oct 22, 2015 at 09:49:58AM +0000, Xie, Huawei wrote:
> On 10/21/2015 11:48 AM, Yuanhan Liu wrote:
> > All queue pairs, including the default (the first) queue pair,
> > are allocated dynamically, when a vring_call message is received
> > first time for a specific queue pair.
> >
> > This is a refactor work for enabling vhost-user multiple queue;
> > it should not break anything as it does no functional changes:
> > we don't support mq set, so there is only one mq at max.
> >
> > This patch is based on Changchun's patch.
> >
> [...]
> >  
> >  void
> > @@ -290,13 +298,9 @@ user_get_vring_base(struct vhost_device_ctx ctx,
> >  	 * sent and only sent in vhost_vring_stop.
> >  	 * TODO: cleanup the vring, it isn't usable since here.
> >  	 */
> > -	if ((dev->virtqueue[VIRTIO_RXQ]->kickfd) >= 0) {
> > -		close(dev->virtqueue[VIRTIO_RXQ]->kickfd);
> > -		dev->virtqueue[VIRTIO_RXQ]->kickfd = -1;
> > -	}
> > -	if ((dev->virtqueue[VIRTIO_TXQ]->kickfd) >= 0) {
> > -		close(dev->virtqueue[VIRTIO_TXQ]->kickfd);
> > -		dev->virtqueue[VIRTIO_TXQ]->kickfd = -1;
> > +	if ((dev->virtqueue[state->index]->kickfd) >= 0) {
> > +		close(dev->virtqueue[state->index]->kickfd);
> > +		dev->virtqueue[state->index]->kickfd = -1;
> >  	}
> Since we change the behavior here, better list in the commit message as
> well.

I checked the code again, and found I should not change that:
GET_VRING_BASE is sent per virt queue pair.

BTW, it's wrong to do this kind of stuff here, we need fix
it in future.

> 
> >  
> >  
> > @@ -680,13 +704,21 @@ set_vring_call(struct vhost_device_ctx ctx, struct vhost_vring_file *file)
> >  {
> >  	struct virtio_net *dev;
> >  	struct vhost_virtqueue *vq;
> > +	uint32_t cur_qp_idx = file->index / VIRTIO_QNUM;
> >  
> >  	dev = get_device(ctx);
> >  	if (dev == NULL)
> >  		return -1;
> >  
> > +	/* alloc vring queue pair if it is a new queue pair */
> > +	if (cur_qp_idx + 1 > dev->virt_qp_nb) {
> > +		if (alloc_vring_queue_pair(dev, cur_qp_idx) < 0)
> > +			return -1;
> > +	}
> > +
> Here we rely on the fact that this set_vring_call message is sent in the
> continuous ascending order of queue idx 0, 1, 2, ...

That's true.

> 
> >  	/* file->index refers to the queue index. The txq is 1, rxq is 0. */
> >  	vq = dev->virtqueue[file->index];
> > +	assert(vq != NULL);
> >  
> If we allocate the queue until the we receive the first vring message,
> better add comment that we rely on this fact.

Will do that.

> Could we add the vhost-user message to tell us the queue number QEMU
> allocates before vring message?

We may need do that. But it's too late to make it in v2.2

	--yliu

> >  	if (vq->callfd >= 0)
> >  		close(vq->callfd);
>