diff mbox series

[v2,3/3] virtio-user: set status on virtio-user reconnect

Message ID 20201020152052.389446-4-amorenoz@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series virtio-user: fix virtio-user server mode |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed

Commit Message

Adrian Moreno Oct. 20, 2020, 3:20 p.m. UTC 
Newer vhost-user backends will rely on SET_STATUS to start the device
so this required to support them.

Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
Cc: maxime.coquelin@redhat.com
Cc: stable@dpdk.org

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

Comments

Maxime Coquelin Oct. 20, 2020, 4:42 p.m. UTC | #1 
On 10/20/20 5:20 PM, Adrian Moreno wrote:
> Newer vhost-user backends will rely on SET_STATUS to start the device
> so this required to support them.
> 
> Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
> Cc: maxime.coquelin@redhat.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
> index e870fb2ff..d8bea4537 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>  		return -1;
>  
>  	dev->vhostfd = connectfd;
> +
> +	vtpci_reset(hw);
> +
> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
> +
> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
> +
>  	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>  				   &dev->device_features) < 0) {
>  		PMD_INIT_LOG(ERR, "get_features failed: %s",
> @@ -111,6 +118,8 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>  
>  	dev->features &= dev->device_features;
>  
> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
> +
>  	/* For packed ring, resetting queues is required in reconnection. */
>  	if (vtpci_packed_queue(hw) &&
>  	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
> @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>  		virtio_user_reset_queues_packed(eth_dev);
>  	}
>  
> -	ret = virtio_user_start_device(dev);
> -	if (ret < 0)
> +	/*  Start the device */
> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
> +	if (!dev->started)
>  		return -1;
>  
>  	if (dev->queue_pairs > 1) {
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime
Wang, Yinan Oct. 22, 2020, 4:01 a.m. UTC | #2 
Hi Maxime/ Adrian，

Thanks for the patch.  we can launch vhost-user with client mode with this fix patch.
But still fail to get throughput with basic vhost/virtio-user server mode loopback test. This is another problem which introduced by 57912824615fd7787a48a7b18e40661466.
Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=541

BR,
Yinan
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: 2020年10月21日 0:43
> To: Adrian Moreno <amorenoz@redhat.com>; dev@dpdk.org
> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick <patrick.fu@intel.com>;
> stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>
> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect
> 
> 
> 
> On 10/20/20 5:20 PM, Adrian Moreno wrote:
> > Newer vhost-user backends will rely on SET_STATUS to start the device
> > so this required to support them.
> >
> > Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
> > Cc: maxime.coquelin@redhat.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> > ---
> >  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> > index e870fb2ff..d8bea4537 100644
> > --- a/drivers/net/virtio/virtio_user_ethdev.c
> > +++ b/drivers/net/virtio/virtio_user_ethdev.c
> > @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct virtio_user_dev
> *dev)
> >  		return -1;
> >
> >  	dev->vhostfd = connectfd;
> > +
> > +	vtpci_reset(hw);
> > +
> > +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
> > +
> > +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
> > +
> >  	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> >  				   &dev->device_features) < 0) {
> >  		PMD_INIT_LOG(ERR, "get_features failed: %s",
> > @@ -111,6 +118,8 @@ virtio_user_server_reconnect(struct virtio_user_dev
> *dev)
> >
> >  	dev->features &= dev->device_features;
> >
> > +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
> > +
> >  	/* For packed ring, resetting queues is required in reconnection. */
> >  	if (vtpci_packed_queue(hw) &&
> >  	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
> > @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct virtio_user_dev
> *dev)
> >  		virtio_user_reset_queues_packed(eth_dev);
> >  	}
> >
> > -	ret = virtio_user_start_device(dev);
> > -	if (ret < 0)
> > +	/*  Start the device */
> > +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
> > +	if (!dev->started)
> >  		return -1;
> >
> >  	if (dev->queue_pairs > 1) {
> >
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Thanks,
> Maxime
Maxime Coquelin Oct. 22, 2020, 7:13 a.m. UTC | #3 
Hi Yinan,

On 10/22/20 6:01 AM, Wang, Yinan wrote:
> Hi Maxime/ Adrian，
> 
> Thanks for the patch.  we can launch vhost-user with client mode with this fix patch.
> But still fail to get throughput with basic vhost/virtio-user server mode loopback test. This is another problem which introduced by 57912824615fd7787a48a7b18e40661466.
> Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=541

Thanks for reporting the issue, I will look at it this morning.

BTW, this virtio-user server mode is broken by design, we should really
fix it.

For example, it assumes features to be supported by the backend before
the negotiation took place:
https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_user/virtio_user_dev.c#n513

As part of my rework, I suggest we implement the same behaviour as QEMU
does, which will be reliable and consistent with QEMU. It means that if
server mode is enabled in device command-line, the driver waits until
the socket is ready.

Chenbo, Adrian, what do you think?

Thanks,
Maxime

> BR,
> Yinan
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: 2020年10月21日 0:43
>> To: Adrian Moreno <amorenoz@redhat.com>; dev@dpdk.org
>> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick <patrick.fu@intel.com>;
>> stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Wang, Zhihong
>> <zhihong.wang@intel.com>
>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect
>>
>>
>>
>> On 10/20/20 5:20 PM, Adrian Moreno wrote:
>>> Newer vhost-user backends will rely on SET_STATUS to start the device
>>> so this required to support them.
>>>
>>> Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
>>> Cc: maxime.coquelin@redhat.com
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>> ---
>>>  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>> b/drivers/net/virtio/virtio_user_ethdev.c
>>> index e870fb2ff..d8bea4537 100644
>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>> @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct virtio_user_dev
>> *dev)
>>>  		return -1;
>>>
>>>  	dev->vhostfd = connectfd;
>>> +
>>> +	vtpci_reset(hw);
>>> +
>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
>>> +
>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
>>> +
>>>  	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>>>  				   &dev->device_features) < 0) {
>>>  		PMD_INIT_LOG(ERR, "get_features failed: %s",
>>> @@ -111,6 +118,8 @@ virtio_user_server_reconnect(struct virtio_user_dev
>> *dev)
>>>
>>>  	dev->features &= dev->device_features;
>>>
>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
>>> +
>>>  	/* For packed ring, resetting queues is required in reconnection. */
>>>  	if (vtpci_packed_queue(hw) &&
>>>  	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
>>> @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct virtio_user_dev
>> *dev)
>>>  		virtio_user_reset_queues_packed(eth_dev);
>>>  	}
>>>
>>> -	ret = virtio_user_start_device(dev);
>>> -	if (ret < 0)
>>> +	/*  Start the device */
>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
>>> +	if (!dev->started)
>>>  		return -1;
>>>
>>>  	if (dev->queue_pairs > 1) {
>>>
>>
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> Thanks,
>> Maxime
>
Chenbo Xia Oct. 22, 2020, 7:37 a.m. UTC | #4 
Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, October 22, 2020 3:14 PM
> To: Wang, Yinan <yinan.wang@intel.com>; Adrian Moreno
> <amorenoz@redhat.com>; dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: Fu, Patrick <patrick.fu@intel.com>; stable@dpdk.org; Wang, Zhihong
> <zhihong.wang@intel.com>; Xu, Qian Q <qian.q.xu@intel.com>
> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
> reconnect
> 
> Hi Yinan,
> 
> On 10/22/20 6:01 AM, Wang, Yinan wrote:
> > Hi Maxime/ Adrian，
> >
> > Thanks for the patch.  we can launch vhost-user with client mode with
> this fix patch.
> > But still fail to get throughput with basic vhost/virtio-user server
> mode loopback test. This is another problem which introduced by
> 57912824615fd7787a48a7b18e40661466.
> > Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=541
> 
> Thanks for reporting the issue, I will look at it this morning.
> 
> BTW, this virtio-user server mode is broken by design, we should really
> fix it.
> 
> For example, it assumes features to be supported by the backend before
> the negotiation took place:
> https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_user/virtio_user_
> dev.c#n513
> 
> As part of my rework, I suggest we implement the same behaviour as QEMU
> does, which will be reliable and consistent with QEMU. It means that if
> server mode is enabled in device command-line, the driver waits until
> the socket is ready.
> 
> Chenbo, Adrian, what do you think?

Yes! I totally agree and have the same opinion for a long time. As I remember, this
feature assumption has caused problems before and I notice the new STATUS feature is
also affected. It's good to make this consistent with QEMU 😊.

Cheers,
Chenbo

> 
> Thanks,
> Maxime
> 
> > BR,
> > Yinan
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: 2020年10月21日 0:43
> >> To: Adrian Moreno <amorenoz@redhat.com>; dev@dpdk.org
> >> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick
> <patrick.fu@intel.com>;
> >> stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Wang, Zhihong
> >> <zhihong.wang@intel.com>
> >> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
> reconnect
> >>
> >>
> >>
> >> On 10/20/20 5:20 PM, Adrian Moreno wrote:
> >>> Newer vhost-user backends will rely on SET_STATUS to start the device
> >>> so this required to support them.
> >>>
> >>> Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
> >>> Cc: maxime.coquelin@redhat.com
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> >>> ---
> >>>  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
> >>>  1 file changed, 12 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> >> b/drivers/net/virtio/virtio_user_ethdev.c
> >>> index e870fb2ff..d8bea4537 100644
> >>> --- a/drivers/net/virtio/virtio_user_ethdev.c
> >>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> >>> @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct virtio_user_dev
> >> *dev)
> >>>  		return -1;
> >>>
> >>>  	dev->vhostfd = connectfd;
> >>> +
> >>> +	vtpci_reset(hw);
> >>> +
> >>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
> >>> +
> >>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
> >>> +
> >>>  	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> >>>  				   &dev->device_features) < 0) {
> >>>  		PMD_INIT_LOG(ERR, "get_features failed: %s",
> >>> @@ -111,6 +118,8 @@ virtio_user_server_reconnect(struct
> virtio_user_dev
> >> *dev)
> >>>
> >>>  	dev->features &= dev->device_features;
> >>>
> >>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
> >>> +
> >>>  	/* For packed ring, resetting queues is required in reconnection. */
> >>>  	if (vtpci_packed_queue(hw) &&
> >>>  	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
> >>> @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct
> virtio_user_dev
> >> *dev)
> >>>  		virtio_user_reset_queues_packed(eth_dev);
> >>>  	}
> >>>
> >>> -	ret = virtio_user_start_device(dev);
> >>> -	if (ret < 0)
> >>> +	/*  Start the device */
> >>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
> >>> +	if (!dev->started)
> >>>  		return -1;
> >>>
> >>>  	if (dev->queue_pairs > 1) {
> >>>
> >>
> >> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>
> >> Thanks,
> >> Maxime
> >
Adrian Moreno Oct. 22, 2020, 8:12 a.m. UTC | #5 
On 10/22/20 9:37 AM, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, October 22, 2020 3:14 PM
>> To: Wang, Yinan <yinan.wang@intel.com>; Adrian Moreno
>> <amorenoz@redhat.com>; dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>
>> Cc: Fu, Patrick <patrick.fu@intel.com>; stable@dpdk.org; Wang, Zhihong
>> <zhihong.wang@intel.com>; Xu, Qian Q <qian.q.xu@intel.com>
>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
>> reconnect
>>
>> Hi Yinan,
>>
>> On 10/22/20 6:01 AM, Wang, Yinan wrote:
>>> Hi Maxime/ Adrian，
>>>
>>> Thanks for the patch.  we can launch vhost-user with client mode with
>> this fix patch.
>>> But still fail to get throughput with basic vhost/virtio-user server
>> mode loopback test. This is another problem which introduced by
>> 57912824615fd7787a48a7b18e40661466.
>>> Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=541
>>
>> Thanks for reporting the issue, I will look at it this morning.
>>
>> BTW, this virtio-user server mode is broken by design, we should really
>> fix it.
>>
>> For example, it assumes features to be supported by the backend before
>> the negotiation took place:
>> https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_user/virtio_user_
>> dev.c#n513
>>
>> As part of my rework, I suggest we implement the same behaviour as QEMU
>> does, which will be reliable and consistent with QEMU. It means that if
>> server mode is enabled in device command-line, the driver waits until
>> the socket is ready.
>>
>> Chenbo, Adrian, what do you think?
> 
> Yes! I totally agree and have the same opinion for a long time. As I remember, this
> feature assumption has caused problems before and I notice the new STATUS feature is
> also affected. It's good to make this consistent with QEMU 😊.
> 
+1

In fact, I was looking into how to get rid of some error messages originated
during the initialization of the virtio-pci layer when the socket is still not
ready.


> Cheers,
> Chenbo
> 
>>
>> Thanks,
>> Maxime
>>
>>> BR,
>>> Yinan
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: 2020年10月21日 0:43
>>>> To: Adrian Moreno <amorenoz@redhat.com>; dev@dpdk.org
>>>> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick
>> <patrick.fu@intel.com>;
>>>> stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Wang, Zhihong
>>>> <zhihong.wang@intel.com>
>>>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
>> reconnect
>>>>
>>>>
>>>>
>>>> On 10/20/20 5:20 PM, Adrian Moreno wrote:
>>>>> Newer vhost-user backends will rely on SET_STATUS to start the device
>>>>> so this required to support them.
>>>>>
>>>>> Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
>>>>> Cc: maxime.coquelin@redhat.com
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>>>> ---
>>>>>  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
>>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>>>> b/drivers/net/virtio/virtio_user_ethdev.c
>>>>> index e870fb2ff..d8bea4537 100644
>>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>>>> @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct virtio_user_dev
>>>> *dev)
>>>>>  		return -1;
>>>>>
>>>>>  	dev->vhostfd = connectfd;
>>>>> +
>>>>> +	vtpci_reset(hw);
>>>>> +
>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
>>>>> +
>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
>>>>> +
>>>>>  	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>>>>>  				   &dev->device_features) < 0) {
>>>>>  		PMD_INIT_LOG(ERR, "get_features failed: %s",
>>>>> @@ -111,6 +118,8 @@ virtio_user_server_reconnect(struct
>> virtio_user_dev
>>>> *dev)
>>>>>
>>>>>  	dev->features &= dev->device_features;
>>>>>
>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
>>>>> +
>>>>>  	/* For packed ring, resetting queues is required in reconnection. */
>>>>>  	if (vtpci_packed_queue(hw) &&
>>>>>  	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
>>>>> @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct
>> virtio_user_dev
>>>> *dev)
>>>>>  		virtio_user_reset_queues_packed(eth_dev);
>>>>>  	}
>>>>>
>>>>> -	ret = virtio_user_start_device(dev);
>>>>> -	if (ret < 0)
>>>>> +	/*  Start the device */
>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
>>>>> +	if (!dev->started)
>>>>>  		return -1;
>>>>>
>>>>>  	if (dev->queue_pairs > 1) {
>>>>>
>>>>
>>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>
>>>> Thanks,
>>>> Maxime
>>>
>
Maxime Coquelin Oct. 22, 2020, 8:32 a.m. UTC | #6 
On 10/22/20 10:12 AM, Adrian Moreno wrote:
> 
> 
> On 10/22/20 9:37 AM, Xia, Chenbo wrote:
>> Hi Maxime,
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Sent: Thursday, October 22, 2020 3:14 PM
>>> To: Wang, Yinan <yinan.wang@intel.com>; Adrian Moreno
>>> <amorenoz@redhat.com>; dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>
>>> Cc: Fu, Patrick <patrick.fu@intel.com>; stable@dpdk.org; Wang, Zhihong
>>> <zhihong.wang@intel.com>; Xu, Qian Q <qian.q.xu@intel.com>
>>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
>>> reconnect
>>>
>>> Hi Yinan,
>>>
>>> On 10/22/20 6:01 AM, Wang, Yinan wrote:
>>>> Hi Maxime/ Adrian，
>>>>
>>>> Thanks for the patch.  we can launch vhost-user with client mode with
>>> this fix patch.
>>>> But still fail to get throughput with basic vhost/virtio-user server
>>> mode loopback test. This is another problem which introduced by
>>> 57912824615fd7787a48a7b18e40661466.
>>>> Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=541
>>>
>>> Thanks for reporting the issue, I will look at it this morning.
>>>
>>> BTW, this virtio-user server mode is broken by design, we should really
>>> fix it.
>>>
>>> For example, it assumes features to be supported by the backend before
>>> the negotiation took place:
>>> https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_user/virtio_user_
>>> dev.c#n513
>>>
>>> As part of my rework, I suggest we implement the same behaviour as QEMU
>>> does, which will be reliable and consistent with QEMU. It means that if
>>> server mode is enabled in device command-line, the driver waits until
>>> the socket is ready.
>>>
>>> Chenbo, Adrian, what do you think?
>>
>> Yes! I totally agree and have the same opinion for a long time. As I remember, this
>> feature assumption has caused problems before and I notice the new STATUS feature is
>> also affected. It's good to make this consistent with QEMU 😊.
>>
> +1
> 
> In fact, I was looking into how to get rid of some error messages originated
> during the initialization of the virtio-pci layer when the socket is still not
> ready.

Is that the error messages you meant?


virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1): Success
virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1): Success
virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1): Success
virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1): Success
virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1): Success


