Re: [Qemu-devel] vhost-user breaks after 96a3d98.

[Jason Wang <jasowang@redhat.com>]

On 2017年01月04日 11:26, Jason Wang wrote:
>
>
> On 2017年01月04日 00:27, Michael S. Tsirkin wrote:
>> On Tue, Jan 03, 2017 at 06:28:18PM +0800, Jason Wang wrote:
>>>
>>> On 2017年01月03日 11:09, Jason Wang wrote:
>>>>
>>>> On 2016年12月30日 20:41, Flavio Leitner wrote:
>>>>> Hi,
>>>>>
>>>>> While I was testing vhost-user using OVS 2.5 and DPDK 2.2.0 in the
>>>>> host and testpmd dpdk 2.2.0 in the guest, I found that the commit
>>>>> below breaks the environment and no packets gets into the guest.
>>>>>
>>>>> dpdk port --> OVS --> vhost-user --> guest --> testpmd
>>>>>                            ^--- drops here         ^--- no packets 
>>>>> here.
>>>>>
>>>>> commit 96a3d98d2cdbd897ff5ab33427aa4cfb94077665
>>>>> Author: Jason Wang <jasowang@redhat.com>
>>>>> Date:   Mon Aug 1 16:07:58 2016 +0800
>>>>>
>>>>>       vhost: don't set vring call if no vector
>>>>>            We used to set vring call fd unconditionally even if guest
>>>>> driver does
>>>>>       not use MSIX for this vritqueue at all. This will cause lots of
>>>>>       unnecessary userspace access and other checks for drivers does
>>>>> not use
>>>>>       interrupt at all (e.g virtio-net pmd). So check and clean vring
>>>>> call
>>>>>       fd if guest does not use any vector for this virtqueue at
>>>>>       all.
>>>>> [...]
>>>>>
>>>>> Thanks,
>>>> Hi Flavio:
>>>>
>>>> Thanks for reporting this issue, could this be a bug of vhost-user? (I
>>>> believe virito-net pmd does not use interrupt for rx/tx at all)
>>>>
>>>> Anyway, will try to reproduce it.
>>>>
>>> Could not reproduce this issue on similar setups (the only 
>>> difference is I
>>> don't create dpdk port) with dpdk 16.11 and ovs.git HEAD. Suspect an 
>>> issue
>>> dpdk. Will try OVS 2.5 + DPDK 2.2.0.
>>>
>>> Thanks
>> Possibly dpdk assumed that call fd must be present unconditionally.
>> Limit this patch to when protocol is updated? add a new protocol flag?
>
> If this is a bug of dpdk, I tend to fix it (or just disable this patch 
> for vhost-user). I'm not sure whether or not it's worthwhile to add a 
> new protocol flag which was used to tell qemu that bug X was fixed.
>
> Thanks
>
>

Haven't tried but looking at vq_is_ready() in v2.2.0:

static int
vq_is_ready(struct vhost_virtqueue *vq)
{
     return vq && vq->desc   &&
            vq->kickfd != -1 &&
            vq->callfd != -1;
}

Which assumes callfd must be set which seems wrong. And this has been 
fixed by

commit fb871d0a4dc1c038a381c524cdb86fe83d21d842
Author: Tetsuya Mukawa <mukawa@igel.co.jp>
Date:   Mon Mar 14 17:53:32 2016 +0900

     vhost: fix default value of kickfd and callfd

     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. Then 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 defines -1 and -2 as VIRTIO_INVALID_EVENTFD and
     VIRTIO_UNINITIALIZED_EVENTFD, and uses VIRTIO_UNINITIALIZED_EVENTFD for
     the default values of kickfd and callfd.

     Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
     Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Flavio, you could try to backport this to 2.2.0 to see if it fixes your 
issue.

Thanks