[dpdk-dev] [PATCH 2/3] net/virtio_user: fix wrong sequence of messages

[Tan, Jianfeng]

Hi Yuanhan,


On 9/6/2016 2:42 PM, Yuanhan Liu wrote:
> On Fri, Aug 05, 2016 at 11:36:42AM +0000, Jianfeng Tan wrote:
>> When virtio_user is used with VPP's native vhost user, it cannot
>> send/receive any packets.
>>
>> The root cause is that vpp-vhost-user translates the message
>> VHOST_USER_SET_FEATURES as puting this device into init state,
>> aka, zero all related structures. However, previous code
>> puts this message at last in the whole initialization process,
>> which leads to all previous information are zeroed.
>>
>> To fix this issue, we rearrange the sequence of those messages.
>>    - step 0, send VHOST_USER_SET_VRING_CALL so that vhost allocates
>>      virtqueue structures;
> Yes, it is. However, it's not that right to do that (you see there is
> a FIXME in vhost_user_set_vring_call()).

I suppose you are specifying vhost_set_vring_call().

>
> That means it need be fixed: we should not rely on fact that it's the
> first per-vring message we will get in the current QEMU implementation
> as the truth.
>
> That also means, naming a function like virtio_user_create_queue() based
> on above behaviour is wrong.

It's actually a good catch. After a light thought, I think in DPDK 
vhost, we may need to create those virtqueues once unix socket gets 
connected, just like in vhost-net, virtqueues are created on char file 
open. Right?

>
>>    - step 1, send VHOST_USER_SET_FEATURES to confirm the features;
>>    - step 2, send VHOST_USER_SET_MEM_TABLE to share mem regions;
>>    - step 3, send VHOST_USER_SET_VRING_NUM, VHOST_USER_SET_VRING_BASE,
>>      VHOST_USER_SET_VRING_ADDR, VHOST_USER_SET_VRING_KICK for each
>>      queue;
>>    - ...
>>
>> Fixes: 37a7eb2ae816 ("net/virtio-user: add device emulation layer")
>>
>> Reported-by: Zhihong Wang <zhihong.wang at intel.com>
>> Signed-off-by: Jianfeng Tan <jianfeng.tan at intel.com>
>> ---
>>   drivers/net/virtio/virtio_user/virtio_user_dev.c | 120 ++++++++++++++---------
>>   1 file changed, 72 insertions(+), 48 deletions(-)
> That's too much of code for a bug fix. I'm wondering how about just
> moving VHOST_USER_GET_PROTOCOL_FEATURES ahead, to the begining of
> virtio_user_start_device()? It should fix this issue.

Why does VHOST_USER_GET_PROTOCOL_FEATURES care? Do you mean shifting 
VHOST_USER_SET_FEATURES earlier?

Thanks,
Jianfeng

>
> 	--yliu