* [dpdk-dev] [PATCH v2 0/3] virtio-user: fix virtio-user server mode
@ 2020-10-20 15:20 Adrian Moreno
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 1/3] virtio-user: fix backend selection if stat fails Adrian Moreno
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Adrian Moreno @ 2020-10-20 15:20 UTC (permalink / raw)
To: dev; +Cc: yinan.wang, patrick.fu, amorenoz
This series addresses some issues identified in the virtio-user driver
when configured in server mode.
Firstly, properly identify the backend type in this mode.
Secondly, ensure no get/set status commands are sent before protocol
feature negotiation to avoid sending them to a backend that does not support them
And finally, add get/set status as per virtio spec so that it can work with
modern vhost-user backends that rely on this message to start the device.
--
v1->v2:
Added patch 2 and 3 addressing additional issues
Check errno to select vhost-user backend and log the detected backend
type
Adrian Moreno (3):
virtio-user: fix backend selection if stat fails
virtio_user: don't set/get_status until FEATURES_OK
virtio-user: set status on virtio-user reconnect
drivers/net/virtio/virtio_user/vhost_user.c | 10 ++++++----
.../net/virtio/virtio_user/virtio_user_dev.c | 7 +++++++
.../net/virtio/virtio_user/virtio_user_dev.h | 1 +
drivers/net/virtio/virtio_user_ethdev.c | 20 ++++++++++++++++---
4 files changed, 31 insertions(+), 7 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [dpdk-dev] [PATCH v2 1/3] virtio-user: fix backend selection if stat fails
2020-10-20 15:20 [dpdk-dev] [PATCH v2 0/3] virtio-user: fix virtio-user server mode Adrian Moreno
@ 2020-10-20 15:20 ` Adrian Moreno
2020-10-20 15:26 ` Maxime Coquelin
2020-10-20 16:52 ` Kevin Traynor
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 2/3] virtio_user: don't set/get_status until FEATURES_OK Adrian Moreno
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect Adrian Moreno
2 siblings, 2 replies; 14+ messages in thread
From: Adrian Moreno @ 2020-10-20 15:20 UTC (permalink / raw)
To: dev
Cc: yinan.wang, patrick.fu, amorenoz, stable, Maxime Coquelin,
Chenbo Xia, Zhihong Wang
If stat fails because the file does not exist, it means that
the backend must be vhost-user in server mode.
Also, log the detected backend type.
Bugzilla ID: 559
Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
Cc: stable@dpdk.org
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
drivers/net/virtio/virtio_user/virtio_user_dev.c | 7 +++++++
drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
drivers/net/virtio/virtio_user_ethdev.c | 6 +++++-
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 3681758ef..27814eadb 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -22,6 +22,13 @@
#define VIRTIO_USER_MEM_EVENT_CLB_NAME "virtio_user_mem_event_clb"
+const char * const virtio_user_backend_strings[] = {
+ [VIRTIO_USER_BACKEND_UNKNOWN] = "VIRTIO_USER_BACKEND_UNKNOWN",
+ [VIRTIO_USER_BACKEND_VHOST_USER] = "VHOST_USER",
+ [VIRTIO_USER_BACKEND_VHOST_KERNEL] = "VHOST_NET",
+ [VIRTIO_USER_BACKEND_VHOST_VDPA] = "VHOST_VDPA",
+};
+
static int
virtio_user_create_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
{
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index 3e9d1a1eb..998986875 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -83,4 +83,5 @@ void virtio_user_handle_cq_packed(struct virtio_user_dev *dev,
uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs);
int virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t status);
int virtio_user_update_status(struct virtio_user_dev *dev);
+extern const char * const virtio_user_backend_strings[];
#endif
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 042665bc0..e870fb2ff 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -560,6 +560,9 @@ virtio_user_backend_type(const char *path)
struct stat sb;
if (stat(path, &sb) == -1) {
+ if (errno == ENOENT)
+ return VIRTIO_USER_BACKEND_VHOST_USER;
+
PMD_INIT_LOG(ERR, "Stat fails: %s (%s)\n", path,
strerror(errno));
return VIRTIO_USER_BACKEND_UNKNOWN;
@@ -697,7 +700,8 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
path);
goto end;
}
-
+ PMD_INIT_LOG(INFO, "Backend type detected: %s",
+ virtio_user_backend_strings[backend_type]);
if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_INTERFACE_NAME) == 1) {
if (backend_type != VIRTIO_USER_BACKEND_VHOST_KERNEL) {
--
2.26.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [dpdk-dev] [PATCH v2 2/3] virtio_user: don't set/get_status until FEATURES_OK
2020-10-20 15:20 [dpdk-dev] [PATCH v2 0/3] virtio-user: fix virtio-user server mode Adrian Moreno
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 1/3] virtio-user: fix backend selection if stat fails Adrian Moreno
@ 2020-10-20 15:20 ` Adrian Moreno
2020-10-20 16:17 ` Maxime Coquelin
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect Adrian Moreno
2 siblings, 1 reply; 14+ messages in thread
From: Adrian Moreno @ 2020-10-20 15:20 UTC (permalink / raw)
To: dev
Cc: yinan.wang, patrick.fu, amorenoz, maxime.coquelin, stable,
Chenbo Xia, Zhihong Wang
According to the virtio spec, ACK and DRIVER status bits should be set
before feature negotiation.
However, until the protocol features are negotiated, the driver does not
know if the device actually supports the those vhost-user messages.
Therefore, until FEATURES_OK is set, the GET/SET_STATUS messages should
not be sent.
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/vhost_user.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index ef290c357..450d77e92 100644
--- a/drivers/net/virtio/virtio_user/vhost_user.c
+++ b/drivers/net/virtio/virtio_user/vhost_user.c
@@ -278,8 +278,9 @@ vhost_user_sock(struct virtio_user_dev *dev,
switch (req) {
case VHOST_USER_GET_STATUS:
- if (!(dev->protocol_features &
- (1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
+ if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) ||
+ (!(dev->protocol_features &
+ (1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
return 0;
/* Fallthrough */
case VHOST_USER_GET_FEATURES:
@@ -288,8 +289,9 @@ vhost_user_sock(struct virtio_user_dev *dev,
break;
case VHOST_USER_SET_STATUS:
- if (!(dev->protocol_features &
- (1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
+ if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) ||
+ (!(dev->protocol_features &
+ (1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
return 0;
if (has_reply_ack)
--
2.26.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect
2020-10-20 15:20 [dpdk-dev] [PATCH v2 0/3] virtio-user: fix virtio-user server mode Adrian Moreno
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 1/3] virtio-user: fix backend selection if stat fails Adrian Moreno
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 2/3] virtio_user: don't set/get_status until FEATURES_OK Adrian Moreno
@ 2020-10-20 15:20 ` Adrian Moreno
2020-10-20 16:42 ` Maxime Coquelin
2 siblings, 1 reply; 14+ messages in thread
From: Adrian Moreno @ 2020-10-20 15:20 UTC (permalink / raw)
To: dev
Cc: yinan.wang, patrick.fu, amorenoz, maxime.coquelin, stable,
Chenbo Xia, Zhihong Wang
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) {
--
2.26.2
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v2 1/3] virtio-user: fix backend selection if stat fails
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 1/3] virtio-user: fix backend selection if stat fails Adrian Moreno
@ 2020-10-20 15:26 ` Maxime Coquelin
2020-10-20 16:52 ` Kevin Traynor
1 sibling, 0 replies; 14+ messages in thread
From: Maxime Coquelin @ 2020-10-20 15:26 UTC (permalink / raw)
To: Adrian Moreno, dev
Cc: yinan.wang, patrick.fu, stable, Chenbo Xia, Zhihong Wang
On 10/20/20 5:20 PM, Adrian Moreno wrote:
> If stat fails because the file does not exist, it means that
> the backend must be vhost-user in server mode.
>
> Also, log the detected backend type.
>
> Bugzilla ID: 559
> Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
> Cc: stable@dpdk.org
>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
> drivers/net/virtio/virtio_user/virtio_user_dev.c | 7 +++++++
> drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
> drivers/net/virtio/virtio_user_ethdev.c | 6 +++++-
> 3 files changed, 13 insertions(+), 1 deletion(-)
>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v2 2/3] virtio_user: don't set/get_status until FEATURES_OK
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 2/3] virtio_user: don't set/get_status until FEATURES_OK Adrian Moreno
@ 2020-10-20 16:17 ` Maxime Coquelin
0 siblings, 0 replies; 14+ messages in thread
From: Maxime Coquelin @ 2020-10-20 16:17 UTC (permalink / raw)
To: Adrian Moreno, dev
Cc: yinan.wang, patrick.fu, stable, Chenbo Xia, Zhihong Wang
On 10/20/20 5:20 PM, Adrian Moreno wrote:
> According to the virtio spec, ACK and DRIVER status bits should be set
> before feature negotiation.
>
> However, until the protocol features are negotiated, the driver does not
> know if the device actually supports the those vhost-user messages.
s/the those/those/
> Therefore, until FEATURES_OK is set, the GET/SET_STATUS messages should
> not be sent.
>
> 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/vhost_user.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
> index ef290c357..450d77e92 100644
> --- a/drivers/net/virtio/virtio_user/vhost_user.c
> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
> @@ -278,8 +278,9 @@ vhost_user_sock(struct virtio_user_dev *dev,
>
> switch (req) {
> case VHOST_USER_GET_STATUS:
> - if (!(dev->protocol_features &
> - (1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
> + if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) ||
> + (!(dev->protocol_features &
> + (1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
> return 0;
> /* Fallthrough */
> case VHOST_USER_GET_FEATURES:
> @@ -288,8 +289,9 @@ vhost_user_sock(struct virtio_user_dev *dev,
> break;
>
> case VHOST_USER_SET_STATUS:
> - if (!(dev->protocol_features &
> - (1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
> + if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) ||
> + (!(dev->protocol_features &
> + (1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
> return 0;
>
> if (has_reply_ack)
>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect Adrian Moreno
@ 2020-10-20 16:42 ` Maxime Coquelin
2020-10-22 4:01 ` Wang, Yinan
0 siblings, 1 reply; 14+ messages in thread
From: Maxime Coquelin @ 2020-10-20 16:42 UTC (permalink / raw)
To: Adrian Moreno, dev
Cc: yinan.wang, patrick.fu, stable, Chenbo Xia, Zhihong Wang
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v2 1/3] virtio-user: fix backend selection if stat fails
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 1/3] virtio-user: fix backend selection if stat fails Adrian Moreno
2020-10-20 15:26 ` Maxime Coquelin
@ 2020-10-20 16:52 ` Kevin Traynor
1 sibling, 0 replies; 14+ messages in thread
From: Kevin Traynor @ 2020-10-20 16:52 UTC (permalink / raw)
To: Adrian Moreno, dev
Cc: yinan.wang, patrick.fu, stable, Maxime Coquelin, Chenbo Xia,
Zhihong Wang
On 20/10/2020 16:20, Adrian Moreno wrote:
> If stat fails because the file does not exist, it means that
> the backend must be vhost-user in server mode.
>
> Also, log the detected backend type.
>
> Bugzilla ID: 559
> Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
> Cc: stable@dpdk.org
>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
> drivers/net/virtio/virtio_user/virtio_user_dev.c | 7 +++++++
> drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
> drivers/net/virtio/virtio_user_ethdev.c | 6 +++++-
> 3 files changed, 13 insertions(+), 1 deletion(-)
Acked-by: Kevin Traynor <ktraynor@redhat.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect
2020-10-20 16:42 ` Maxime Coquelin
@ 2020-10-22 4:01 ` Wang, Yinan
2020-10-22 7:13 ` Maxime Coquelin
0 siblings, 1 reply; 14+ messages in thread
From: Wang, Yinan @ 2020-10-22 4:01 UTC (permalink / raw)
To: Maxime Coquelin, Adrian Moreno, dev
Cc: Fu, Patrick, stable, Xia, Chenbo, Wang, Zhihong, Xu, Qian Q
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect
2020-10-22 4:01 ` Wang, Yinan
@ 2020-10-22 7:13 ` Maxime Coquelin
2020-10-22 7:37 ` Xia, Chenbo
0 siblings, 1 reply; 14+ messages in thread
From: Maxime Coquelin @ 2020-10-22 7:13 UTC (permalink / raw)
To: Wang, Yinan, Adrian Moreno, dev, Xia, Chenbo
Cc: Fu, Patrick, stable, Wang, Zhihong, Xu, Qian Q
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
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect
2020-10-22 7:13 ` Maxime Coquelin
@ 2020-10-22 7:37 ` Xia, Chenbo
2020-10-22 8:12 ` Adrian Moreno
0 siblings, 1 reply; 14+ messages in thread
From: Xia, Chenbo @ 2020-10-22 7:37 UTC (permalink / raw)
To: Maxime Coquelin, Wang, Yinan, Adrian Moreno, dev
Cc: Fu, Patrick, stable, Wang, Zhihong, Xu, Qian Q
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
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect
2020-10-22 7:37 ` Xia, Chenbo
@ 2020-10-22 8:12 ` Adrian Moreno
2020-10-22 8:32 ` Maxime Coquelin
0 siblings, 1 reply; 14+ messages in thread
From: Adrian Moreno @ 2020-10-22 8:12 UTC (permalink / raw)
To: Xia, Chenbo, Maxime Coquelin, Wang, Yinan, dev
Cc: Fu, Patrick, stable, Wang, Zhihong, Xu, Qian Q
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
>>>
>
--
Adrián Moreno
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect
2020-10-22 8:12 ` Adrian Moreno
@ 2020-10-22 8:32 ` Maxime Coquelin
2020-10-23 5:21 ` Jiang, YuX
0 siblings, 1 reply; 14+ messages in thread
From: Maxime Coquelin @ 2020-10-22 8:32 UTC (permalink / raw)
To: Adrian Moreno, Xia, Chenbo, Wang, Yinan, dev
Cc: Fu, Patrick, stable, Wang, Zhihong, Xu, Qian Q
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
>>>>
>>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect
2020-10-22 8:32 ` Maxime Coquelin
@ 2020-10-23 5:21 ` Jiang, YuX
0 siblings, 0 replies; 14+ messages in thread
From: Jiang, YuX @ 2020-10-23 5:21 UTC (permalink / raw)
To: Maxime Coquelin, Adrian Moreno, Xia, Chenbo, Wang, Yinan, dev
Cc: Fu, Patrick, stable, Wang, Zhihong, Xu, Qian Q, Jiang, YuX
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
> >>>>
> >>
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-10-23 5:21 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-20 15:20 [dpdk-dev] [PATCH v2 0/3] virtio-user: fix virtio-user server mode Adrian Moreno
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 1/3] virtio-user: fix backend selection if stat fails Adrian Moreno
2020-10-20 15:26 ` Maxime Coquelin
2020-10-20 16:52 ` Kevin Traynor
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 2/3] virtio_user: don't set/get_status until FEATURES_OK Adrian Moreno
2020-10-20 16:17 ` Maxime Coquelin
2020-10-20 15:20 ` [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect Adrian Moreno
2020-10-20 16:42 ` Maxime Coquelin
2020-10-22 4:01 ` Wang, Yinan
2020-10-22 7:13 ` Maxime Coquelin
2020-10-22 7:37 ` Xia, Chenbo
2020-10-22 8:12 ` Adrian Moreno
2020-10-22 8:32 ` Maxime Coquelin
2020-10-23 5:21 ` Jiang, YuX
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).