Re: [dpdk-dev] [PATCH 3/4] net/virtio-user: support server mode

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 02/28/2018 02:53 AM, Yang, Zhiyong wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Wednesday, February 28, 2018 2:02 AM
>> To: Yang, Zhiyong <zhiyong.yang@intel.com>; dev@dpdk.org;
>> yliu@fridaylinux.org; Tan, Jianfeng <jianfeng.tan@intel.com>; Bie, Tiwei
>> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>
>> Cc: Wang, Dong1 <dong1.wang@intel.com>
>> Subject: Re: [PATCH 3/4] net/virtio-user: support server mode
>>
>>
>>
>> On 02/14/2018 03:53 PM, Zhiyong Yang wrote:
>>> virtio user adds to support for server mode.
>>>
>>> Virtio user with server mode creates socket file and then starts to
>>> wait for first connection from vhost user with client mode in blocking mode.
>>>
>>> Server mode virtio user supports many times' vhost reconnections with
>>> same configurations.
>>>
>>> Support only one connection at the same time in server mode.
>>>
>>> Signed-off-by: Zhiyong Yang <zhiyong.yang@intel.com>
>>> ---
>>>    drivers/net/virtio/virtio_ethdev.c               |  9 ++-
>>>    drivers/net/virtio/virtio_user/vhost_user.c      | 77
>> ++++++++++++++++++++--
>>>    drivers/net/virtio/virtio_user/virtio_user_dev.c | 44 +++++++++----
>>>    drivers/net/virtio/virtio_user_ethdev.c          | 81
>> ++++++++++++++++++++++--
>>>    4 files changed, 186 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_ethdev.c
>>> b/drivers/net/virtio/virtio_ethdev.c
>>> index 884f74ad0..44d037d6b 100644
>>> --- a/drivers/net/virtio/virtio_ethdev.c
>>> +++ b/drivers/net/virtio/virtio_ethdev.c
>>> @@ -1273,9 +1273,13 @@ static void
>>>    virtio_notify_peers(struct rte_eth_dev *dev)
>>>    {
>>>    	struct virtio_hw *hw = dev->data->dev_private;
>>> -	struct virtnet_rx *rxvq = dev->data->rx_queues[0];
>>> +	struct virtnet_rx *rxvq = NULL;
>> I don't think it is needed to assign to NULL here.
> 
> Ok.
>>
>>>    	struct rte_mbuf *rarp_mbuf;
>>>
>>> +	if (!dev->data->rx_queues)
>>> +		return;
>>> +
>>> +	rxvq = dev->data->rx_queues[0];
>> The above change is valid, but I think it should be in a dedicated patch, as it
>> might be backported to -stable.
>>
> Ok, it will cause crash in some cases. For example, the code goes here before memory allocation of  rxvq is done.
> 
>>>    	rarp_mbuf = rte_net_make_rarp_packet(rxvq->mpool,
>>>    			(struct ether_addr *)hw->mac_addr);
>>>    	if (rarp_mbuf == NULL) {
>>> @@ -1333,7 +1337,8 @@ virtio_interrupt_handler(void *param)
>>>
>>>    	if (isr & VIRTIO_NET_S_ANNOUNCE) {
>>>    		virtio_notify_peers(dev);
>>> -		virtio_ack_link_announce(dev);
>>> +		if (hw->cvq) > +
>> 	virtio_ack_link_announce(dev);
>> Is this change also related to server mode?
>> It may deserve a dedicated patch too.
> 
> Above changes are not related to server mode. :).
> Just looks more reasonable.
> 
> If format extra two patches,  Should I put them in this series or in another series?

Thanks. Put them in the same series, with a Fixes: line and Cc:
stable@dpdk.org if the fixed patch was introduced before v17.11.


> Thanks
> Zhiyong
> 
>>>    	}
>>>    }
>>
>> Thanks,
>> Maxime