diff mbox

[dpdk-dev,03/17] vhost: use new APIs to handle features

Message ID 1488534682-3494-4-git-send-email-yuanhan.liu@linux.intel.com (mailing list archive)
State Superseded, archived
Delegated to: Yuanhan Liu
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail apply patch file failure

Commit Message

Yuanhan Liu March 3, 2017, 9:51 a.m. UTC 
  Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 examples/tep_termination/main.c        |  4 +++-
 examples/vhost/main.c                  | 43 +++++++++++++++++++++-------------
 lib/librte_vhost/rte_vhost_version.map |  3 ---
 lib/librte_vhost/rte_virtio_net.h      | 13 ----------
 lib/librte_vhost/socket.c              | 23 +++++++++++++++++-
 lib/librte_vhost/vhost.c               | 41 --------------------------------
 lib/librte_vhost/vhost.h               | 20 ++++++++++++++++
 lib/librte_vhost/vhost_user.c          |  8 +++----
 8 files changed, 76 insertions(+), 79 deletions(-)

Comments

Maxime Coquelin March 14, 2017, 10:43 a.m. UTC | #1 
On 03/03/2017 10:51 AM, Yuanhan Liu wrote:
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
>  examples/tep_termination/main.c        |  4 +++-
>  examples/vhost/main.c                  | 43 +++++++++++++++++++++-------------
>  lib/librte_vhost/rte_vhost_version.map |  3 ---
>  lib/librte_vhost/rte_virtio_net.h      | 13 ----------
>  lib/librte_vhost/socket.c              | 23 +++++++++++++++++-
>  lib/librte_vhost/vhost.c               | 41 --------------------------------
>  lib/librte_vhost/vhost.h               | 20 ++++++++++++++++
>  lib/librte_vhost/vhost_user.c          |  8 +++----
>  8 files changed, 76 insertions(+), 79 deletions(-)
>

</snip>