That would be good to have them fixed

> 
>> Cheers,
>> Chenbo
>>
>>>
>>> Thanks,
>>> Maxime
>>>
>>>> BR,
>>>> Yinan
>>>>> -----Original Message-----
>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>> Sent: 2020年10月21日 0:43
>>>>> To: Adrian Moreno <amorenoz@redhat.com>; dev@dpdk.org
>>>>> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick
>>> <patrick.fu@intel.com>;
>>>>> stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Wang, Zhihong
>>>>> <zhihong.wang@intel.com>
>>>>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
>>> reconnect
>>>>>
>>>>>
>>>>>
>>>>> On 10/20/20 5:20 PM, Adrian Moreno wrote:
>>>>>> Newer vhost-user backends will rely on SET_STATUS to start the device
>>>>>> so this required to support them.
>>>>>>
>>>>>> Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
>>>>>> Cc: maxime.coquelin@redhat.com
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>>>>> ---
>>>>>>  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
>>>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>>>>> b/drivers/net/virtio/virtio_user_ethdev.c
>>>>>> index e870fb2ff..d8bea4537 100644
>>>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>>>>> @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct virtio_user_dev
>>>>> *dev)
>>>>>>  		return -1;
>>>>>>
>>>>>>  	dev->vhostfd = connectfd;
>>>>>> +
>>>>>> +	vtpci_reset(hw);
>>>>>> +
>>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
>>>>>> +
>>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
>>>>>> +
>>>>>>  	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>>>>>>  				   &dev->device_features) < 0) {
>>>>>>  		PMD_INIT_LOG(ERR, "get_features failed: %s",
>>>>>> @@ -111,6 +118,8 @@ virtio_user_server_reconnect(struct
>>> virtio_user_dev
>>>>> *dev)
>>>>>>
>>>>>>  	dev->features &= dev->device_features;
>>>>>>
>>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
>>>>>> +
>>>>>>  	/* For packed ring, resetting queues is required in reconnection. */
>>>>>>  	if (vtpci_packed_queue(hw) &&
>>>>>>  	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
>>>>>> @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct
>>> virtio_user_dev
>>>>> *dev)
>>>>>>  		virtio_user_reset_queues_packed(eth_dev);
>>>>>>  	}
>>>>>>
>>>>>> -	ret = virtio_user_start_device(dev);
>>>>>> -	if (ret < 0)
>>>>>> +	/*  Start the device */
>>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
>>>>>> +	if (!dev->started)
>>>>>>  		return -1;
>>>>>>
>>>>>>  	if (dev->queue_pairs > 1) {
>>>>>>
>>>>>
>>>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>
>>>>> Thanks,
>>>>> Maxime
>>>>
>>
>
Jiang, YuX Oct. 23, 2020, 5:21 a.m. UTC | #7 
Tested-by: JiangYuX <yux.jiang@intel.com>

    Best Regards
    Jiang yu


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
> Sent: Thursday, October 22, 2020 4:32 PM
> To: Adrian Moreno <amorenoz@redhat.com>; Xia, Chenbo
> <chenbo.xia@intel.com>; Wang, Yinan <yinan.wang@intel.com>;
> dev@dpdk.org
> Cc: Fu, Patrick <patrick.fu@intel.com>; stable@dpdk.org; Wang, Zhihong
> <zhihong.wang@intel.com>; Xu, Qian Q <qian.q.xu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user
> reconnect
> 
> 
> 
> On 10/22/20 10:12 AM, Adrian Moreno wrote:
> >
> >
> > On 10/22/20 9:37 AM, Xia, Chenbo wrote:
> >> Hi Maxime,
> >>
> >>> -----Original Message-----
> >>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>> Sent: Thursday, October 22, 2020 3:14 PM
> >>> To: Wang, Yinan <yinan.wang@intel.com>; Adrian Moreno
> >>> <amorenoz@redhat.com>; dev@dpdk.org; Xia, Chenbo
> >>> <chenbo.xia@intel.com>
> >>> Cc: Fu, Patrick <patrick.fu@intel.com>; stable@dpdk.org; Wang,
> >>> Zhihong <zhihong.wang@intel.com>; Xu, Qian Q <qian.q.xu@intel.com>
> >>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
> >>> reconnect
> >>>
> >>> Hi Yinan,
> >>>
> >>> On 10/22/20 6:01 AM, Wang, Yinan wrote:
> >>>> Hi Maxime/ Adrian，
> >>>>
> >>>> Thanks for the patch.  we can launch vhost-user with client mode
> >>>> with
> >>> this fix patch.
> >>>> But still fail to get throughput with basic vhost/virtio-user
> >>>> server
> >>> mode loopback test. This is another problem which introduced by
> >>> 57912824615fd7787a48a7b18e40661466.
> >>>> Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=541
> >>>
> >>> Thanks for reporting the issue, I will look at it this morning.
> >>>
> >>> BTW, this virtio-user server mode is broken by design, we should
> >>> really fix it.
> >>>
> >>> For example, it assumes features to be supported by the backend
> >>> before the negotiation took place:
> >>> https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_user/virtio
> >>> _user_
> >>> dev.c#n513
> >>>
> >>> As part of my rework, I suggest we implement the same behaviour as
> >>> QEMU does, which will be reliable and consistent with QEMU. It means
> >>> that if server mode is enabled in device command-line, the driver
> >>> waits until the socket is ready.
> >>>
> >>> Chenbo, Adrian, what do you think?
> >>
> >> Yes! I totally agree and have the same opinion for a long time. As I
> >> remember, this feature assumption has caused problems before and I
> >> notice the new STATUS feature is also affected. It's good to make this
> consistent with QEMU 😊.
> >>
> > +1
> >
> > In fact, I was looking into how to get rid of some error messages
> > originated during the initialization of the virtio-pci layer when the
> > socket is still not ready.
> 
> Is that the error messages you meant?
> 
> 
> virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1):
> Success
> virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
> virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
> virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1):
> Success
> virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
> virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1):
> Success
> virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
> virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1):
> Success
> virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
> virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
> virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1):
> Success
> 
> 
> That would be good to have them fixed
> 
> >
> >> Cheers,
> >> Chenbo
> >>
> >>>
> >>> Thanks,
> >>> Maxime
> >>>
> >>>> BR,
> >>>> Yinan
> >>>>> -----Original Message-----
> >>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>> Sent: 2020年10月21日 0:43
> >>>>> To: Adrian Moreno <amorenoz@redhat.com>; dev@dpdk.org
> >>>>> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick
> >>> <patrick.fu@intel.com>;
> >>>>> stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Wang,
> Zhihong
> >>>>> <zhihong.wang@intel.com>
> >>>>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
> >>> reconnect
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 10/20/20 5:20 PM, Adrian Moreno wrote:
> >>>>>> Newer vhost-user backends will rely on SET_STATUS to start the
> >>>>>> device so this required to support them.
> >>>>>>
> >>>>>> Fixes: 57912824615f ("net/virtio-user: support vhost status
> >>>>>> setting")
> >>>>>> Cc: maxime.coquelin@redhat.com
> >>>>>> Cc: stable@dpdk.org
> >>>>>>
> >>>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> >>>>>> ---
> >>>>>>  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
> >>>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> >>>>> b/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>> index e870fb2ff..d8bea4537 100644
> >>>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>> @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct
> >>>>>> virtio_user_dev
> >>>>> *dev)
> >>>>>>  		return -1;
> >>>>>>
> >>>>>>  	dev->vhostfd = connectfd;
> >>>>>> +
> >>>>>> +	vtpci_reset(hw);
> >>>>>> +
> >>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
> >>>>>> +
> >>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
> >>>>>> +
> >>>>>>  	if (dev->ops->send_request(dev,
> VHOST_USER_GET_FEATURES,
> >>>>>>  				   &dev->device_features) < 0) {
> >>>>>>  		PMD_INIT_LOG(ERR, "get_features failed: %s", @@ -
> 111,6 +118,8
> >>>>>> @@ virtio_user_server_reconnect(struct
> >>> virtio_user_dev
> >>>>> *dev)
> >>>>>>
> >>>>>>  	dev->features &= dev->device_features;
> >>>>>>
> >>>>>> +	vtpci_set_status(hw,
> VIRTIO_CONFIG_STATUS_FEATURES_OK);
> >>>>>> +
> >>>>>>  	/* For packed ring, resetting queues is required in
> reconnection. */
> >>>>>>  	if (vtpci_packed_queue(hw) &&
> >>>>>>  	   (vtpci_get_status(hw) &
> VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
> >>>>>> @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct
> >>> virtio_user_dev
> >>>>> *dev)
> >>>>>>  		virtio_user_reset_queues_packed(eth_dev);
> >>>>>>  	}
> >>>>>>
> >>>>>> -	ret = virtio_user_start_device(dev);
> >>>>>> -	if (ret < 0)
> >>>>>> +	/*  Start the device */
> >>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
> >>>>>> +	if (!dev->started)
> >>>>>>  		return -1;
> >>>>>>
> >>>>>>  	if (dev->queue_pairs > 1) {
> >>>>>>
> >>>>>
> >>>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>>
> >>>>> Thanks,
> >>>>> Maxime
> >>>>
> >>
> >
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index e870fb2ff..d8bea4537 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -78,6 +78,13 @@  virtio_user_server_reconnect(struct virtio_user_dev *dev)
 		return -1;
 
 	dev->vhostfd = connectfd;
+
+	vtpci_reset(hw);
+
+	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
+
+	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
+
 	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
 				   &dev->device_features) < 0) {
 		PMD_INIT_LOG(ERR, "get_features failed: %s",
@@ -111,6 +118,8 @@  virtio_user_server_reconnect(struct virtio_user_dev *dev)
 
 	dev->features &= dev->device_features;
 
+	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
+
 	/* For packed ring, resetting queues is required in reconnection. */
 	if (vtpci_packed_queue(hw) &&
 	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
@@ -119,8 +128,9 @@  virtio_user_server_reconnect(struct virtio_user_dev *dev)
 		virtio_user_reset_queues_packed(eth_dev);
 	}
 
-	ret = virtio_user_start_device(dev);
-	if (ret < 0)
+	/*  Start the device */
+	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
+	if (!dev->started)
 		return -1;
 
 	if (dev->queue_pairs > 1) {