[dpdk-dev] [PATCH 0/4] make vhost PMD available for secondary processes

[dpdk-dev] [PATCH 0/4] make vhost PMD available for secondary processes

[oda]

From: Itsuro Oda <oda@valinux.co.jp>

vhost PMD has not been available for secondary processes since
DPDK v18.11.  (https://bugs.dpdk.org/show_bug.cgi?id=194)
(for a long term !)
This series of patches intend to make vhost PMD available for
secondary processes.
Because now setting vhost driver to communicate with a vhost-user
master (ex. Qemu) is accomplished by the probe function of the
primary process, only the primary process can be a vhost-user
slave.
With this patch, setting vhost driver is delayed at eth_dev
configuration in order to be able to set it from a secondary
process. Because (in the first place,) setting vhost driver is not
necessary to be done at probe (it is enough to be done up to eth_dev
start), this fix is no problem for the primary process.
There is a precondition that the same process has to operate
a vhost interface from begining to end (eth_dev configuration to
eth_dev close). (This patch leaves it to user's responsibility.)
This precondition will not be a problem in most use cases
(including SPP).

Itsuro Oda (4):
  net/vhost: remove an unused member
  net/vhost: allocate iface_name from heap
  net/vhost: delay vhost driver setup
  net/vhost: make secondary probe complete

 drivers/net/vhost/rte_eth_vhost.c | 152 +++++++++++++++++-------------
 1 file changed, 88 insertions(+), 64 deletions(-)

-- 
2.17.0

[dpdk-dev] [PATCH 1/4] net/vhost: remove an unused member

[oda]

From: Itsuro Oda <oda@valinux.co.jp>

remove an unused member from pmd_internal.
---
 drivers/net/vhost/rte_eth_vhost.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 46f01a7f4..d4e3485ce 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -95,7 +95,6 @@ struct vhost_queue {
 
 struct pmd_internal {
 	rte_atomic32_t dev_attached;
-	char *dev_name;
 	char *iface_name;
 	uint16_t max_queues;
 	int vid;
@@ -1008,7 +1007,6 @@ eth_dev_close(struct rte_eth_dev *dev)
 		for (i = 0; i < dev->data->nb_tx_queues; i++)
 			rte_free(dev->data->tx_queues[i]);
 
-	free(internal->dev_name);
 	free(internal->iface_name);
 	rte_free(internal);
 
@@ -1253,9 +1251,6 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	 * - and point eth_dev structure to new eth_dev_data structure
 	 */
 	internal = eth_dev->data->dev_private;
-	internal->dev_name = strdup(name);
-	if (internal->dev_name == NULL)
-		goto error;
 	internal->iface_name = strdup(iface_name);
 	if (internal->iface_name == NULL)
 		goto error;
@@ -1305,10 +1300,8 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	return data->port_id;
 
 error:
-	if (internal) {
+	if (internal)
 		free(internal->iface_name);
-		free(internal->dev_name);
-	}
 	rte_free(vring_state);
 	rte_eth_dev_release_port(eth_dev);
 	rte_free(list);
-- 
2.17.0

[dpdk-dev] [PATCH 2/4] net/vhost: allocate iface_name from heap

[oda]

From: Itsuro Oda <oda@valinux.co.jp>

allocate iface_name of pmd_internal from heap in order to
be able to refer from secondary processes.
---
 drivers/net/vhost/rte_eth_vhost.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index d4e3485ce..1b07aad24 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1007,7 +1007,7 @@ eth_dev_close(struct rte_eth_dev *dev)
 		for (i = 0; i < dev->data->nb_tx_queues; i++)
 			rte_free(dev->data->tx_queues[i]);
 
-	free(internal->iface_name);
+	rte_free(internal->iface_name);
 	rte_free(internal);
 
 	dev->data->dev_private = NULL;
@@ -1251,9 +1251,11 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	 * - and point eth_dev structure to new eth_dev_data structure
 	 */
 	internal = eth_dev->data->dev_private;
-	internal->iface_name = strdup(iface_name);
+	internal->iface_name = rte_malloc_socket(name, strlen(iface_name) + 1,
+						 0, numa_node);
 	if (internal->iface_name == NULL)
 		goto error;
+	strcpy(internal->iface_name, iface_name);
 
 	list->eth_dev = eth_dev;
 	pthread_mutex_lock(&internal_list_lock);
@@ -1301,7 +1303,7 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 
 error:
 	if (internal)
-		free(internal->iface_name);
+		rte_free(internal->iface_name);
 	rte_free(vring_state);
 	rte_eth_dev_release_port(eth_dev);
 	rte_free(list);
-- 
2.17.0

[dpdk-dev] [PATCH 3/4] net/vhost: delay vhost driver setup

[oda]

From: Itsuro Oda <oda@valinux.co.jp>

setting vhost driver is delayed at eth_dev configuration
in order to be able to set it from a secondary process.
---
 drivers/net/vhost/rte_eth_vhost.c | 130 ++++++++++++++++++------------
 1 file changed, 78 insertions(+), 52 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 1b07aad24..0b8b5a4ca 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -96,6 +96,8 @@ struct vhost_queue {
 struct pmd_internal {
 	rte_atomic32_t dev_attached;
 	char *iface_name;
+	uint64_t flags;
+	uint64_t disable_flags;
 	uint16_t max_queues;
 	int vid;
 	rte_atomic32_t started;
@@ -490,17 +492,6 @@ eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
 	return nb_tx;
 }
 
-static int
-eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
-{
-	struct pmd_internal *internal = dev->data->dev_private;
-	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
-
-	internal->vlan_strip = !!(rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP);
-
-	return 0;
-}
-
 static inline struct internal_list *
 find_internal_resource(char *ifname)
 {
@@ -876,6 +867,62 @@ static struct vhost_device_ops vhost_ops = {
 	.vring_state_changed = vring_state_changed,
 };
 
+static int
+vhost_driver_setup(struct rte_eth_dev *eth_dev)
+{
+	struct pmd_internal *internal = eth_dev->data->dev_private;
+	struct internal_list *list = NULL;
+	struct rte_vhost_vring_state *vring_state = NULL;
+	unsigned int numa_node = eth_dev->device->numa_node;
+	const char *name = eth_dev->device->name;
+
+	list = rte_zmalloc_socket(name, sizeof(*list), 0, numa_node);
+	if (list == NULL)
+		goto error;
+
+	vring_state = rte_zmalloc_socket(name, sizeof(*vring_state),
+					 0, numa_node);
+	if (vring_state == NULL)
+		goto error;
+
+	list->eth_dev = eth_dev;
+	pthread_mutex_lock(&internal_list_lock);
+	TAILQ_INSERT_TAIL(&internal_list, list, next);
+	pthread_mutex_unlock(&internal_list_lock);
+
+	rte_spinlock_init(&vring_state->lock);
+	vring_states[eth_dev->data->port_id] = vring_state;
+
+	if (rte_vhost_driver_register(internal->iface_name, internal->flags))
+		goto error;
+
+	if (internal->disable_flags) {
+		if (rte_vhost_driver_disable_features(internal->iface_name,
+						      internal->disable_flags))
+			goto error;
+	}
+
+	if (rte_vhost_driver_callback_register(internal->iface_name,
+					       &vhost_ops) < 0) {
+		VHOST_LOG(ERR, "Can't register callbacks\n");
+		goto error;
+	}
+
+	if (rte_vhost_driver_start(internal->iface_name) < 0) {
+		VHOST_LOG(ERR, "Failed to start driver for %s\n",
+			  internal->iface_name);
+		goto error;
+	}
+
+	return 0;
+
+error:
+	rte_free(vring_state);
+	rte_free(list);
+
+	return -1;
+}
+
 int
 rte_eth_vhost_get_queue_event(uint16_t port_id,
 		struct rte_eth_vhost_queue_event *event)
@@ -942,6 +989,24 @@ rte_eth_vhost_get_vid_from_port_id(uint16_t port_id)
 	return vid;
 }
 
+static int
+eth_dev_configure(struct rte_eth_dev *dev)
+{
+	struct pmd_internal *internal = dev->data->dev_private;
+	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
+
+	/* NOTE: the same process has to operate a vhost interface
+	 * from begining to end (eth_dev configure to eth_dev close).
+	 * It is user's responsibility at the moment.
+	 */
+	if (vhost_driver_setup(dev) < 0)
+		return -1;
+
+	internal->vlan_strip = !!(rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP);
+
+	return 0;
+}
+
 static int
 eth_dev_start(struct rte_eth_dev *eth_dev)
 {
@@ -1217,16 +1282,10 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	struct pmd_internal *internal = NULL;
 	struct rte_eth_dev *eth_dev = NULL;
 	struct rte_ether_addr *eth_addr = NULL;
-	struct rte_vhost_vring_state *vring_state = NULL;
-	struct internal_list *list = NULL;
 
 	VHOST_LOG(INFO, "Creating VHOST-USER backend on numa socket %u\n",
 		numa_node);
 
-	list = rte_zmalloc_socket(name, sizeof(*list), 0, numa_node);
-	if (list == NULL)
-		goto error;
-
 	/* reserve an ethdev entry */
 	eth_dev = rte_eth_vdev_allocate(dev, sizeof(*internal));
 	if (eth_dev == NULL)
@@ -1240,11 +1299,6 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	*eth_addr = base_eth_addr;
 	eth_addr->addr_bytes[5] = eth_dev->data->port_id;
 
-	vring_state = rte_zmalloc_socket(name,
-			sizeof(*vring_state), 0, numa_node);
-	if (vring_state == NULL)
-		goto error;
-
 	/* now put it all together
 	 * - store queue data in internal,
 	 * - point eth_dev_data to internals
@@ -1257,18 +1311,12 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 		goto error;
 	strcpy(internal->iface_name, iface_name);
 
-	list->eth_dev = eth_dev;
-	pthread_mutex_lock(&internal_list_lock);
-	TAILQ_INSERT_TAIL(&internal_list, list, next);
-	pthread_mutex_unlock(&internal_list_lock);
-
-	rte_spinlock_init(&vring_state->lock);
-	vring_states[eth_dev->data->port_id] = vring_state;
-
 	data->nb_rx_queues = queues;
 	data->nb_tx_queues = queues;
 	internal->max_queues = queues;
 	internal->vid = -1;
+	internal->flags = flags;
+	internal->disable_flags = disable_flags;
 	data->dev_link = pmd_link;
 	data->dev_flags = RTE_ETH_DEV_INTR_LSC | RTE_ETH_DEV_CLOSE_REMOVE;
 
@@ -1278,35 +1326,13 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	eth_dev->rx_pkt_burst = eth_vhost_rx;
 	eth_dev->tx_pkt_burst = eth_vhost_tx;
 
-	if (rte_vhost_driver_register(iface_name, flags))
-		goto error;
-
-	if (disable_flags) {
-		if (rte_vhost_driver_disable_features(iface_name,
-					disable_flags))
-			goto error;
-	}
-
-	if (rte_vhost_driver_callback_register(iface_name, &vhost_ops) < 0) {
-		VHOST_LOG(ERR, "Can't register callbacks\n");
-		goto error;
-	}
-
-	if (rte_vhost_driver_start(iface_name) < 0) {
-		VHOST_LOG(ERR, "Failed to start driver for %s\n",
-			iface_name);
-		goto error;
-	}
-
 	rte_eth_dev_probing_finish(eth_dev);
 	return data->port_id;
 
 error:
 	if (internal)
 		rte_free(internal->iface_name);
-	rte_free(vring_state);
 	rte_eth_dev_release_port(eth_dev);
-	rte_free(list);
 
 	return -1;
 }
-- 
2.17.0

[dpdk-dev] [PATCH 4/4] net/vhost: make secondary probe complete

[oda]

From: Itsuro Oda <oda@valinux.co.jp>

add lacking member setting and make secondary probe complete.
---
 drivers/net/vhost/rte_eth_vhost.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 0b8b5a4ca..7a501cf91 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1390,8 +1390,11 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
 			VHOST_LOG(ERR, "Failed to probe %s\n", name);
 			return -1;
 		}
-		/* TODO: request info from primary to set up Rx and Tx */
+		eth_dev->rx_pkt_burst = eth_vhost_rx;
+		eth_dev->tx_pkt_burst = eth_vhost_tx;
 		eth_dev->dev_ops = &ops;
+		if (dev->device.numa_node == SOCKET_ID_ANY)
+			dev->device.numa_node = rte_socket_id();
 		eth_dev->device = &dev->device;
 		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
-- 
2.17.0

Re: [dpdk-dev] [PATCH 0/4] make vhost PMD available for secondary processes

[Itsuro ODA]

Hi,

I will fix to add signed-off-by and correct spelling error
according to test-report@dpdk.
Please check the content until then.

Thanks.

On Wed,  8 Jan 2020 15:25:06 +0900
oda@valinux.co.jp wrote:

> From: Itsuro Oda <oda@valinux.co.jp>
> 
> vhost PMD has not been available for secondary processes since
> DPDK v18.11.  (https://bugs.dpdk.org/show_bug.cgi?id=194)
> (for a long term !)
> This series of patches intend to make vhost PMD available for
> secondary processes.
> Because now setting vhost driver to communicate with a vhost-user
> master (ex. Qemu) is accomplished by the probe function of the
> primary process, only the primary process can be a vhost-user
> slave.
> With this patch, setting vhost driver is delayed at eth_dev
> configuration in order to be able to set it from a secondary
> process. Because (in the first place,) setting vhost driver is not
> necessary to be done at probe (it is enough to be done up to eth_dev
> start), this fix is no problem for the primary process.
> There is a precondition that the same process has to operate
> a vhost interface from begining to end (eth_dev configuration to
> eth_dev close). (This patch leaves it to user's responsibility.)
> This precondition will not be a problem in most use cases
> (including SPP).
> 
> Itsuro Oda (4):
>   net/vhost: remove an unused member
>   net/vhost: allocate iface_name from heap
>   net/vhost: delay vhost driver setup
>   net/vhost: make secondary probe complete
> 
>  drivers/net/vhost/rte_eth_vhost.c | 152 +++++++++++++++++-------------
>  1 file changed, 88 insertions(+), 64 deletions(-)
> 
> -- 
> 2.17.0

-- 
Itsuro ODA <oda@valinux.co.jp>

[dpdk-dev] [PATCH 0/4] make vhost PMD available for secondary processes

[Itsuro Oda]

vhost PMD has not been available for secondary processes since
DPDK v18.11.  (https://bugs.dpdk.org/show_bug.cgi?id=194)
(for a long term !)
This series of patches intend to make vhost PMD available for
secondary processes.
Because now setting vhost driver to communicate with a vhost-user
master (ex. Qemu) is accomplished by the probe function of the
primary process, only the primary process can be a vhost-user
slave.
With this patch, setting vhost driver is delayed at eth_dev
configuration in order to be able to set it from a secondary
process. Because (in the first place,) setting vhost driver is not
necessary to be done at probe (it is enough to be done up to eth_dev
start), this fix is no problem for the primary process.
There is a precondition that the same process has to operate
a vhost interface from beginning to end (from eth_dev configuration
to eth_dev close). (This patch leaves it to user's responsibility.)
This precondition will not be a problem in most use cases
(including SPP).

v2:
- add signed-off-by
- fix spelling error

Itsuro Oda (4):
  net/vhost: remove an unused member
  net/vhost: allocate iface_name from heap
  net/vhost: delay vhost driver setup
  net/vhost: make secondary probe complete

 drivers/net/vhost/rte_eth_vhost.c | 152 +++++++++++++++++-------------
 1 file changed, 88 insertions(+), 64 deletions(-)

-- 
2.17.0

[dpdk-dev] [PATCH 1/4] net/vhost: remove an unused member

[Itsuro Oda]

remove an unused member from pmd_internal.

Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
---
 drivers/net/vhost/rte_eth_vhost.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 46f01a7f4..d4e3485ce 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -95,7 +95,6 @@ struct vhost_queue {
 
 struct pmd_internal {
 	rte_atomic32_t dev_attached;
-	char *dev_name;
 	char *iface_name;
 	uint16_t max_queues;
 	int vid;
@@ -1008,7 +1007,6 @@ eth_dev_close(struct rte_eth_dev *dev)
 		for (i = 0; i < dev->data->nb_tx_queues; i++)
 			rte_free(dev->data->tx_queues[i]);
 
-	free(internal->dev_name);
 	free(internal->iface_name);
 	rte_free(internal);
 
@@ -1253,9 +1251,6 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	 * - and point eth_dev structure to new eth_dev_data structure
 	 */
 	internal = eth_dev->data->dev_private;
-	internal->dev_name = strdup(name);
-	if (internal->dev_name == NULL)
-		goto error;
 	internal->iface_name = strdup(iface_name);
 	if (internal->iface_name == NULL)
 		goto error;
@@ -1305,10 +1300,8 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	return data->port_id;
 
 error:
-	if (internal) {
+	if (internal)
 		free(internal->iface_name);
-		free(internal->dev_name);
-	}
 	rte_free(vring_state);
 	rte_eth_dev_release_port(eth_dev);
 	rte_free(list);
-- 
2.17.0

[dpdk-dev] [PATCH 2/4] net/vhost: allocate iface_name from heap

[Itsuro Oda]

allocate iface_name of pmd_internal from heap in order to
be able to refer from secondary processes.

Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
---
 drivers/net/vhost/rte_eth_vhost.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index d4e3485ce..1b07aad24 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1007,7 +1007,7 @@ eth_dev_close(struct rte_eth_dev *dev)
 		for (i = 0; i < dev->data->nb_tx_queues; i++)
 			rte_free(dev->data->tx_queues[i]);
 
-	free(internal->iface_name);
+	rte_free(internal->iface_name);
 	rte_free(internal);
 
 	dev->data->dev_private = NULL;
@@ -1251,9 +1251,11 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	 * - and point eth_dev structure to new eth_dev_data structure
 	 */
 	internal = eth_dev->data->dev_private;
-	internal->iface_name = strdup(iface_name);
+	internal->iface_name = rte_malloc_socket(name, strlen(iface_name) + 1,
+						 0, numa_node);
 	if (internal->iface_name == NULL)
 		goto error;
+	strcpy(internal->iface_name, iface_name);
 
 	list->eth_dev = eth_dev;
 	pthread_mutex_lock(&internal_list_lock);
@@ -1301,7 +1303,7 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 
 error:
 	if (internal)
-		free(internal->iface_name);
+		rte_free(internal->iface_name);
 	rte_free(vring_state);
 	rte_eth_dev_release_port(eth_dev);
 	rte_free(list);
-- 
2.17.0

[dpdk-dev] [PATCH 3/4] net/vhost: delay vhost driver setup

[Itsuro Oda]

setting vhost driver is delayed at eth_dev configuration
in order to be able to set it from a secondary process.

Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
---
 drivers/net/vhost/rte_eth_vhost.c | 130 ++++++++++++++++++------------
 1 file changed, 78 insertions(+), 52 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 1b07aad24..44f44cea3 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -96,6 +96,8 @@ struct vhost_queue {
 struct pmd_internal {
 	rte_atomic32_t dev_attached;
 	char *iface_name;
+	uint64_t flags;
+	uint64_t disable_flags;
 	uint16_t max_queues;
 	int vid;
 	rte_atomic32_t started;
@@ -490,17 +492,6 @@ eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
 	return nb_tx;
 }
 
-static int
-eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
-{
-	struct pmd_internal *internal = dev->data->dev_private;
-	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
-
-	internal->vlan_strip = !!(rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP);
-
-	return 0;
-}
-
 static inline struct internal_list *
 find_internal_resource(char *ifname)
 {
@@ -876,6 +867,62 @@ static struct vhost_device_ops vhost_ops = {
 	.vring_state_changed = vring_state_changed,
 };
 
+static int
+vhost_driver_setup(struct rte_eth_dev *eth_dev)
+{
+	struct pmd_internal *internal = eth_dev->data->dev_private;
+	struct internal_list *list = NULL;
+	struct rte_vhost_vring_state *vring_state = NULL;
+	unsigned int numa_node = eth_dev->device->numa_node;
+	const char *name = eth_dev->device->name;
+
+	list = rte_zmalloc_socket(name, sizeof(*list), 0, numa_node);
+	if (list == NULL)
+		goto error;
+
+	vring_state = rte_zmalloc_socket(name, sizeof(*vring_state),
+					 0, numa_node);
+	if (vring_state == NULL)
+		goto error;
+
+	list->eth_dev = eth_dev;
+	pthread_mutex_lock(&internal_list_lock);
+	TAILQ_INSERT_TAIL(&internal_list, list, next);
+	pthread_mutex_unlock(&internal_list_lock);
+
+	rte_spinlock_init(&vring_state->lock);
+	vring_states[eth_dev->data->port_id] = vring_state;
+
+	if (rte_vhost_driver_register(internal->iface_name, internal->flags))
+		goto error;
+
+	if (internal->disable_flags) {
+		if (rte_vhost_driver_disable_features(internal->iface_name,
+						      internal->disable_flags))
+			goto error;
+	}
+
+	if (rte_vhost_driver_callback_register(internal->iface_name,
+					       &vhost_ops) < 0) {
+		VHOST_LOG(ERR, "Can't register callbacks\n");
+		goto error;
+	}
+
+	if (rte_vhost_driver_start(internal->iface_name) < 0) {
+		VHOST_LOG(ERR, "Failed to start driver for %s\n",
+			  internal->iface_name);
+		goto error;
+	}
+
+	return 0;
+
+error:
+	rte_free(vring_state);
+	rte_free(list);
+
+	return -1;
+}
+
 int
 rte_eth_vhost_get_queue_event(uint16_t port_id,
 		struct rte_eth_vhost_queue_event *event)
@@ -942,6 +989,24 @@ rte_eth_vhost_get_vid_from_port_id(uint16_t port_id)
 	return vid;
 }
 
+static int
+eth_dev_configure(struct rte_eth_dev *dev)
+{
+	struct pmd_internal *internal = dev->data->dev_private;
+	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
+
+	/* NOTE: the same process has to operate a vhost interface
+	 * from beginning to end (from eth_dev configure to eth_dev close).
+	 * It is user's responsibility at the moment.
+	 */
+	if (vhost_driver_setup(dev) < 0)
+		return -1;
+
+	internal->vlan_strip = !!(rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP);
+
+	return 0;
+}
+
 static int
 eth_dev_start(struct rte_eth_dev *eth_dev)
 {
@@ -1217,16 +1282,10 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	struct pmd_internal *internal = NULL;
 	struct rte_eth_dev *eth_dev = NULL;
 	struct rte_ether_addr *eth_addr = NULL;
-	struct rte_vhost_vring_state *vring_state = NULL;
-	struct internal_list *list = NULL;
 
 	VHOST_LOG(INFO, "Creating VHOST-USER backend on numa socket %u\n",
 		numa_node);
 
-	list = rte_zmalloc_socket(name, sizeof(*list), 0, numa_node);
-	if (list == NULL)
-		goto error;
-
 	/* reserve an ethdev entry */
 	eth_dev = rte_eth_vdev_allocate(dev, sizeof(*internal));
 	if (eth_dev == NULL)
@@ -1240,11 +1299,6 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	*eth_addr = base_eth_addr;
 	eth_addr->addr_bytes[5] = eth_dev->data->port_id;
 
-	vring_state = rte_zmalloc_socket(name,
-			sizeof(*vring_state), 0, numa_node);
-	if (vring_state == NULL)
-		goto error;
-
 	/* now put it all together
 	 * - store queue data in internal,
 	 * - point eth_dev_data to internals
@@ -1257,18 +1311,12 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 		goto error;
 	strcpy(internal->iface_name, iface_name);
 
-	list->eth_dev = eth_dev;
-	pthread_mutex_lock(&internal_list_lock);
-	TAILQ_INSERT_TAIL(&internal_list, list, next);
-	pthread_mutex_unlock(&internal_list_lock);
-
-	rte_spinlock_init(&vring_state->lock);
-	vring_states[eth_dev->data->port_id] = vring_state;
-
 	data->nb_rx_queues = queues;
 	data->nb_tx_queues = queues;
 	internal->max_queues = queues;
 	internal->vid = -1;
+	internal->flags = flags;
+	internal->disable_flags = disable_flags;
 	data->dev_link = pmd_link;
 	data->dev_flags = RTE_ETH_DEV_INTR_LSC | RTE_ETH_DEV_CLOSE_REMOVE;
 
@@ -1278,35 +1326,13 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	eth_dev->rx_pkt_burst = eth_vhost_rx;
 	eth_dev->tx_pkt_burst = eth_vhost_tx;
 
-	if (rte_vhost_driver_register(iface_name, flags))
-		goto error;
-
-	if (disable_flags) {
-		if (rte_vhost_driver_disable_features(iface_name,
-					disable_flags))
-			goto error;
-	}
-
-	if (rte_vhost_driver_callback_register(iface_name, &vhost_ops) < 0) {
-		VHOST_LOG(ERR, "Can't register callbacks\n");
-		goto error;
-	}
-
-	if (rte_vhost_driver_start(iface_name) < 0) {
-		VHOST_LOG(ERR, "Failed to start driver for %s\n",
-			iface_name);
-		goto error;
-	}
-
 	rte_eth_dev_probing_finish(eth_dev);
 	return data->port_id;
 
 error:
 	if (internal)
 		rte_free(internal->iface_name);
-	rte_free(vring_state);
 	rte_eth_dev_release_port(eth_dev);
-	rte_free(list);
 
 	return -1;
 }
-- 
2.17.0

[dpdk-dev] [PATCH 4/4] net/vhost: make secondary probe complete

[Itsuro Oda]

add lacking member setting and make secondary probe complete.

Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
---
 drivers/net/vhost/rte_eth_vhost.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 44f44cea3..485a88794 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1390,8 +1390,11 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
 			VHOST_LOG(ERR, "Failed to probe %s\n", name);
 			return -1;
 		}
-		/* TODO: request info from primary to set up Rx and Tx */
+		eth_dev->rx_pkt_burst = eth_vhost_rx;
+		eth_dev->tx_pkt_burst = eth_vhost_tx;
 		eth_dev->dev_ops = &ops;
+		if (dev->device.numa_node == SOCKET_ID_ANY)
+			dev->device.numa_node = rte_socket_id();
 		eth_dev->device = &dev->device;
 		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
-- 
2.17.0

Re: [dpdk-dev] [PATCH 0/4] make vhost PMD available for secondary processes

[Itsuro ODA]

Hi vhost PMD maitainers,

I have not got any feedback yet. 
Since this is the first time I submit a patch, something
may be wrong, would you tell me what should I do ?

Thanks.

On Thu,  9 Jan 2020 08:22:05 +0900
Itsuro Oda <oda@valinux.co.jp> wrote:

> vhost PMD has not been available for secondary processes since
> DPDK v18.11.  (https://bugs.dpdk.org/show_bug.cgi?id=194)
> (for a long term !)
> This series of patches intend to make vhost PMD available for
> secondary processes.
> Because now setting vhost driver to communicate with a vhost-user
> master (ex. Qemu) is accomplished by the probe function of the
> primary process, only the primary process can be a vhost-user
> slave.
> With this patch, setting vhost driver is delayed at eth_dev
> configuration in order to be able to set it from a secondary
> process. Because (in the first place,) setting vhost driver is not
> necessary to be done at probe (it is enough to be done up to eth_dev
> start), this fix is no problem for the primary process.
> There is a precondition that the same process has to operate
> a vhost interface from beginning to end (from eth_dev configuration
> to eth_dev close). (This patch leaves it to user's responsibility.)
> This precondition will not be a problem in most use cases
> (including SPP).
> 
> v2:
> - add signed-off-by
> - fix spelling error
> 
> Itsuro Oda (4):
>   net/vhost: remove an unused member
>   net/vhost: allocate iface_name from heap
>   net/vhost: delay vhost driver setup
>   net/vhost: make secondary probe complete
> 
>  drivers/net/vhost/rte_eth_vhost.c | 152 +++++++++++++++++-------------
>  1 file changed, 88 insertions(+), 64 deletions(-)
> 
> -- 
> 2.17.0

-- 
Itsuro ODA <oda@valinux.co.jp>

Re: [dpdk-dev] [PATCH 0/4] make vhost PMD available for secondary processes

[Maxime Coquelin]

Hi Itsuro,

On 1/20/20 3:17 AM, Itsuro ODA wrote:
> Hi vhost PMD maitainers,
> 
> I have not got any feedback yet. 
> Since this is the first time I submit a patch, something
> may be wrong, would you tell me what should I do ?

Sorry for the delay, and thanks for the contribution.

You series does not apply properly on dpdk-next-virtio master branch:
https://git.dpdk.org/next/dpdk-next-virtio

I will review it, so when doing v3, please rebase it.

More generally, you series comprises fixes (patch 2 to 4), and
cleanup (patch 1).

Cleanup patch should be the last, in order to ease the backporting
of the fixes to LTSes (we avoid backporting cleanup patches).

Regarding fixes patches, it should tag the faulty commit in
master branch, and stable@dpdk.org should be Cc'ed.

Example of commit message with fixes tag:
http://patches.dpdk.org/patch/63305/

Finally, when you'll post the v3, please prefix the patches subject
with the revision number:

git format-patch --subject-prefix"PATCH v3" ...

Thanks,
Maxime

> Thanks.
> 
> On Thu,  9 Jan 2020 08:22:05 +0900
> Itsuro Oda <oda@valinux.co.jp> wrote:
> 
>> vhost PMD has not been available for secondary processes since
>> DPDK v18.11.  (https://bugs.dpdk.org/show_bug.cgi?id=194)
>> (for a long term !)
>> This series of patches intend to make vhost PMD available for
>> secondary processes.
>> Because now setting vhost driver to communicate with a vhost-user
>> master (ex. Qemu) is accomplished by the probe function of the
>> primary process, only the primary process can be a vhost-user
>> slave.
>> With this patch, setting vhost driver is delayed at eth_dev
>> configuration in order to be able to set it from a secondary
>> process. Because (in the first place,) setting vhost driver is not
>> necessary to be done at probe (it is enough to be done up to eth_dev
>> start), this fix is no problem for the primary process.
>> There is a precondition that the same process has to operate
>> a vhost interface from beginning to end (from eth_dev configuration
>> to eth_dev close). (This patch leaves it to user's responsibility.)
>> This precondition will not be a problem in most use cases
>> (including SPP).
>>
>> v2:
>> - add signed-off-by
>> - fix spelling error
>>
>> Itsuro Oda (4):
>>   net/vhost: remove an unused member
>>   net/vhost: allocate iface_name from heap
>>   net/vhost: delay vhost driver setup
>>   net/vhost: make secondary probe complete
>>
>>  drivers/net/vhost/rte_eth_vhost.c | 152 +++++++++++++++++-------------
>>  1 file changed, 88 insertions(+), 64 deletions(-)
>>
>> -- 
>> 2.17.0
> 

Re: [dpdk-dev] [PATCH 1/4] net/vhost: remove an unused member

[Maxime Coquelin]

On 1/9/20 12:22 AM, Itsuro Oda wrote:
> remove an unused member from pmd_internal.

Please try to do a complete sentence:

This patch removes an unused...

> Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
> ---
>  drivers/net/vhost/rte_eth_vhost.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 46f01a7f4..d4e3485ce 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -95,7 +95,6 @@ struct vhost_queue {
>  
>  struct pmd_internal {
>  	rte_atomic32_t dev_attached;
> -	char *dev_name;
>  	char *iface_name;
>  	uint16_t max_queues;
>  	int vid;
> @@ -1008,7 +1007,6 @@ eth_dev_close(struct rte_eth_dev *dev)
>  		for (i = 0; i < dev->data->nb_tx_queues; i++)
>  			rte_free(dev->data->tx_queues[i]);
>  
> -	free(internal->dev_name);
>  	free(internal->iface_name);
>  	rte_free(internal);
>  
> @@ -1253,9 +1251,6 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
>  	 * - and point eth_dev structure to new eth_dev_data structure
>  	 */
>  	internal = eth_dev->data->dev_private;
> -	internal->dev_name = strdup(name);
> -	if (internal->dev_name == NULL)
> -		goto error;
>  	internal->iface_name = strdup(iface_name);
>  	if (internal->iface_name == NULL)
>  		goto error;
> @@ -1305,10 +1300,8 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
>  	return data->port_id;
>  
>  error:
> -	if (internal) {
> +	if (internal)
>  		free(internal->iface_name);
> -		free(internal->dev_name);
> -	}
>  	rte_free(vring_state);
>  	rte_eth_dev_release_port(eth_dev);
>  	rte_free(list);
> 

Other than that, it looks good to me.
With the commit message fixed, and this patch moved to the end of the
series:

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

Thanks,
Maxime

Re: [dpdk-dev] [PATCH 2/4] net/vhost: allocate iface_name from heap

[Maxime Coquelin]

No variable or function names in the title, as per the contribution
guidelines:

net/vhost: allocate interface name from heap

On 1/9/20 12:22 AM, Itsuro Oda wrote:
> allocate iface_name of pmd_internal from heap in order to
> be able to refer from secondary processes.

As for patch, better to have a full sentence.

And remember to add Fixes tag and cc stable.

> Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
> ---
>  drivers/net/vhost/rte_eth_vhost.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index d4e3485ce..1b07aad24 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -1007,7 +1007,7 @@ eth_dev_close(struct rte_eth_dev *dev)
>  		for (i = 0; i < dev->data->nb_tx_queues; i++)
>  			rte_free(dev->data->tx_queues[i]);
>  
> -	free(internal->iface_name);
> +	rte_free(internal->iface_name);
>  	rte_free(internal);
>  
>  	dev->data->dev_private = NULL;
> @@ -1251,9 +1251,11 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
>  	 * - and point eth_dev structure to new eth_dev_data structure
>  	 */
>  	internal = eth_dev->data->dev_private;
> -	internal->iface_name = strdup(iface_name);
> +	internal->iface_name = rte_malloc_socket(name, strlen(iface_name) + 1,
> +						 0, numa_node);
>  	if (internal->iface_name == NULL)
>  		goto error;
> +	strcpy(internal->iface_name, iface_name);
>  
>  	list->eth_dev = eth_dev;
>  	pthread_mutex_lock(&internal_list_lock);
> @@ -1301,7 +1303,7 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
>  
>  error:
>  	if (internal)
> -		free(internal->iface_name);
> +		rte_free(internal->iface_name);
>  	rte_free(vring_state);
>  	rte_eth_dev_release_port(eth_dev);
>  	rte_free(list);
> 


When above comments taken into account:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [dpdk-dev] [PATCH 3/4] net/vhost: delay vhost driver setup

[Maxime Coquelin]

On 1/9/20 12:22 AM, Itsuro Oda wrote:
> setting vhost driver is delayed at eth_dev configuration
Sentences starts with an upper-case.

Vhost driver setup is delayed...

> in order to be able to set it from a secondary process.


Add Fixes tag and cc stable.

> Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
> ---
>  drivers/net/vhost/rte_eth_vhost.c | 130 ++++++++++++++++++------------
>  1 file changed, 78 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 1b07aad24..44f44cea3 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -96,6 +96,8 @@ struct vhost_queue {
>  struct pmd_internal {
>  	rte_atomic32_t dev_attached;
>  	char *iface_name;
> +	uint64_t flags;
> +	uint64_t disable_flags;
>  	uint16_t max_queues;
>  	int vid;
>  	rte_atomic32_t started;
> @@ -490,17 +492,6 @@ eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
>  	return nb_tx;
>  }
>  
> -static int
> -eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
> -{
> -	struct pmd_internal *internal = dev->data->dev_private;
> -	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
> -
> -	internal->vlan_strip = !!(rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP);
> -
> -	return 0;
> -}
> -
>  static inline struct internal_list *
>  find_internal_resource(char *ifname)
>  {
> @@ -876,6 +867,62 @@ static struct vhost_device_ops vhost_ops = {
>  	.vring_state_changed = vring_state_changed,
>  };
>  
> +static int
> +vhost_driver_setup(struct rte_eth_dev *eth_dev)
> +{
> +	struct pmd_internal *internal = eth_dev->data->dev_private;
> +	struct internal_list *list = NULL;
> +	struct rte_vhost_vring_state *vring_state = NULL;
> +	unsigned int numa_node = eth_dev->device->numa_node;
> +	const char *name = eth_dev->device->name;
> +
> +	list = rte_zmalloc_socket(name, sizeof(*list), 0, numa_node);
> +	if (list == NULL)
> +		goto error;
> +
> +	vring_state = rte_zmalloc_socket(name, sizeof(*vring_state),
> +					 0, numa_node);
> +	if (vring_state == NULL)
> +		goto error;
> +
> +	list->eth_dev = eth_dev;
> +	pthread_mutex_lock(&internal_list_lock);
> +	TAILQ_INSERT_TAIL(&internal_list, list, next);
> +	pthread_mutex_unlock(&internal_list_lock);
> +
> +	rte_spinlock_init(&vring_state->lock);
> +	vring_states[eth_dev->data->port_id] = vring_state;
> +
> +	if (rte_vhost_driver_register(internal->iface_name, internal->flags))
> +		goto error;
> +
> +	if (internal->disable_flags) {
> +		if (rte_vhost_driver_disable_features(internal->iface_name,
> +						      internal->disable_flags))
> +			goto error;
> +	}
> +
> +	if (rte_vhost_driver_callback_register(internal->iface_name,
> +					       &vhost_ops) < 0) {
> +		VHOST_LOG(ERR, "Can't register callbacks\n");
> +		goto error;
> +	}
> +
> +	if (rte_vhost_driver_start(internal->iface_name) < 0) {
> +		VHOST_LOG(ERR, "Failed to start driver for %s\n",
> +			  internal->iface_name);
> +		goto error;
> +	}
> +
> +	return 0;
> +
> +error:
> +	rte_free(vring_state);
> +	rte_free(list);
> +
> +	return -1;
> +}
> +
>  int
>  rte_eth_vhost_get_queue_event(uint16_t port_id,
>  		struct rte_eth_vhost_queue_event *event)
> @@ -942,6 +989,24 @@ rte_eth_vhost_get_vid_from_port_id(uint16_t port_id)
>  	return vid;
>  }
>  
> +static int
> +eth_dev_configure(struct rte_eth_dev *dev)
> +{
> +	struct pmd_internal *internal = dev->data->dev_private;
> +	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
> +
> +	/* NOTE: the same process has to operate a vhost interface
> +	 * from beginning to end (from eth_dev configure to eth_dev close).
> +	 * It is user's responsibility at the moment.
> +	 */
> +	if (vhost_driver_setup(dev) < 0)
> +		return -1;
> +
> +	internal->vlan_strip = !!(rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP);
> +
> +	return 0;
> +}
> +
>  static int
>  eth_dev_start(struct rte_eth_dev *eth_dev)
>  {
> @@ -1217,16 +1282,10 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
>  	struct pmd_internal *internal = NULL;
>  	struct rte_eth_dev *eth_dev = NULL;
>  	struct rte_ether_addr *eth_addr = NULL;
> -	struct rte_vhost_vring_state *vring_state = NULL;
> -	struct internal_list *list = NULL;
>  
>  	VHOST_LOG(INFO, "Creating VHOST-USER backend on numa socket %u\n",
>  		numa_node);
>  
> -	list = rte_zmalloc_socket(name, sizeof(*list), 0, numa_node);
> -	if (list == NULL)
> -		goto error;
> -
>  	/* reserve an ethdev entry */
>  	eth_dev = rte_eth_vdev_allocate(dev, sizeof(*internal));
>  	if (eth_dev == NULL)
> @@ -1240,11 +1299,6 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
>  	*eth_addr = base_eth_addr;
>  	eth_addr->addr_bytes[5] = eth_dev->data->port_id;
>  
> -	vring_state = rte_zmalloc_socket(name,
> -			sizeof(*vring_state), 0, numa_node);
> -	if (vring_state == NULL)
> -		goto error;
> -
>  	/* now put it all together
>  	 * - store queue data in internal,
>  	 * - point eth_dev_data to internals
> @@ -1257,18 +1311,12 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
>  		goto error;
>  	strcpy(internal->iface_name, iface_name);
>  
> -	list->eth_dev = eth_dev;
> -	pthread_mutex_lock(&internal_list_lock);
> -	TAILQ_INSERT_TAIL(&internal_list, list, next);
> -	pthread_mutex_unlock(&internal_list_lock);
> -
> -	rte_spinlock_init(&vring_state->lock);
> -	vring_states[eth_dev->data->port_id] = vring_state;
> -
>  	data->nb_rx_queues = queues;
>  	data->nb_tx_queues = queues;
>  	internal->max_queues = queues;
>  	internal->vid = -1;
> +	internal->flags = flags;
> +	internal->disable_flags = disable_flags;
>  	data->dev_link = pmd_link;
>  	data->dev_flags = RTE_ETH_DEV_INTR_LSC | RTE_ETH_DEV_CLOSE_REMOVE;
>  
> @@ -1278,35 +1326,13 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
>  	eth_dev->rx_pkt_burst = eth_vhost_rx;
>  	eth_dev->tx_pkt_burst = eth_vhost_tx;
>  
> -	if (rte_vhost_driver_register(iface_name, flags))
> -		goto error;
> -
> -	if (disable_flags) {
> -		if (rte_vhost_driver_disable_features(iface_name,
> -					disable_flags))
> -			goto error;
> -	}
> -
> -	if (rte_vhost_driver_callback_register(iface_name, &vhost_ops) < 0) {
> -		VHOST_LOG(ERR, "Can't register callbacks\n");
> -		goto error;
> -	}
> -
> -	if (rte_vhost_driver_start(iface_name) < 0) {
> -		VHOST_LOG(ERR, "Failed to start driver for %s\n",
> -			iface_name);
> -		goto error;
> -	}
> -
>  	rte_eth_dev_probing_finish(eth_dev);
>  	return data->port_id;
>  
>  error:
>  	if (internal)
>  		rte_free(internal->iface_name);
> -	rte_free(vring_state);
>  	rte_eth_dev_release_port(eth_dev);
> -	rte_free(list);
>  
>  	return -1;
>  }
> 

With above comments fixed:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [dpdk-dev] [PATCH 4/4] net/vhost: make secondary probe complete

[Maxime Coquelin]

On 1/9/20 12:22 AM, Itsuro Oda wrote:
> add lacking member setting and make secondary probe complete.

Add

> Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
> ---
>  drivers/net/vhost/rte_eth_vhost.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 44f44cea3..485a88794 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -1390,8 +1390,11 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
>  			VHOST_LOG(ERR, "Failed to probe %s\n", name);
>  			return -1;
>  		}
> -		/* TODO: request info from primary to set up Rx and Tx */
> +		eth_dev->rx_pkt_burst = eth_vhost_rx;
> +		eth_dev->tx_pkt_burst = eth_vhost_tx;
>  		eth_dev->dev_ops = &ops;
> +		if (dev->device.numa_node == SOCKET_ID_ANY)
> +			dev->device.numa_node = rte_socket_id();
>  		eth_dev->device = &dev->device;
>  		rte_eth_dev_probing_finish(eth_dev);
>  		return 0;
> 

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

Thanks,
Maxime

Re: [dpdk-dev] [PATCH 0/4] make vhost PMD available for secondary processes

[Itsuro ODA]

Hi Maxime,

Thank you for your reply and review.
I will make fixes of the patches according to your
indication and post them.

Thanks.
Itsuro Oda

On Tue, 4 Feb 2020 18:54:23 +0100
Maxime Coquelin <maxime.coquelin@redhat.com> wrote:

> Hi Itsuro,
> 
> On 1/20/20 3:17 AM, Itsuro ODA wrote:
> > Hi vhost PMD maitainers,
> > 
> > I have not got any feedback yet. 
> > Since this is the first time I submit a patch, something
> > may be wrong, would you tell me what should I do ?
> 
> Sorry for the delay, and thanks for the contribution.
> 
> You series does not apply properly on dpdk-next-virtio master branch:
> https://git.dpdk.org/next/dpdk-next-virtio
> 
> I will review it, so when doing v3, please rebase it.
> 
> More generally, you series comprises fixes (patch 2 to 4), and
> cleanup (patch 1).
> 
> Cleanup patch should be the last, in order to ease the backporting
> of the fixes to LTSes (we avoid backporting cleanup patches).
> 
> Regarding fixes patches, it should tag the faulty commit in
> master branch, and stable@dpdk.org should be Cc'ed.
> 
> Example of commit message with fixes tag:
> http://patches.dpdk.org/patch/63305/
> 
> Finally, when you'll post the v3, please prefix the patches subject
> with the revision number:
> 
> git format-patch --subject-prefix"PATCH v3" ...
> 
> Thanks,
> Maxime
> 
> > Thanks.
> > 
> > On Thu,  9 Jan 2020 08:22:05 +0900
> > Itsuro Oda <oda@valinux.co.jp> wrote:
> > 
> >> vhost PMD has not been available for secondary processes since
> >> DPDK v18.11.  (https://bugs.dpdk.org/show_bug.cgi?id=194)
> >> (for a long term !)
> >> This series of patches intend to make vhost PMD available for
> >> secondary processes.
> >> Because now setting vhost driver to communicate with a vhost-user
> >> master (ex. Qemu) is accomplished by the probe function of the
> >> primary process, only the primary process can be a vhost-user
> >> slave.
> >> With this patch, setting vhost driver is delayed at eth_dev
> >> configuration in order to be able to set it from a secondary
> >> process. Because (in the first place,) setting vhost driver is not
> >> necessary to be done at probe (it is enough to be done up to eth_dev
> >> start), this fix is no problem for the primary process.
> >> There is a precondition that the same process has to operate
> >> a vhost interface from beginning to end (from eth_dev configuration
> >> to eth_dev close). (This patch leaves it to user's responsibility.)
> >> This precondition will not be a problem in most use cases
> >> (including SPP).
> >>
> >> v2:
> >> - add signed-off-by
> >> - fix spelling error
> >>
> >> Itsuro Oda (4):
> >>   net/vhost: remove an unused member
> >>   net/vhost: allocate iface_name from heap
> >>   net/vhost: delay vhost driver setup
> >>   net/vhost: make secondary probe complete
> >>
> >>  drivers/net/vhost/rte_eth_vhost.c | 152 +++++++++++++++++-------------
> >>  1 file changed, 88 insertions(+), 64 deletions(-)
> >>
> >> -- 
> >> 2.17.0
> > 

-- 
Itsuro ODA <oda@valinux.co.jp>

[dpdk-dev] [PATCH v3 0/4] make vhost PMD available for secondary processes

[Itsuro Oda]

vhost PMD has not been available for secondary processes since
DPDK v18.11.  (https://bugs.dpdk.org/show_bug.cgi?id=194)
(for a long term !)
This series of patches intend to make vhost PMD available for
secondary processes.
Because now setting vhost driver to communicate with a vhost-user
master (ex. Qemu) is accomplished by the probe function of the
primary process, only the primary process can be a vhost-user
slave.
With this patch, setting vhost driver is delayed at eth_dev
configuration in order to be able to set it from a secondary
process. Because (in the first place,) setting vhost driver is not
necessary to be done at probe (it is enough to be done up to eth_dev
start), this fix is no problem for the primary process.
There is a precondition that the same process has to operate
a vhost interface from beginning to end (from eth_dev configuration
to eth_dev close). (This patch leaves it to user's responsibility.)
This precondition will not be a problem in most use cases
(including SPP).

v2:
- add signed-off-by
- fix spelling error

v3:
- rebase on dpdk-next-virtio master
- change patch order
- fix subject and commit message

Itsuro Oda (4):
  net/vhost: allocate interface name from heap
  net/vhost: delay vhost driver setup
  net/vhost: make secondary probe complete
  net/vhost: remove an unused member

 drivers/net/vhost/rte_eth_vhost.c | 152 +++++++++++++++++-------------
 1 file changed, 88 insertions(+), 64 deletions(-)

-- 
2.17.0

[dpdk-dev] [PATCH v3 1/4] net/vhost: allocate interface name from heap

[Itsuro Oda]

This patch allocates iface_name of pmd_internal from heap
in order to be able to refer from secondary processes.

Fixes: 4852aa8f6e21 (drivers/net: enable hotplug on secondary process)
Cc: stable@dpdk.org

Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index a63588986..cea2ead2d 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1009,7 +1009,7 @@ eth_dev_close(struct rte_eth_dev *dev)
 			rte_free(dev->data->tx_queues[i]);
 
 	free(internal->dev_name);
-	free(internal->iface_name);
+	rte_free(internal->iface_name);
 	rte_free(internal);
 
 	dev->data->dev_private = NULL;
@@ -1256,9 +1256,11 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	internal->dev_name = strdup(name);
 	if (internal->dev_name == NULL)
 		goto error;
-	internal->iface_name = strdup(iface_name);
+	internal->iface_name = rte_malloc_socket(name, strlen(iface_name) + 1,
+						 0, numa_node);
 	if (internal->iface_name == NULL)
 		goto error;
+	strcpy(internal->iface_name, iface_name);
 
 	list->eth_dev = eth_dev;
 	pthread_mutex_lock(&internal_list_lock);
@@ -1306,7 +1308,7 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 
 error:
 	if (internal) {
-		free(internal->iface_name);
+		rte_free(internal->iface_name);
 		free(internal->dev_name);
 	}
 	rte_free(vring_state);
-- 
2.17.0

[dpdk-dev] [PATCH v3 2/4] net/vhost: delay vhost driver setup

[Itsuro Oda]

Vhost driver setup is delayed at eth_dev configuration
in order to be able to set it from a secondary process.

Fixes: 4852aa8f6e21 (drivers/net: enable hotplug on secondary process)
Cc: stable@dpdk.org

Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 130 ++++++++++++++++++------------
 1 file changed, 78 insertions(+), 52 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index cea2ead2d..d7bba5c6e 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -97,6 +97,8 @@ struct pmd_internal {
 	rte_atomic32_t dev_attached;
 	char *dev_name;
 	char *iface_name;
+	uint64_t flags;
+	uint64_t disable_flags;
 	uint16_t max_queues;
 	int vid;
 	rte_atomic32_t started;
@@ -491,17 +493,6 @@ eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
 	return nb_tx;
 }
 
-static int
-eth_dev_configure(struct rte_eth_dev *dev __rte_unused)
-{
-	struct pmd_internal *internal = dev->data->dev_private;
-	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
-
-	internal->vlan_strip = !!(rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP);
-
-	return 0;
-}
-
 static inline struct internal_list *
 find_internal_resource(char *ifname)
 {
@@ -877,6 +868,62 @@ static struct vhost_device_ops vhost_ops = {
 	.vring_state_changed = vring_state_changed,
 };
 
+static int
+vhost_driver_setup(struct rte_eth_dev *eth_dev)
+{
+	struct pmd_internal *internal = eth_dev->data->dev_private;
+	struct internal_list *list = NULL;
+	struct rte_vhost_vring_state *vring_state = NULL;
+	unsigned int numa_node = eth_dev->device->numa_node;
+	const char *name = eth_dev->device->name;
+
+	list = rte_zmalloc_socket(name, sizeof(*list), 0, numa_node);
+	if (list == NULL)
+		goto error;
+
+	vring_state = rte_zmalloc_socket(name, sizeof(*vring_state),
+					 0, numa_node);
+	if (vring_state == NULL)
+		goto error;
+
+	list->eth_dev = eth_dev;
+	pthread_mutex_lock(&internal_list_lock);
+	TAILQ_INSERT_TAIL(&internal_list, list, next);
+	pthread_mutex_unlock(&internal_list_lock);
+
+	rte_spinlock_init(&vring_state->lock);
+	vring_states[eth_dev->data->port_id] = vring_state;
+
+	if (rte_vhost_driver_register(internal->iface_name, internal->flags))
+		goto error;
+
+	if (internal->disable_flags) {
+		if (rte_vhost_driver_disable_features(internal->iface_name,
+						      internal->disable_flags))
+			goto error;
+	}
+
+	if (rte_vhost_driver_callback_register(internal->iface_name,
+					       &vhost_ops) < 0) {
+		VHOST_LOG(ERR, "Can't register callbacks\n");
+		goto error;
+	}
+
+	if (rte_vhost_driver_start(internal->iface_name) < 0) {
+		VHOST_LOG(ERR, "Failed to start driver for %s\n",
+			  internal->iface_name);
+		goto error;
+	}
+
+	return 0;
+
+error:
+	rte_free(vring_state);
+	rte_free(list);
+
+	return -1;
+}
+
 int
 rte_eth_vhost_get_queue_event(uint16_t port_id,
 		struct rte_eth_vhost_queue_event *event)
@@ -943,6 +990,24 @@ rte_eth_vhost_get_vid_from_port_id(uint16_t port_id)
 	return vid;
 }
 
+static int
+eth_dev_configure(struct rte_eth_dev *dev)
+{
+	struct pmd_internal *internal = dev->data->dev_private;
+	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
+
+	/* NOTE: the same process has to operate a vhost interface
+	 * from beginning to end (from eth_dev configure to eth_dev close).
+	 * It is user's responsibility at the moment.
+	 */
+	if (vhost_driver_setup(dev) < 0)
+		return -1;
+
+	internal->vlan_strip = !!(rxmode->offloads & DEV_RX_OFFLOAD_VLAN_STRIP);
+
+	return 0;
+}
+
 static int
 eth_dev_start(struct rte_eth_dev *eth_dev)
 {
@@ -1219,16 +1284,10 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	struct pmd_internal *internal = NULL;
 	struct rte_eth_dev *eth_dev = NULL;
 	struct rte_ether_addr *eth_addr = NULL;
-	struct rte_vhost_vring_state *vring_state = NULL;
-	struct internal_list *list = NULL;
 
 	VHOST_LOG(INFO, "Creating VHOST-USER backend on numa socket %u\n",
 		numa_node);
 
-	list = rte_zmalloc_socket(name, sizeof(*list), 0, numa_node);
-	if (list == NULL)
-		goto error;
-
 	/* reserve an ethdev entry */
 	eth_dev = rte_eth_vdev_allocate(dev, sizeof(*internal));
 	if (eth_dev == NULL)
@@ -1242,11 +1301,6 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	*eth_addr = base_eth_addr;
 	eth_addr->addr_bytes[5] = eth_dev->data->port_id;
 
-	vring_state = rte_zmalloc_socket(name,
-			sizeof(*vring_state), 0, numa_node);
-	if (vring_state == NULL)
-		goto error;
-
 	/* now put it all together
 	 * - store queue data in internal,
 	 * - point eth_dev_data to internals
@@ -1262,18 +1316,12 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 		goto error;
 	strcpy(internal->iface_name, iface_name);
 
-	list->eth_dev = eth_dev;
-	pthread_mutex_lock(&internal_list_lock);
-	TAILQ_INSERT_TAIL(&internal_list, list, next);
-	pthread_mutex_unlock(&internal_list_lock);
-
-	rte_spinlock_init(&vring_state->lock);
-	vring_states[eth_dev->data->port_id] = vring_state;
-
 	data->nb_rx_queues = queues;
 	data->nb_tx_queues = queues;
 	internal->max_queues = queues;
 	internal->vid = -1;
+	internal->flags = flags;
+	internal->disable_flags = disable_flags;
 	data->dev_link = pmd_link;
 	data->dev_flags = RTE_ETH_DEV_INTR_LSC | RTE_ETH_DEV_CLOSE_REMOVE;
 
@@ -1283,26 +1331,6 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	eth_dev->rx_pkt_burst = eth_vhost_rx;
 	eth_dev->tx_pkt_burst = eth_vhost_tx;
 
-	if (rte_vhost_driver_register(iface_name, flags))
-		goto error;
-
-	if (disable_flags) {
-		if (rte_vhost_driver_disable_features(iface_name,
-					disable_flags))
-			goto error;
-	}
-
-	if (rte_vhost_driver_callback_register(iface_name, &vhost_ops) < 0) {
-		VHOST_LOG(ERR, "Can't register callbacks\n");
-		goto error;
-	}
-
-	if (rte_vhost_driver_start(iface_name) < 0) {
-		VHOST_LOG(ERR, "Failed to start driver for %s\n",
-			iface_name);
-		goto error;
-	}
-
 	rte_eth_dev_probing_finish(eth_dev);
 	return 0;
 
@@ -1311,9 +1339,7 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 		rte_free(internal->iface_name);
 		free(internal->dev_name);
 	}
-	rte_free(vring_state);
 	rte_eth_dev_release_port(eth_dev);
-	rte_free(list);
 
 	return -1;
 }
-- 
2.17.0

[dpdk-dev] [PATCH v3 3/4] net/vhost: make secondary probe complete

[Itsuro Oda]

This patch adds lacking member setting and makes secondary
probe complete.

Fixes: 4852aa8f6e21 (drivers/net: enable hotplug on secondary process)
Cc: stable@dpdk.org

Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index d7bba5c6e..307de2c68 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -1397,8 +1397,11 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
 			VHOST_LOG(ERR, "Failed to probe %s\n", name);
 			return -1;
 		}
-		/* TODO: request info from primary to set up Rx and Tx */
+		eth_dev->rx_pkt_burst = eth_vhost_rx;
+		eth_dev->tx_pkt_burst = eth_vhost_tx;
 		eth_dev->dev_ops = &ops;
+		if (dev->device.numa_node == SOCKET_ID_ANY)
+			dev->device.numa_node = rte_socket_id();
 		eth_dev->device = &dev->device;
 		rte_eth_dev_probing_finish(eth_dev);
 		return 0;
-- 
2.17.0

[dpdk-dev] [PATCH v3 4/4] net/vhost: remove an unused member

[Itsuro Oda]

This patch removes an unused member from pmd_internal.

Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 307de2c68..90263ae77 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -95,7 +95,6 @@ struct vhost_queue {
 
 struct pmd_internal {
 	rte_atomic32_t dev_attached;
-	char *dev_name;
 	char *iface_name;
 	uint64_t flags;
 	uint64_t disable_flags;
@@ -1073,7 +1072,6 @@ eth_dev_close(struct rte_eth_dev *dev)
 		for (i = 0; i < dev->data->nb_tx_queues; i++)
 			rte_free(dev->data->tx_queues[i]);
 
-	free(internal->dev_name);
 	rte_free(internal->iface_name);
 	rte_free(internal);
 
@@ -1307,9 +1305,6 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	 * - and point eth_dev structure to new eth_dev_data structure
 	 */
 	internal = eth_dev->data->dev_private;
-	internal->dev_name = strdup(name);
-	if (internal->dev_name == NULL)
-		goto error;
 	internal->iface_name = rte_malloc_socket(name, strlen(iface_name) + 1,
 						 0, numa_node);
 	if (internal->iface_name == NULL)
@@ -1335,10 +1330,8 @@ eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
 	return 0;
 
 error:
-	if (internal) {
+	if (internal)
 		rte_free(internal->iface_name);
-		free(internal->dev_name);
-	}
 	rte_eth_dev_release_port(eth_dev);
 
 	return -1;
-- 
2.17.0

Re: [dpdk-dev] [PATCH v3 0/4] make vhost PMD available for secondary processes

[Maxime Coquelin]

On 2/6/20 2:39 AM, Itsuro Oda wrote:
> vhost PMD has not been available for secondary processes since
> DPDK v18.11.  (https://bugs.dpdk.org/show_bug.cgi?id=194)
> (for a long term !)
> This series of patches intend to make vhost PMD available for
> secondary processes.
> Because now setting vhost driver to communicate with a vhost-user
> master (ex. Qemu) is accomplished by the probe function of the
> primary process, only the primary process can be a vhost-user
> slave.
> With this patch, setting vhost driver is delayed at eth_dev
> configuration in order to be able to set it from a secondary
> process. Because (in the first place,) setting vhost driver is not
> necessary to be done at probe (it is enough to be done up to eth_dev
> start), this fix is no problem for the primary process.
> There is a precondition that the same process has to operate
> a vhost interface from beginning to end (from eth_dev configuration
> to eth_dev close). (This patch leaves it to user's responsibility.)
> This precondition will not be a problem in most use cases
> (including SPP).
> 
> v2:
> - add signed-off-by
> - fix spelling error
> 
> v3:
> - rebase on dpdk-next-virtio master
> - change patch order
> - fix subject and commit message
> 
> Itsuro Oda (4):
>   net/vhost: allocate interface name from heap
>   net/vhost: delay vhost driver setup
>   net/vhost: make secondary probe complete
>   net/vhost: remove an unused member
> 
>  drivers/net/vhost/rte_eth_vhost.c | 152 +++++++++++++++++-------------
>  1 file changed, 88 insertions(+), 64 deletions(-)
> 

Thanks Itsuro, it looks good to me.
I will pull the series for v20.02-rc3.

Maxime

Re: [dpdk-dev] [PATCH v3 0/4] make vhost PMD available for secondary processes

[Maxime Coquelin]

On 2/6/20 2:39 AM, Itsuro Oda wrote:
> vhost PMD has not been available for secondary processes since
> DPDK v18.11.  (https://bugs.dpdk.org/show_bug.cgi?id=194)
> (for a long term !)
> This series of patches intend to make vhost PMD available for
> secondary processes.
> Because now setting vhost driver to communicate with a vhost-user
> master (ex. Qemu) is accomplished by the probe function of the
> primary process, only the primary process can be a vhost-user
> slave.
> With this patch, setting vhost driver is delayed at eth_dev
> configuration in order to be able to set it from a secondary
> process. Because (in the first place,) setting vhost driver is not
> necessary to be done at probe (it is enough to be done up to eth_dev
> start), this fix is no problem for the primary process.
> There is a precondition that the same process has to operate
> a vhost interface from beginning to end (from eth_dev configuration
> to eth_dev close). (This patch leaves it to user's responsibility.)
> This precondition will not be a problem in most use cases
> (including SPP).
> 
> v2:
> - add signed-off-by
> - fix spelling error
> 
> v3:
> - rebase on dpdk-next-virtio master
> - change patch order
> - fix subject and commit message
> 
> Itsuro Oda (4):
>   net/vhost: allocate interface name from heap
>   net/vhost: delay vhost driver setup
>   net/vhost: make secondary probe complete
>   net/vhost: remove an unused member
> 
>  drivers/net/vhost/rte_eth_vhost.c | 152 +++++++++++++++++-------------
>  1 file changed, 88 insertions(+), 64 deletions(-)
> 

Applied to dpdk-next-virtio/master

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v3 2/4] net/vhost: delay vhost driver setup

[Wang, Yinan]

Hi Itsuro/Maxime,

Below patch (commit id:3d01b759d2679c216725689eabe44147d1737326)blocked vhost/virtio basic test: If re-config vhost port，testpmd will launch failed.

Reproduce steps:

./x86_64-native-linuxapp-gcc/app/testpmd -l 2-4 -n 4 --socket-mem 1024,1024  --legacy-mem --file-prefix=vhost --vdev 'net_vhost0,iface=vhost-net,queues=1,client=0' --no-pci -- -i --txd=1024 --rxd=1024
    testpmd>set fwd csum
    testpmd>stop
    testpmd>port stop 0
    testpmd>csum set tcp sw 0
    testpmd>port start 0

VHOST_CONFIG: vhost-user server: socket created, fd: 26
VHOST_CONFIG: failed to bind to vhost-net: Address already in use; remove it and try again
Failed to start driver for vhost-net
Port0 dev_configure = -1
Fail to configure port 0

BR,
Yinan
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Itsuro Oda
> Sent: 2020年2月6日 9:40
> To: dev@dpdk.org; maxime.coquelin@redhat.com; Bie, Tiwei
> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>; Burakov,
> Anatoly <anatoly.burakov@intel.com>
> Cc: stable@dpdk.org
> Subject: [dpdk-dev] [PATCH v3 2/4] net/vhost: delay vhost driver setup
> 
> Vhost driver setup is delayed at eth_dev configuration in order to be able to set
> it from a secondary process.
> 
> Fixes: 4852aa8f6e21 (drivers/net: enable hotplug on secondary process)
> Cc: stable@dpdk.org
> 
> Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/vhost/rte_eth_vhost.c | 130 ++++++++++++++++++------------
>  1 file changed, 78 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> b/drivers/net/vhost/rte_eth_vhost.c
> index cea2ead2d..d7bba5c6e 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -97,6 +97,8 @@ struct pmd_internal {
>  	rte_atomic32_t dev_attached;
>  	char *dev_name;
>  	char *iface_name;
> +	uint64_t flags;
> +	uint64_t disable_flags;
>  	uint16_t max_queues;
>  	int vid;
>  	rte_atomic32_t started;
> @@ -491,17 +493,6 @@ eth_vhost_tx(void *q, struct rte_mbuf **bufs,
> uint16_t nb_bufs)
>  	return nb_tx;
>  }
> 
> -static int
> -eth_dev_configure(struct rte_eth_dev *dev __rte_unused) -{
> -	struct pmd_internal *internal = dev->data->dev_private;
> -	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
> -
> -	internal->vlan_strip = !!(rxmode->offloads &
> DEV_RX_OFFLOAD_VLAN_STRIP);
> -
> -	return 0;
> -}
> -
>  static inline struct internal_list *
>  find_internal_resource(char *ifname)
>  {
> @@ -877,6 +868,62 @@ static struct vhost_device_ops vhost_ops = {
>  	.vring_state_changed = vring_state_changed,  };
> 
> +static int
> +vhost_driver_setup(struct rte_eth_dev *eth_dev) {
> +	struct pmd_internal *internal = eth_dev->data->dev_private;
> +	struct internal_list *list = NULL;
> +	struct rte_vhost_vring_state *vring_state = NULL;
> +	unsigned int numa_node = eth_dev->device->numa_node;
> +	const char *name = eth_dev->device->name;
> +
> +	list = rte_zmalloc_socket(name, sizeof(*list), 0, numa_node);
> +	if (list == NULL)
> +		goto error;
> +
> +	vring_state = rte_zmalloc_socket(name, sizeof(*vring_state),
> +					 0, numa_node);
> +	if (vring_state == NULL)
> +		goto error;
> +
> +	list->eth_dev = eth_dev;
> +	pthread_mutex_lock(&internal_list_lock);
> +	TAILQ_INSERT_TAIL(&internal_list, list, next);
> +	pthread_mutex_unlock(&internal_list_lock);
> +
> +	rte_spinlock_init(&vring_state->lock);
> +	vring_states[eth_dev->data->port_id] = vring_state;
> +
> +	if (rte_vhost_driver_register(internal->iface_name, internal->flags))
> +		goto error;
> +
> +	if (internal->disable_flags) {
> +		if (rte_vhost_driver_disable_features(internal->iface_name,
> +						      internal->disable_flags))
> +			goto error;
> +	}
> +
> +	if (rte_vhost_driver_callback_register(internal->iface_name,
> +					       &vhost_ops) < 0) {
> +		VHOST_LOG(ERR, "Can't register callbacks\n");
> +		goto error;
> +	}
> +
> +	if (rte_vhost_driver_start(internal->iface_name) < 0) {
> +		VHOST_LOG(ERR, "Failed to start driver for %s\n",
> +			  internal->iface_name);
> +		goto error;
> +	}
> +
> +	return 0;
> +
> +error:
> +	rte_free(vring_state);
> +	rte_free(list);
> +
> +	return -1;
> +}
> +
>  int
>  rte_eth_vhost_get_queue_event(uint16_t port_id,
>  		struct rte_eth_vhost_queue_event *event) @@ -943,6 +990,24 @@
> rte_eth_vhost_get_vid_from_port_id(uint16_t port_id)
>  	return vid;
>  }
> 
> +static int
> +eth_dev_configure(struct rte_eth_dev *dev) {
> +	struct pmd_internal *internal = dev->data->dev_private;
> +	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
> +
> +	/* NOTE: the same process has to operate a vhost interface
> +	 * from beginning to end (from eth_dev configure to eth_dev close).
> +	 * It is user's responsibility at the moment.
> +	 */
> +	if (vhost_driver_setup(dev) < 0)
> +		return -1;
> +
> +	internal->vlan_strip = !!(rxmode->offloads &
> +DEV_RX_OFFLOAD_VLAN_STRIP);
> +
> +	return 0;
> +}
> +
>  static int
>  eth_dev_start(struct rte_eth_dev *eth_dev)  { @@ -1219,16 +1284,10 @@
> eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
>  	struct pmd_internal *internal = NULL;
>  	struct rte_eth_dev *eth_dev = NULL;
>  	struct rte_ether_addr *eth_addr = NULL;
> -	struct rte_vhost_vring_state *vring_state = NULL;
> -	struct internal_list *list = NULL;
> 
>  	VHOST_LOG(INFO, "Creating VHOST-USER backend on numa
> socket %u\n",
>  		numa_node);
> 
> -	list = rte_zmalloc_socket(name, sizeof(*list), 0, numa_node);
> -	if (list == NULL)
> -		goto error;
> -
>  	/* reserve an ethdev entry */
>  	eth_dev = rte_eth_vdev_allocate(dev, sizeof(*internal));
>  	if (eth_dev == NULL)
> @@ -1242,11 +1301,6 @@ eth_dev_vhost_create(struct rte_vdev_device *dev,
> char *iface_name,
>  	*eth_addr = base_eth_addr;
>  	eth_addr->addr_bytes[5] = eth_dev->data->port_id;
> 
> -	vring_state = rte_zmalloc_socket(name,
> -			sizeof(*vring_state), 0, numa_node);
> -	if (vring_state == NULL)
> -		goto error;
> -
>  	/* now put it all together
>  	 * - store queue data in internal,
>  	 * - point eth_dev_data to internals
> @@ -1262,18 +1316,12 @@ eth_dev_vhost_create(struct rte_vdev_device
> *dev, char *iface_name,
>  		goto error;
>  	strcpy(internal->iface_name, iface_name);
> 
> -	list->eth_dev = eth_dev;
> -	pthread_mutex_lock(&internal_list_lock);
> -	TAILQ_INSERT_TAIL(&internal_list, list, next);
> -	pthread_mutex_unlock(&internal_list_lock);
> -
> -	rte_spinlock_init(&vring_state->lock);
> -	vring_states[eth_dev->data->port_id] = vring_state;
> -
>  	data->nb_rx_queues = queues;
>  	data->nb_tx_queues = queues;
>  	internal->max_queues = queues;
>  	internal->vid = -1;
> +	internal->flags = flags;
> +	internal->disable_flags = disable_flags;
>  	data->dev_link = pmd_link;
>  	data->dev_flags = RTE_ETH_DEV_INTR_LSC |
> RTE_ETH_DEV_CLOSE_REMOVE;
> 
> @@ -1283,26 +1331,6 @@ eth_dev_vhost_create(struct rte_vdev_device *dev,
> char *iface_name,
>  	eth_dev->rx_pkt_burst = eth_vhost_rx;
>  	eth_dev->tx_pkt_burst = eth_vhost_tx;
> 
> -	if (rte_vhost_driver_register(iface_name, flags))
> -		goto error;
> -
> -	if (disable_flags) {
> -		if (rte_vhost_driver_disable_features(iface_name,
> -					disable_flags))
> -			goto error;
> -	}
> -
> -	if (rte_vhost_driver_callback_register(iface_name, &vhost_ops) < 0) {
> -		VHOST_LOG(ERR, "Can't register callbacks\n");
> -		goto error;
> -	}
> -
> -	if (rte_vhost_driver_start(iface_name) < 0) {
> -		VHOST_LOG(ERR, "Failed to start driver for %s\n",
> -			iface_name);
> -		goto error;
> -	}
> -
>  	rte_eth_dev_probing_finish(eth_dev);
>  	return 0;
> 
> @@ -1311,9 +1339,7 @@ eth_dev_vhost_create(struct rte_vdev_device *dev,
> char *iface_name,
>  		rte_free(internal->iface_name);
>  		free(internal->dev_name);
>  	}
> -	rte_free(vring_state);
>  	rte_eth_dev_release_port(eth_dev);
> -	rte_free(list);
> 
>  	return -1;
>  }
> --
> 2.17.0

Re: [dpdk-dev] [PATCH v3 2/4] net/vhost: delay vhost driver setup

[Maxime Coquelin]

Thanks Yinan for reporting the issue.

I'm working on a fix, which should be ready today:
1. Don't do vhost setup if device is already in the list
2. Fix the error path in setup function.

Maxime
On 2/18/20 9:50 AM, Wang, Yinan wrote:
> Hi Itsuro/Maxime,
> 
> Below patch (commit id:3d01b759d2679c216725689eabe44147d1737326)blocked vhost/virtio basic test: If re-config vhost port，testpmd will launch failed.
> 
> Reproduce steps:
> 
> ./x86_64-native-linuxapp-gcc/app/testpmd -l 2-4 -n 4 --socket-mem 1024,1024  --legacy-mem --file-prefix=vhost --vdev 'net_vhost0,iface=vhost-net,queues=1,client=0' --no-pci -- -i --txd=1024 --rxd=1024
>     testpmd>set fwd csum
>     testpmd>stop
>     testpmd>port stop 0
>     testpmd>csum set tcp sw 0
>     testpmd>port start 0
> 
> VHOST_CONFIG: vhost-user server: socket created, fd: 26
> VHOST_CONFIG: failed to bind to vhost-net: Address already in use; remove it and try again
> Failed to start driver for vhost-net
> Port0 dev_configure = -1
> Fail to configure port 0
> 
> BR,
> Yinan
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Itsuro Oda
>> Sent: 2020年2月6日 9:40
>> To: dev@dpdk.org; maxime.coquelin@redhat.com; Bie, Tiwei
>> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>; Burakov,
>> Anatoly <anatoly.burakov@intel.com>
>> Cc: stable@dpdk.org
>> Subject: [dpdk-dev] [PATCH v3 2/4] net/vhost: delay vhost driver setup
>>
>> Vhost driver setup is delayed at eth_dev configuration in order to be able to set
>> it from a secondary process.
>>
>> Fixes: 4852aa8f6e21 (drivers/net: enable hotplug on secondary process)
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Itsuro Oda <oda@valinux.co.jp>
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  drivers/net/vhost/rte_eth_vhost.c | 130 ++++++++++++++++++------------
>>  1 file changed, 78 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/net/vhost/rte_eth_vhost.c
>> b/drivers/net/vhost/rte_eth_vhost.c
>> index cea2ead2d..d7bba5c6e 100644
>> --- a/drivers/net/vhost/rte_eth_vhost.c
>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>> @@ -97,6 +97,8 @@ struct pmd_internal {
>>  	rte_atomic32_t dev_attached;
>>  	char *dev_name;
>>  	char *iface_name;
>> +	uint64_t flags;
>> +	uint64_t disable_flags;
>>  	uint16_t max_queues;
>>  	int vid;
>>  	rte_atomic32_t started;
>> @@ -491,17 +493,6 @@ eth_vhost_tx(void *q, struct rte_mbuf **bufs,
>> uint16_t nb_bufs)
>>  	return nb_tx;
>>  }
>>
>> -static int
>> -eth_dev_configure(struct rte_eth_dev *dev __rte_unused) -{
>> -	struct pmd_internal *internal = dev->data->dev_private;
>> -	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
>> -
>> -	internal->vlan_strip = !!(rxmode->offloads &
>> DEV_RX_OFFLOAD_VLAN_STRIP);
>> -
>> -	return 0;
>> -}
>> -
>>  static inline struct internal_list *
>>  find_internal_resource(char *ifname)
>>  {
>> @@ -877,6 +868,62 @@ static struct vhost_device_ops vhost_ops = {
>>  	.vring_state_changed = vring_state_changed,  };
>>
>> +static int
>> +vhost_driver_setup(struct rte_eth_dev *eth_dev) {
>> +	struct pmd_internal *internal = eth_dev->data->dev_private;
>> +	struct internal_list *list = NULL;
>> +	struct rte_vhost_vring_state *vring_state = NULL;
>> +	unsigned int numa_node = eth_dev->device->numa_node;
>> +	const char *name = eth_dev->device->name;
>> +
>> +	list = rte_zmalloc_socket(name, sizeof(*list), 0, numa_node);
>> +	if (list == NULL)
>> +		goto error;
>> +
>> +	vring_state = rte_zmalloc_socket(name, sizeof(*vring_state),
>> +					 0, numa_node);
>> +	if (vring_state == NULL)
>> +		goto error;
>> +
>> +	list->eth_dev = eth_dev;
>> +	pthread_mutex_lock(&internal_list_lock);
>> +	TAILQ_INSERT_TAIL(&internal_list, list, next);
>> +	pthread_mutex_unlock(&internal_list_lock);
>> +
>> +	rte_spinlock_init(&vring_state->lock);
>> +	vring_states[eth_dev->data->port_id] = vring_state;
>> +
>> +	if (rte_vhost_driver_register(internal->iface_name, internal->flags))
>> +		goto error;
>> +
>> +	if (internal->disable_flags) {
>> +		if (rte_vhost_driver_disable_features(internal->iface_name,
>> +						      internal->disable_flags))
>> +			goto error;
>> +	}
>> +
>> +	if (rte_vhost_driver_callback_register(internal->iface_name,
>> +					       &vhost_ops) < 0) {
>> +		VHOST_LOG(ERR, "Can't register callbacks\n");
>> +		goto error;
>> +	}
>> +
>> +	if (rte_vhost_driver_start(internal->iface_name) < 0) {
>> +		VHOST_LOG(ERR, "Failed to start driver for %s\n",
>> +			  internal->iface_name);
>> +		goto error;
>> +	}
>> +
>> +	return 0;
>> +
>> +error:
>> +	rte_free(vring_state);
>> +	rte_free(list);
>> +
>> +	return -1;
>> +}
>> +
>>  int
>>  rte_eth_vhost_get_queue_event(uint16_t port_id,
>>  		struct rte_eth_vhost_queue_event *event) @@ -943,6 +990,24 @@
>> rte_eth_vhost_get_vid_from_port_id(uint16_t port_id)
>>  	return vid;
>>  }
>>
>> +static int
>> +eth_dev_configure(struct rte_eth_dev *dev) {
>> +	struct pmd_internal *internal = dev->data->dev_private;
>> +	const struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
>> +
>> +	/* NOTE: the same process has to operate a vhost interface
>> +	 * from beginning to end (from eth_dev configure to eth_dev close).
>> +	 * It is user's responsibility at the moment.
>> +	 */
>> +	if (vhost_driver_setup(dev) < 0)
>> +		return -1;
>> +
>> +	internal->vlan_strip = !!(rxmode->offloads &
>> +DEV_RX_OFFLOAD_VLAN_STRIP);
>> +
>> +	return 0;
>> +}
>> +
>>  static int
>>  eth_dev_start(struct rte_eth_dev *eth_dev)  { @@ -1219,16 +1284,10 @@
>> eth_dev_vhost_create(struct rte_vdev_device *dev, char *iface_name,
>>  	struct pmd_internal *internal = NULL;
>>  	struct rte_eth_dev *eth_dev = NULL;
>>  	struct rte_ether_addr *eth_addr = NULL;
>> -	struct rte_vhost_vring_state *vring_state = NULL;
>> -	struct internal_list *list = NULL;
>>
>>  	VHOST_LOG(INFO, "Creating VHOST-USER backend on numa
>> socket %u\n",
>>  		numa_node);
>>
>> -	list = rte_zmalloc_socket(name, sizeof(*list), 0, numa_node);
>> -	if (list == NULL)
>> -		goto error;
>> -
>>  	/* reserve an ethdev entry */
>>  	eth_dev = rte_eth_vdev_allocate(dev, sizeof(*internal));
>>  	if (eth_dev == NULL)
>> @@ -1242,11 +1301,6 @@ eth_dev_vhost_create(struct rte_vdev_device *dev,
>> char *iface_name,
>>  	*eth_addr = base_eth_addr;
>>  	eth_addr->addr_bytes[5] = eth_dev->data->port_id;
>>
>> -	vring_state = rte_zmalloc_socket(name,
>> -			sizeof(*vring_state), 0, numa_node);
>> -	if (vring_state == NULL)
>> -		goto error;
>> -
>>  	/* now put it all together
>>  	 * - store queue data in internal,
>>  	 * - point eth_dev_data to internals
>> @@ -1262,18 +1316,12 @@ eth_dev_vhost_create(struct rte_vdev_device
>> *dev, char *iface_name,
>>  		goto error;
>>  	strcpy(internal->iface_name, iface_name);
>>
>> -	list->eth_dev = eth_dev;
>> -	pthread_mutex_lock(&internal_list_lock);
>> -	TAILQ_INSERT_TAIL(&internal_list, list, next);
>> -	pthread_mutex_unlock(&internal_list_lock);
>> -
>> -	rte_spinlock_init(&vring_state->lock);
>> -	vring_states[eth_dev->data->port_id] = vring_state;
>> -
>>  	data->nb_rx_queues = queues;
>>  	data->nb_tx_queues = queues;
>>  	internal->max_queues = queues;
>>  	internal->vid = -1;
>> +	internal->flags = flags;
>> +	internal->disable_flags = disable_flags;
>>  	data->dev_link = pmd_link;
>>  	data->dev_flags = RTE_ETH_DEV_INTR_LSC |
>> RTE_ETH_DEV_CLOSE_REMOVE;
>>
>> @@ -1283,26 +1331,6 @@ eth_dev_vhost_create(struct rte_vdev_device *dev,
>> char *iface_name,
>>  	eth_dev->rx_pkt_burst = eth_vhost_rx;
>>  	eth_dev->tx_pkt_burst = eth_vhost_tx;
>>
>> -	if (rte_vhost_driver_register(iface_name, flags))
>> -		goto error;
>> -
>> -	if (disable_flags) {
>> -		if (rte_vhost_driver_disable_features(iface_name,
>> -					disable_flags))
>> -			goto error;
>> -	}
>> -
>> -	if (rte_vhost_driver_callback_register(iface_name, &vhost_ops) < 0) {
>> -		VHOST_LOG(ERR, "Can't register callbacks\n");
>> -		goto error;
>> -	}
>> -
>> -	if (rte_vhost_driver_start(iface_name) < 0) {
>> -		VHOST_LOG(ERR, "Failed to start driver for %s\n",
>> -			iface_name);
>> -		goto error;
>> -	}
>> -
>>  	rte_eth_dev_probing_finish(eth_dev);
>>  	return 0;
>>
>> @@ -1311,9 +1339,7 @@ eth_dev_vhost_create(struct rte_vdev_device *dev,
>> char *iface_name,
>>  		rte_free(internal->iface_name);
>>  		free(internal->dev_name);
>>  	}
>> -	rte_free(vring_state);
>>  	rte_eth_dev_release_port(eth_dev);
>> -	rte_free(list);
>>
>>  	return -1;
>>  }
>> --
>> 2.17.0
>