>
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 8433a54..f7227bf 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -143,9 +143,9 @@
>   * The features that we support are requested.
>   */
>  static uint64_t
> -vhost_user_get_features(void)
> +vhost_user_get_features(struct virtio_net *dev)
>  {
> -	return VHOST_FEATURES;
> +	return rte_vhost_driver_get_features(dev->ifname);
>  }
>
>  /*
> @@ -154,7 +154,7 @@
>  static int
>  vhost_user_set_features(struct virtio_net *dev, uint64_t features)
>  {
> -	if (features & ~VHOST_FEATURES)
> +	if (features & ~rte_vhost_driver_get_features(dev->ifname))

rte_vhost_driver_get_features() returns -1 if the socket is not found.
It would result in accepting any feature trying to be set.

Maxime
Yuanhan Liu March 16, 2017, 7:43 a.m. UTC | #2 
On Tue, Mar 14, 2017 at 11:43:44AM +0100, Maxime Coquelin wrote:
> >diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> >index 8433a54..f7227bf 100644
> >--- a/lib/librte_vhost/vhost_user.c
> >+++ b/lib/librte_vhost/vhost_user.c
> >@@ -143,9 +143,9 @@
> >  * The features that we support are requested.
> >  */
> > static uint64_t
> >-vhost_user_get_features(void)
> >+vhost_user_get_features(struct virtio_net *dev)
> > {
> >-	return VHOST_FEATURES;
> >+	return rte_vhost_driver_get_features(dev->ifname);
> > }
> >
> > /*
> >@@ -154,7 +154,7 @@
> > static int
> > vhost_user_set_features(struct virtio_net *dev, uint64_t features)
> > {
> >-	if (features & ~VHOST_FEATURES)
> >+	if (features & ~rte_vhost_driver_get_features(dev->ifname))
> 
> rte_vhost_driver_get_features() returns -1 if the socket is not found.
> It would result in accepting any feature trying to be set.

If we have gone here, I think rte_vhost_driver_get_features() should not
return -1. The only exception is user unregistered such socket during
the negotiation?

	--yliu
Maxime Coquelin March 16, 2017, 9:39 a.m. UTC | #3 
On 03/16/2017 08:43 AM, Yuanhan Liu wrote:
> On Tue, Mar 14, 2017 at 11:43:44AM +0100, Maxime Coquelin wrote:
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index 8433a54..f7227bf 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -143,9 +143,9 @@
>>>  * The features that we support are requested.
>>>  */
>>> static uint64_t
>>> -vhost_user_get_features(void)
>>> +vhost_user_get_features(struct virtio_net *dev)
>>> {
>>> -	return VHOST_FEATURES;
>>> +	return rte_vhost_driver_get_features(dev->ifname);
>>> }
>>>
>>> /*
>>> @@ -154,7 +154,7 @@
>>> static int
>>> vhost_user_set_features(struct virtio_net *dev, uint64_t features)
>>> {
>>> -	if (features & ~VHOST_FEATURES)
>>> +	if (features & ~rte_vhost_driver_get_features(dev->ifname))
>>
>> rte_vhost_driver_get_features() returns -1 if the socket is not found.
>> It would result in accepting any feature trying to be set.
>
> If we have gone here, I think rte_vhost_driver_get_features() should not
> return -1. The only exception is user unregistered such socket during
> the negotiation?

Maybe this could never happen.
I just noticed that rte_vhost_driver_get_features() can fail, and in
that case, we wouldn't see it and the behavior could make hard the
cause to be hard to spot.

As this is not in the hot code path, my view is that checking its return 
and print an error message does not hurt.

Or maybe we could directly do the error prints into
rte_vhost_driver_*_features() functions if the socket name is not found?

Cheers,
Maxime
Yuanhan Liu March 17, 2017, 5:48 a.m. UTC | #4 
On Thu, Mar 16, 2017 at 10:39:00AM +0100, Maxime Coquelin wrote:
> 
> 
> On 03/16/2017 08:43 AM, Yuanhan Liu wrote:
> >On Tue, Mar 14, 2017 at 11:43:44AM +0100, Maxime Coquelin wrote:
> >>>diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> >>>index 8433a54..f7227bf 100644
> >>>--- a/lib/librte_vhost/vhost_user.c
> >>>+++ b/lib/librte_vhost/vhost_user.c
> >>>@@ -143,9 +143,9 @@
> >>> * The features that we support are requested.
> >>> */
> >>>static uint64_t
> >>>-vhost_user_get_features(void)
> >>>+vhost_user_get_features(struct virtio_net *dev)
> >>>{
> >>>-	return VHOST_FEATURES;
> >>>+	return rte_vhost_driver_get_features(dev->ifname);
> >>>}
> >>>
> >>>/*
> >>>@@ -154,7 +154,7 @@
> >>>static int
> >>>vhost_user_set_features(struct virtio_net *dev, uint64_t features)
> >>>{
> >>>-	if (features & ~VHOST_FEATURES)
> >>>+	if (features & ~rte_vhost_driver_get_features(dev->ifname))
> >>
> >>rte_vhost_driver_get_features() returns -1 if the socket is not found.
> >>It would result in accepting any feature trying to be set.
> >
> >If we have gone here, I think rte_vhost_driver_get_features() should not
> >return -1. The only exception is user unregistered such socket during
> >the negotiation?
> 
> Maybe this could never happen.
> I just noticed that rte_vhost_driver_get_features() can fail, and in
> that case, we wouldn't see it and the behavior could make hard the
> cause to be hard to spot.
> 
> As this is not in the hot code path, my view is that checking its return and
> print an error message does not hurt.
> 
> Or maybe we could directly do the error prints into
> rte_vhost_driver_*_features() functions if the socket name is not found?

Yes, I think we could do that. Thanks!

	--yliu
diff mbox

Patch

diff --git a/examples/tep_termination/main.c b/examples/tep_termination/main.c
index bd1dc96..8c45128 100644
--- a/examples/tep_termination/main.c
+++ b/examples/tep_termination/main.c
@@ -1250,12 +1250,14 @@  static inline void __attribute__((always_inline))
 		rte_eal_remote_launch(switch_worker,
 			mbuf_pool, lcore_id);
 	}
-	rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_MRG_RXBUF);
 
 	ret = rte_vhost_driver_register((char *)&dev_basename, 0);
 	if (ret != 0)
 		rte_exit(EXIT_FAILURE, "failed to register vhost driver.\n");
 
+	rte_vhost_driver_disable_features(dev_basename,
+		1ULL << VIRTIO_NET_F_MRG_RXBUF);
+
 	rte_vhost_driver_callback_register(&virtio_net_device_ops);
 
 	rte_vhost_driver_session_start();
diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 4789947..972a6a8 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -328,16 +328,6 @@  struct mbuf_table {
 
 	if (port >= rte_eth_dev_count()) return -1;
 
-	if (enable_tx_csum == 0)
-		rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_CSUM);
-
-	if (enable_tso == 0) {
-		rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_HOST_TSO4);
-		rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_HOST_TSO6);
-		rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_GUEST_TSO4);
-		rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_GUEST_TSO6);
-	}
-
 	rx_rings = (uint16_t)dev_info.max_rx_queues;
 	/* Configure ethernet device. */
 	retval = rte_eth_dev_configure(port, rx_rings, tx_rings, &port_conf);
