Re: [PATCH] vhost: Fix reset_owner message handling not to clear callfd

[Tetsuya Mukawa <mukawa@igel.co.jp>]

On 2015/11/19 16:03, Yuanhan Liu wrote:
> On Thu, Nov 19, 2015 at 03:23:26PM +0900, Tetsuya Mukawa wrote:
>> The patch fixes reset_owner message handling not to clear callfd,
>> because callfd will be valid while connection is establihed.
>>
>> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
>> ---
>>  lib/librte_vhost/virtio-net.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
>> index 886c104..ae1e4bd 100644
>> --- a/lib/librte_vhost/virtio-net.c
>> +++ b/lib/librte_vhost/virtio-net.c
>> @@ -322,6 +322,25 @@ init_vring_queue_pair(struct virtio_net *dev, uint32_t qp_idx)
>>  	init_vring_queue(dev->virtqueue[base_idx + VIRTIO_TXQ], qp_idx);
>>  }
>>  
>> +static void
>> +reset_vring_queue(struct vhost_virtqueue *vq, int qp_idx)
>> +{
>> +	int callfd;
>> +
>> +	callfd = vq->callfd;
>> +	init_vring_queue(vq, qp_idx);
>> +	vq->callfd = callfd;
> It may fix your issue, but it's not a proper fix. As we actually closed
> callfd at cleanup_vq().

Yes, you are correct.
We also need to fix above.

>
> BTW, the name, cleanup_vq/device, is not well taken. For a name like
> cleanup, those functions should be invoked at device remove stage (say,
> shutting down a VM), but not at reset stage (which is actually treated
> as STOP in current QEMU implementation).
>
> I have a plan to correct it, but I guess it's not the right time as
> v2.2 is coming soon. However, for a fix, it could be simple: how about
> adding another arg to cleanup_vq:
>
> 	cleanup_vq(struct vhost_virtqueue *vq, int destory)
>
> And we only close callfd when destroy flag is set. (And of course, this
> patch is needed).

Yes I agree with it!
Let me have a few hours, I will send it by today.

Thanks,
Tetsuya

> What do you think of that?
>
> 	--yliu
>> +}
>> +
>> +static void
>> +reset_vring_queue_pair(struct virtio_net *dev, uint32_t qp_idx)
>> +{
>> +	uint32_t base_idx = qp_idx * VIRTIO_QNUM;
>> +
>> +	reset_vring_queue(dev->virtqueue[base_idx + VIRTIO_RXQ], qp_idx);
>> +	reset_vring_queue(dev->virtqueue[base_idx + VIRTIO_TXQ], qp_idx);
>> +}
>> +
>>  static int
>>  alloc_vring_queue_pair(struct virtio_net *dev, uint32_t qp_idx)
>>  {
>> @@ -362,7 +381,7 @@ reset_device(struct virtio_net *dev)
>>  	dev->flags = 0;
>>  
>>  	for (i = 0; i < dev->virt_qp_nb; i++)
>> -		init_vring_queue_pair(dev, i);
>> +		reset_vring_queue_pair(dev, i);
>>  }
>>  
>>  /*
>> -- 
>> 2.1.4