[dpdk-dev] net/virtio-user: fix feature setting with vhost-net backend
Checks
Commit Message
Currently, when the backend is vhost-net, which implies
virtio-user works in client mode, virtio-user is assigned
to the default feature VIRTIO_USER_SUPPORTED_FEATURES.
However, virtio-user should request features from the
backend in this case.
Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
---
drivers/net/virtio/virtio_user/virtio_user_dev.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
Comments
On Mon, May 07, 2018 at 02:50:06PM +0800, Jiayu Hu wrote:
> Currently, when the backend is vhost-net, which implies
> virtio-user works in client mode, virtio-user is assigned
> to the default feature VIRTIO_USER_SUPPORTED_FEATURES.
> However, virtio-user should request features from the
> backend in this case.
>
> Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
> Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> ---
> drivers/net/virtio/virtio_user/virtio_user_dev.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 38b8bc9..f745163 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -353,20 +353,22 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
> return -1;
> }
>
> - if (dev->vhostfd >= 0) {
> + if (!dev->is_server) {
Server mode also needs to run below code when the
connection to the backend is established.
> if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER,
> - NULL) < 0) {
> + NULL) < 0) {
There is no need to change the indent.
> PMD_INIT_LOG(ERR, "set_owner fails: %s",
> - strerror(errno));
> + strerror(errno));
There is no need to change the indent.
> return -1;
> }
>
> if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> - &dev->device_features) < 0) {
> + &dev->device_features) < 0) {
There is no need to change the indent.
> PMD_INIT_LOG(ERR, "get_features failed: %s",
> - strerror(errno));
> + strerror(errno));
There is no need to change the indent.
> return -1;
> }
> + if (dev->mac_specified)
> + dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
Why move above code?
> } else {
> /* We just pretend vhost-user can support all these features.
> * Note that this could be problematic that if some feature is
> @@ -376,9 +378,6 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
> dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;
> }
>
> - if (dev->mac_specified)
> - dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
> -
> if (cq) {
> /* device does not really need to know anything about CQ,
> * so if necessary, we just claim to support CQ
> --
> 2.7.4
>
Hi Tiwei,
On Tue, May 08, 2018 at 10:49:50AM +0800, Tiwei Bie wrote:
> On Mon, May 07, 2018 at 02:50:06PM +0800, Jiayu Hu wrote:
> > Currently, when the backend is vhost-net, which implies
> > virtio-user works in client mode, virtio-user is assigned
> > to the default feature VIRTIO_USER_SUPPORTED_FEATURES.
> > However, virtio-user should request features from the
> > backend in this case.
> >
> > Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
> > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > ---
> > drivers/net/virtio/virtio_user/virtio_user_dev.c | 15 +++++++--------
> > 1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > index 38b8bc9..f745163 100644
> > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > @@ -353,20 +353,22 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
> > return -1;
> > }
> >
> > - if (dev->vhostfd >= 0) {
> > + if (!dev->is_server) {
>
> Server mode also needs to run below code when the
> connection to the backend is established.
For server mode, the below code is called by virtio_user_start_device(),
instead of virtio_user_dev_init(). Only client mode virtio-user
needs to call it when init.
>
> > if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER,
> > - NULL) < 0) {
> > + NULL) < 0) {
>
> There is no need to change the indent.
Thanks, change in the next patch.
>
> > PMD_INIT_LOG(ERR, "set_owner fails: %s",
> > - strerror(errno));
> > + strerror(errno));
>
> There is no need to change the indent.
ditto.
>
> > return -1;
> > }
> >
> > if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> > - &dev->device_features) < 0) {
> > + &dev->device_features) < 0) {
>
> There is no need to change the indent.
ditto.
>
> > PMD_INIT_LOG(ERR, "get_features failed: %s",
> > - strerror(errno));
> > + strerror(errno));
>
> There is no need to change the indent.
ditto.
>
> > return -1;
> > }
> > + if (dev->mac_specified)
> > + dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
>
> Why move above code?
The server mode virtio-user is assigned the default features, which include
VIRTIO_NET_F_MAC. So checking if support VIRTIO_NET_F_MAC is only necessary
for client mode.
Thanks,
Jiayu
>
> > } else {
> > /* We just pretend vhost-user can support all these features.
> > * Note that this could be problematic that if some feature is
> > @@ -376,9 +378,6 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
> > dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;
> > }
> >
> > - if (dev->mac_specified)
> > - dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
> > -
> > if (cq) {
> > /* device does not really need to know anything about CQ,
> > * so if necessary, we just claim to support CQ
> > --
> > 2.7.4
> >
On Tue, May 08, 2018 at 03:44:22PM +0800, Jiayu Hu wrote:
> Hi Tiwei,
>
> On Tue, May 08, 2018 at 10:49:50AM +0800, Tiwei Bie wrote:
> > On Mon, May 07, 2018 at 02:50:06PM +0800, Jiayu Hu wrote:
> > > Currently, when the backend is vhost-net, which implies
> > > virtio-user works in client mode, virtio-user is assigned
> > > to the default feature VIRTIO_USER_SUPPORTED_FEATURES.
> > > However, virtio-user should request features from the
> > > backend in this case.
> > >
> > > Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
> > > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > > ---
> > > drivers/net/virtio/virtio_user/virtio_user_dev.c | 15 +++++++--------
> > > 1 file changed, 7 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > > index 38b8bc9..f745163 100644
> > > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > > @@ -353,20 +353,22 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
> > > return -1;
> > > }
> > >
> > > - if (dev->vhostfd >= 0) {
> > > + if (!dev->is_server) {
> >
> > Server mode also needs to run below code when the
> > connection to the backend is established.
>
> For server mode, the below code is called by virtio_user_start_device(),
> instead of virtio_user_dev_init(). Only client mode virtio-user
> needs to call it when init.
Okay. So in server mode, virtio-user never get a chance
to do GET_FEATURES.
>
> >
> > > if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER,
> > > - NULL) < 0) {
> > > + NULL) < 0) {
> >
> > There is no need to change the indent.
>
> Thanks, change in the next patch.
>
> >
> > > PMD_INIT_LOG(ERR, "set_owner fails: %s",
> > > - strerror(errno));
> > > + strerror(errno));
> >
> > There is no need to change the indent.
>
> ditto.
>
> >
> > > return -1;
> > > }
> > >
> > > if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> > > - &dev->device_features) < 0) {
> > > + &dev->device_features) < 0) {
> >
> > There is no need to change the indent.
>
> ditto.
>
> >
> > > PMD_INIT_LOG(ERR, "get_features failed: %s",
> > > - strerror(errno));
> > > + strerror(errno));
> >
> > There is no need to change the indent.
>
> ditto.
>
> >
> > > return -1;
> > > }
> > > + if (dev->mac_specified)
> > > + dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
> >
> > Why move above code?
>
> The server mode virtio-user is assigned the default features, which include
> VIRTIO_NET_F_MAC. So checking if support VIRTIO_NET_F_MAC is only necessary
> for client mode.
I think we should set this bit only when mac is
specified. And in server mode, we need to clear
this bit when mac isn't specified. So instead
of moving it, it should be changed to something
like this:
if (dev->mac_specified)
dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
else
dev->device_features &= ~(1ull << VIRTIO_NET_F_MAC);
Thanks
>
> Thanks,
> Jiayu
>
> >
> > > } else {
> > > /* We just pretend vhost-user can support all these features.
> > > * Note that this could be problematic that if some feature is
> > > @@ -376,9 +378,6 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
> > > dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;
> > > }
> > >
> > > - if (dev->mac_specified)
> > > - dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
> > > -
> > > if (cq) {
> > > /* device does not really need to know anything about CQ,
> > > * so if necessary, we just claim to support CQ
> > > --
> > > 2.7.4
> > >
Hi Tiwei,
> -----Original Message-----
> From: Bie, Tiwei
> Sent: Thursday, May 10, 2018 1:05 PM
> To: Hu, Jiayu <jiayu.hu@intel.com>
> Cc: dev@dpdk.org; Yang, Zhiyong <zhiyong.yang@intel.com>;
> maxime.coquelin@redhat.com
> Subject: Re: [dpdk-dev] [PATCH] net/virtio-user: fix feature setting with
> vhost-net backend
>
> On Tue, May 08, 2018 at 03:44:22PM +0800, Jiayu Hu wrote:
> > Hi Tiwei,
> >
> > On Tue, May 08, 2018 at 10:49:50AM +0800, Tiwei Bie wrote:
> > > On Mon, May 07, 2018 at 02:50:06PM +0800, Jiayu Hu wrote:
> > > > Currently, when the backend is vhost-net, which implies
> > > > virtio-user works in client mode, virtio-user is assigned
> > > > to the default feature VIRTIO_USER_SUPPORTED_FEATURES.
> > > > However, virtio-user should request features from the
> > > > backend in this case.
> > > >
> > > > Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
> > > > Signed-off-by: Jiayu Hu <jiayu.hu@intel.com>
> > > > ---
> > > > drivers/net/virtio/virtio_user/virtio_user_dev.c | 15 +++++++--------
> > > > 1 file changed, 7 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > > > index 38b8bc9..f745163 100644
> > > > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > > > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> > > > @@ -353,20 +353,22 @@ virtio_user_dev_init(struct virtio_user_dev
> *dev, char *path, int queues,
> > > > return -1;
> > > > }
> > > >
> > > > - if (dev->vhostfd >= 0) {
> > > > + if (!dev->is_server) {
> > >
> > > Server mode also needs to run below code when the
> > > connection to the backend is established.
> >
> > For server mode, the below code is called by virtio_user_start_device(),
> > instead of virtio_user_dev_init(). Only client mode virtio-user
> > needs to call it when init.
>
> Okay. So in server mode, virtio-user never get a chance
> to do GET_FEATURES.
In current design, virtio-user server mode always has no way to do GET_FEATURES.
I think it's the design problem of virtio-user server mode. This patch is just for solving
client mode virtio-user feature setting.
>
> >
> > >
> > > > if (dev->ops->send_request(dev,
> VHOST_USER_SET_OWNER,
> > > > - NULL) < 0) {
> > > > + NULL) < 0) {
> > >
> > > There is no need to change the indent.
> >
> > Thanks, change in the next patch.
> >
> > >
> > > > PMD_INIT_LOG(ERR, "set_owner fails: %s",
> > > > - strerror(errno));
> > > > + strerror(errno));
> > >
> > > There is no need to change the indent.
> >
> > ditto.
> >
> > >
> > > > return -1;
> > > > }
> > > >
> > > > if (dev->ops->send_request(dev,
> VHOST_USER_GET_FEATURES,
> > > > - &dev->device_features) < 0) {
> > > > + &dev->device_features) < 0) {
> > >
> > > There is no need to change the indent.
> >
> > ditto.
> >
> > >
> > > > PMD_INIT_LOG(ERR, "get_features failed: %s",
> > > > - strerror(errno));
> > > > + strerror(errno));
> > >
> > > There is no need to change the indent.
> >
> > ditto.
> >
> > >
> > > > return -1;
> > > > }
> > > > + if (dev->mac_specified)
> > > > + dev->device_features |= (1ull <<
> VIRTIO_NET_F_MAC);
> > >
> > > Why move above code?
> >
> > The server mode virtio-user is assigned the default features, which include
> > VIRTIO_NET_F_MAC. So checking if support VIRTIO_NET_F_MAC is only
> necessary
> > for client mode.
>
> I think we should set this bit only when mac is
> specified. And in server mode, we need to clear
> this bit when mac isn't specified. So instead
> of moving it, it should be changed to something
> like this:
>
> if (dev->mac_specified)
> dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
> else
> dev->device_features &= ~(1ull << VIRTIO_NET_F_MAC);
>
Make sense. It shouldn't change in this patch. I will change it. Thanks.
Jiayu
> Thanks
>
> >
> > Thanks,
> > Jiayu
> >
> > >
> > > > } else {
> > > > /* We just pretend vhost-user can support all these
> features.
> > > > * Note that this could be problematic that if some feature
> is
> > > > @@ -376,9 +378,6 @@ virtio_user_dev_init(struct virtio_user_dev
> *dev, char *path, int queues,
> > > > dev->device_features =
> VIRTIO_USER_SUPPORTED_FEATURES;
> > > > }
> > > >
> > > > - if (dev->mac_specified)
> > > > - dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
> > > > -
> > > > if (cq) {
> > > > /* device does not really need to know anything about CQ,
> > > > * so if necessary, we just claim to support CQ
> > > > --
> > > > 2.7.4
> > > >
@@ -353,20 +353,22 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
return -1;
}
- if (dev->vhostfd >= 0) {
+ if (!dev->is_server) {
if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER,
- NULL) < 0) {
+ NULL) < 0) {
PMD_INIT_LOG(ERR, "set_owner fails: %s",
- strerror(errno));
+ strerror(errno));
return -1;
}
if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
- &dev->device_features) < 0) {
+ &dev->device_features) < 0) {
PMD_INIT_LOG(ERR, "get_features failed: %s",
- strerror(errno));
+ strerror(errno));
return -1;
}
+ if (dev->mac_specified)
+ dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
} else {
/* We just pretend vhost-user can support all these features.
* Note that this could be problematic that if some feature is
@@ -376,9 +378,6 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;
}
- if (dev->mac_specified)
- dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
-
if (cq) {
/* device does not really need to know anything about CQ,
* so if necessary, we just claim to support CQ