@@ -531,7 +521,6 @@  struct mbuf_table {
 			vmdq_conf_default.rx_adv_conf.vmdq_rx_conf.rx_mode =
 				ETH_VMDQ_ACCEPT_BROADCAST |
 				ETH_VMDQ_ACCEPT_MULTICAST;
-			rte_vhost_feature_enable(1ULL << VIRTIO_NET_F_CTRL_RX);
 
 			break;
 
@@ -1509,9 +1498,6 @@  static inline void __attribute__((always_inline))
 	RTE_LCORE_FOREACH_SLAVE(lcore_id)
 		rte_eal_remote_launch(switch_worker, NULL, lcore_id);
 
-	if (mergeable == 0)
-		rte_vhost_feature_disable(1ULL << VIRTIO_NET_F_MRG_RXBUF);
-
 	if (client_mode)
 		flags |= RTE_VHOST_USER_CLIENT;
 
@@ -1520,13 +1506,38 @@  static inline void __attribute__((always_inline))
 
 	/* Register vhost user driver to handle vhost messages. */
 	for (i = 0; i < nb_sockets; i++) {
-		ret = rte_vhost_driver_register
-				(socket_files + i * PATH_MAX, flags);
+		char *file = socket_files + i * PATH_MAX;
+		ret = rte_vhost_driver_register(file, flags);
 		if (ret != 0) {
 			unregister_drivers(i);
 			rte_exit(EXIT_FAILURE,
 				"vhost driver register failure.\n");
 		}
+		if (mergeable == 0) {
+			rte_vhost_driver_disable_features(file,
+				1ULL << VIRTIO_NET_F_MRG_RXBUF);
+		}
+
+		if (enable_tx_csum == 0) {
+			rte_vhost_driver_disable_features(file,
+				1ULL << VIRTIO_NET_F_CSUM);
+		}
+
+		if (enable_tso == 0) {
+			rte_vhost_driver_disable_features(file,
+				1ULL << VIRTIO_NET_F_HOST_TSO4);
+			rte_vhost_driver_disable_features(file,
+				1ULL << VIRTIO_NET_F_HOST_TSO6);
+			rte_vhost_driver_disable_features(file,
+				1ULL << VIRTIO_NET_F_GUEST_TSO4);
+			rte_vhost_driver_disable_features(file,
+				1ULL << VIRTIO_NET_F_GUEST_TSO6);
+		}
+
+		if (promiscuous) {
+			rte_vhost_driver_enable_features(file,
+				1ULL << VIRTIO_NET_F_CTRL_RX);
+		}
 	}
 
 	rte_vhost_driver_callback_register(&virtio_net_device_ops);
diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
index d4f2f69..ee72d5f 100644
--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -7,9 +7,6 @@  DPDK_2.0 {
 	rte_vhost_driver_session_start;
 	rte_vhost_enable_guest_notification;
 	rte_vhost_enqueue_burst;
-	rte_vhost_feature_disable;
-	rte_vhost_feature_enable;
-	rte_vhost_feature_get;
 
 	local: *;
 };
diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h
index 6e166a6..51f5166 100644
--- a/lib/librte_vhost/rte_virtio_net.h
+++ b/lib/librte_vhost/rte_virtio_net.h
@@ -70,19 +70,6 @@  struct virtio_net_device_ops {
 	void *reserved[5]; /**< Reserved for future extension */
 };
 
-/**
- *  Disable features in feature_mask. Returns 0 on success.
- */
-int rte_vhost_feature_disable(uint64_t feature_mask);
-
-/**
- *  Enable features in feature_mask. Returns 0 on success.
- */
-int rte_vhost_feature_enable(uint64_t feature_mask);
-
-/* Returns currently supported vhost features */
-uint64_t rte_vhost_feature_get(void);
-
 int rte_vhost_enable_guest_notification(int vid, uint16_t queue_id, int enable);
 
 /**
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 76eb426..9e0ec05 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -500,6 +500,21 @@  struct vhost_user_reconnect_list {
 	return NULL;
 }
 
+/*
+ * Applications know nothing about features the builtin virtio net
+ * driver (virtio_net.c) supports, thus it's not possible for them
+ * to invoke rte_vhost_driver_set_features(). To workaround it, here
+ * we set it when no such call is invoked.
+ */
+static void
+workaround_builtin_virtio_net_features(struct vhost_user_socket *vsocket)
+{
+	if (vsocket->supported_features == UINT64_MAX) {
+		vsocket->supported_features = VIRTIO_NET_SUPPORTED_FEATURES;
+		vsocket->features           = VIRTIO_NET_SUPPORTED_FEATURES;
+	}
+}
+
 int
 rte_vhost_driver_disable_features(const char *path, uint64_t features)
 {
@@ -507,8 +522,10 @@  struct vhost_user_reconnect_list {
 
 	pthread_mutex_lock(&vhost_user.mutex);
 	vsocket = find_vhost_user_socket(path);
-	if (vsocket)
+	if (vsocket) {
+		workaround_builtin_virtio_net_features(vsocket);
 		vsocket->features &= ~features;
+	}
 	pthread_mutex_unlock(&vhost_user.mutex);
 
 	return vsocket ? 0 : -1;
@@ -522,6 +539,7 @@  struct vhost_user_reconnect_list {
 	pthread_mutex_lock(&vhost_user.mutex);
 	vsocket = find_vhost_user_socket(path);
 	if (vsocket) {
+		workaround_builtin_virtio_net_features(vsocket);
 		if ((vsocket->supported_features & features) != features) {
 			/*
 			 * trying to enable features the driver doesn't
@@ -560,6 +578,8 @@  struct vhost_user_reconnect_list {
 
 	pthread_mutex_lock(&vhost_user.mutex);
 	vsocket = find_vhost_user_socket(path);
+	if (vsocket)
+		workaround_builtin_virtio_net_features(vsocket);
 	pthread_mutex_unlock(&vhost_user.mutex);
 
 	return vsocket ? vsocket->features : (uint64_t)-1;
@@ -594,6 +614,7 @@  struct vhost_user_reconnect_list {
 	vsocket->path = strdup(path);
 	vsocket->connfd = -1;
 	vsocket->dequeue_zero_copy = flags & RTE_VHOST_USER_DEQUEUE_ZERO_COPY;
+	vsocket->supported_features = UINT64_MAX;
 
 	if ((flags & RTE_VHOST_USER_CLIENT) != 0) {
 		vsocket->reconnect = !(flags & RTE_VHOST_USER_NO_RECONNECT);
diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 3c3f6a4..2790f17 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -49,27 +49,6 @@ 
 
 #include "vhost.h"
 
-#define VHOST_USER_F_PROTOCOL_FEATURES	30
-
-/* Features supported by this lib. */
-#define VHOST_SUPPORTED_FEATURES ((1ULL << VIRTIO_NET_F_MRG_RXBUF) | \
-				(1ULL << VIRTIO_NET_F_CTRL_VQ) | \
-				(1ULL << VIRTIO_NET_F_CTRL_RX) | \
-				(1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE) | \
-				(1ULL << VIRTIO_NET_F_MQ)      | \
-				(1ULL << VIRTIO_F_VERSION_1)   | \
-				(1ULL << VHOST_F_LOG_ALL)      | \
-				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES) | \
-				(1ULL << VIRTIO_NET_F_HOST_TSO4) | \
-				(1ULL << VIRTIO_NET_F_HOST_TSO6) | \
-				(1ULL << VIRTIO_NET_F_CSUM)    | \
-				(1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
-				(1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
-				(1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
-				(1ULL << VIRTIO_RING_F_INDIRECT_DESC))
-
-uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
-
 struct virtio_net *vhost_devices[MAX_VHOST_DEVICE];
 
 /* device ops to add/remove device to/from data core. */
@@ -399,26 +378,6 @@  struct virtio_net *
 	return 0;
 }
 
-uint64_t rte_vhost_feature_get(void)
-{
-	return VHOST_FEATURES;
-}
-
-int rte_vhost_feature_disable(uint64_t feature_mask)
-{
-	VHOST_FEATURES = VHOST_FEATURES & ~feature_mask;
-	return 0;
-}
-
-int rte_vhost_feature_enable(uint64_t feature_mask)
-{
-	if ((feature_mask & VHOST_SUPPORTED_FEATURES) == feature_mask) {
-		VHOST_FEATURES = VHOST_FEATURES | feature_mask;
-		return 0;
-	}
-	return -1;
-}
-
 /*
  * Register ops so that we can add/remove device to data core.
  */
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index d97df1d..61e7448 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -44,6 +44,26 @@ 
 
 #include "rte_virtio_net.h"
 
+#define VHOST_USER_F_PROTOCOL_FEATURES	30
+
+/* Features supported by this lib. */
+#define VIRTIO_NET_SUPPORTED_FEATURES ((1ULL << VIRTIO_NET_F_MRG_RXBUF) | \
+				(1ULL << VIRTIO_NET_F_CTRL_VQ) | \
+				(1ULL << VIRTIO_NET_F_CTRL_RX) | \
+				(1ULL << VIRTIO_NET_F_GUEST_ANNOUNCE) | \
+				(1ULL << VIRTIO_NET_F_MQ)      | \
+				(1ULL << VIRTIO_F_VERSION_1)   | \
+				(1ULL << VHOST_F_LOG_ALL)      | \
+				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES) | \
+				(1ULL << VIRTIO_NET_F_HOST_TSO4) | \
+				(1ULL << VIRTIO_NET_F_HOST_TSO6) | \
+				(1ULL << VIRTIO_NET_F_CSUM)    | \
+				(1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
+				(1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
+				(1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
+				(1ULL << VIRTIO_RING_F_INDIRECT_DESC))
+
+
 /* Used to indicate that the device is running on a data core */
 #define VIRTIO_DEV_RUNNING 1
 
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 8433a54..f7227bf 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -143,9 +143,9 @@ 
  * The features that we support are requested.
  */
 static uint64_t
-vhost_user_get_features(void)
+vhost_user_get_features(struct virtio_net *dev)
 {
-	return VHOST_FEATURES;
+	return rte_vhost_driver_get_features(dev->ifname);
 }
 
 /*
@@ -154,7 +154,7 @@ 
 static int
 vhost_user_set_features(struct virtio_net *dev, uint64_t features)
 {
-	if (features & ~VHOST_FEATURES)
+	if (features & ~rte_vhost_driver_get_features(dev->ifname))
 		return -1;
 
 	dev->features = features;
@@ -980,7 +980,7 @@ 
 
 	switch (msg.request) {
 	case VHOST_USER_GET_FEATURES:
-		msg.payload.u64 = vhost_user_get_features();
+		msg.payload.u64 = vhost_user_get_features(dev);
 		msg.size = sizeof(msg.payload.u64);
 		send_vhost_message(fd, &msg);
 		break;