[dpdk-dev] vhost: Fix reset_owner message handling not to clear callfd
Commit Message
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(-)
Comments
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().
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).
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
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
@@ -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;
+}
+
+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);
}
/*