Re: [dpdk-dev] [PATCH v2] vhost: Fix default value of kickfd and callfd

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

On 2016/03/14 11:08, Yuanhan Liu wrote:
> On Mon, Mar 14, 2016 at 10:54:14AM +0900, Tetsuya Mukawa wrote:
>> On 2016/03/11 16:19, Yuanhan Liu wrote:
>>> On Thu, Mar 10, 2016 at 04:06:05PM +0900, Tetsuya Mukawa wrote:
>>>> Currently, default values of kickfd and callfd are -1.
>>>> If the values are -1, current code guesses kickfd and callfd haven't
>>>> been initialized yet. And vhost library will guess the virtqueue isn't
>>>> ready for processing.
>>>> But callfd and kickfd will be set as -1 when "--enable-kvm"
>>>> isn't specified in QEMU command line. It means we cannot treat -1 as
>>>> uninitialized state. The patch changes default values to -2. And the
>>>> patch defines -2 as VIRTIO_UNINITIALIZED_EVENTFD.
>>> This looks more like a workaround to me. 
>>
>> Hi Yuanhan,
>>
>> Sorry for late reply.
>> I have checked QEMU documentation, and found below.
>>
>> ----------
>>  * VHOST_USER_SET_VRING_CALL
>>
>>       Id: 14
>>       Equivalent ioctl: VHOST_SET_VRING_CALL
>>       Master payload: u64
>>
>>       Set the event file descriptor to signal when buffers are used. It
>>       is passed in the ancillary data.
>>       Bits (0-7) of the payload contain the vring index. Bit 8 is the
>>       invalid FD flag.
>> ----------
>>
>> VHOST_USER_SET_VRING_KICK has almost same description.
>> I will check this invalid flag, and if it works for our case, then will
>> use it.
>> How about it?
> Yeah, that indeed sounds much better.

I've checked current dpdk code.
It seems we've already checked invalid flag like below.

        if (pmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK)
                file.fd = -1;
        else
                file.fd = pmsg->fds[0];

So how about adding below macros or enum?

#define VIRTIO_UNINITIALIZED_EVENTFD   (-2)
#define VIRTIO_INVALID_EVENTFD              (-1)

I am still not sure whether using enum is better or not.
But here is one of example patch.
What do you think?

diff --git a/lib/librte_vhost/rte_virtio_net.h
b/lib/librte_vhost/rte_virtio_net.h
index 7d1fde2..2a7566d 100644
--- a/lib/librte_vhost/rte_virtio_net.h
+++ b/lib/librte_vhost/rte_virtio_net.h
@@ -89,6 +89,8 @@ struct vhost_virtqueue {
        uint16_t                vhost_hlen;             /**< Vhost
header length (varies depending on RX merge buffers. */
        volatile uint16_t       last_used_idx;          /**< Last index
used on the available ring */
        volatile uint16_t       last_used_idx_res;      /**< Used for
multiple devices reserving buffers. */
+#define VIRTIO_UNINITIALIZED_EVENTFD   (-2)
+#define VIRTIO_INVALID_EVENTFD         (-1)
        int                     callfd;                 /**< Used to
notify the guest (trigger interrupt). */
        int                     kickfd;                 /**< Currently
unused as polling mode is enabled. */
        int                     enabled;
diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c
index 9d23eb1..29d002d 100644
--- a/lib/librte_vhost/vhost_rxtx.c
+++ b/lib/librte_vhost/vhost_rxtx.c
@@ -333,7 +333,8 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id,
        rte_mb();

        /* Kick the guest if necessary. */
-       if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
+       if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT)
+                       && (vq->callfd >= 0))
                eventfd_write(vq->callfd, (eventfd_t)1);
        return count;
 }
@@ -654,7 +655,8 @@ merge_rx_exit:
                rte_mb();

                /* Kick the guest if necessary. */
-               if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
+               if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT)
+                               && (vq->callfd >= 0))
                        eventfd_write(vq->callfd, (eventfd_t)1);
        }

@@ -1048,7 +1050,8 @@ rte_vhost_dequeue_burst(struct virtio_net *dev,
uint16_t queue_id,
                        sizeof(vq->used->idx));

        /* Kick guest if required. */
-       if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT))
+       if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT)
+                       && (vq->callfd >= 0))
                eventfd_write(vq->callfd, (eventfd_t)1);

 out:
diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.c
b/lib/librte_vhost/vhost_user/virtio-net-user.c
index 65b5652..f5248bc 100644
--- a/lib/librte_vhost/vhost_user/virtio-net-user.c
+++ b/lib/librte_vhost/vhost_user/virtio-net-user.c
@@ -225,8 +225,8 @@ static int
 vq_is_ready(struct vhost_virtqueue *vq)
 {
        return vq && vq->desc   &&
-              vq->kickfd != -1 &&
-              vq->callfd != -1;
+              vq->kickfd != VIRTIO_UNINITIALIZED_EVENTFD &&
+              vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD;
 }

 static int
@@ -258,7 +258,7 @@ user_set_vring_call(struct vhost_device_ctx ctx,
struct VhostUserMsg *pmsg)

        file.index = pmsg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
        if (pmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK)
-               file.fd = -1;
+               file.fd = VIRTIO_INVALID_EVENTFD;
        else
                file.fd = pmsg->fds[0];
        RTE_LOG(INFO, VHOST_CONFIG,
@@ -279,7 +279,7 @@ user_set_vring_kick(struct vhost_device_ctx ctx,
struct VhostUserMsg *pmsg)

        file.index = pmsg->payload.u64 & VHOST_USER_VRING_IDX_MASK;
        if (pmsg->payload.u64 & VHOST_USER_VRING_NOFD_MASK)
-               file.fd = -1;
+               file.fd = VIRTIO_INVALID_EVENTFD;
        else
                file.fd = pmsg->fds[0];
        RTE_LOG(INFO, VHOST_CONFIG,
@@ -316,10 +316,10 @@ 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[state->index]->kickfd >= 0) {
+       if (dev->virtqueue[state->index]->kickfd >= 0)
                close(dev->virtqueue[state->index]->kickfd);
-               dev->virtqueue[state->index]->kickfd = -1;
-       }
+
+       dev->virtqueue[state->index]->kickfd = VIRTIO_UNINITIALIZED_EVENTFD;

        return 0;
 }
diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index fe1a77e..3498e9d 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -263,8 +263,8 @@ init_vring_queue(struct vhost_virtqueue *vq, int qp_idx)
 {
        memset(vq, 0, sizeof(struct vhost_virtqueue));

-       vq->kickfd = -1;
-       vq->callfd = -1;
+       vq->kickfd = VIRTIO_UNINITIALIZED_EVENTFD;
+       vq->callfd = VIRTIO_UNINITIALIZED_EVENTFD;

        /* Backends are set to -1 indicating an inactive device. */
        vq->backend = -1;



> 	--yliu
>
>>> Besides, this patch would make
>>> following fail:
>>>
>>>     eventfd_write(vq->callfd, (eventfd_t)1);
>> It's my fault.
>> I thought above case should be blocked by virtio spec itself. So just
>> leave it.
>> But eventfd is came from vhost spec, so VRING_AVAIL_F_NO_INTERRUPT might
>> be set even when kickfd and callfd are -1.
>> Thanks for checking it.
>>
>> Tetsuya
>>
>>> 	--yliu