[PATCH 0/5] app/testpmd: improve attach/detach support

[PATCH 0/5] app/testpmd: improve attach/detach support

[Thomas Monjalon]

While working on EAL probe/remove and ethdev iterator/close,
some scenarios appeared to not be managed by testpmd, especially
because it was not designed for multi-ports devices:
  - configure dependent port (detected via event)
  - configuring twice (if already probed before)
  - detaching twice

Thomas Monjalon (5):
  app/testpmd: check not detaching device twice
  app/testpmd: merge ports list update functions
  app/testpmd: check not configuring port twice
  app/testpmd: move ethdev events registration
  app/testpmd: setup attached ports on probe event

 app/test-pmd/cmdline.c                      |  59 +++++-
 app/test-pmd/testpmd.c                      | 213 +++++++++++---------
 app/test-pmd/testpmd.h                      |   6 +-
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |   9 +
 4 files changed, 191 insertions(+), 96 deletions(-)

-- 
2.19.0

[PATCH 1/5] app/testpmd: check not detaching device twice

[Thomas Monjalon]

The command "port detach" is removing the EAL rte_device
of the ethdev port specified as parameter.
The function name and some comments are updated to make clear
that we are detaching the whole device.

After detaching, the pointer, which maps a port to its device,
is reset. This way, it is possible to check whether a port
is still associated to a (not removed) device.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test-pmd/cmdline.c |  2 +-
 app/test-pmd/testpmd.c | 35 ++++++++++++++++++++++++++++-------
 app/test-pmd/testpmd.h |  2 +-
 3 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 3b469ac64..f041a01aa 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1305,7 +1305,7 @@ static void cmd_operate_detach_port_parsed(void *parsed_result,
 	struct cmd_operate_detach_port_result *res = parsed_result;
 
 	if (!strcmp(res->keyword, "detach"))
-		detach_port(res->port_id);
+		detach_port_device(res->port_id);
 	else
 		printf("Unknown parameter\n");
 }
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 14647fa19..56e408cdf 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2351,10 +2351,19 @@ setup_attached_port(portid_t pi)
 }
 
 void
-detach_port(portid_t port_id)
+detach_port_device(portid_t port_id)
 {
+	struct rte_device *dev;
+	portid_t sibling;
+
 	printf("Removing a device...\n");
 
+	dev = rte_eth_devices[port_id].device;
+	if (dev == NULL) {
+		printf("Device already removed\n");
+		return;
+	}
+
 	if (ports[port_id].port_status != RTE_PORT_CLOSED) {
 		if (ports[port_id].port_status != RTE_PORT_STOPPED) {
 			printf("Port not stopped\n");
@@ -2365,15 +2374,27 @@ detach_port(portid_t port_id)
 			port_flow_flush(port_id);
 	}
 
-	if (rte_dev_remove(rte_eth_devices[port_id].device) != 0) {
-		TESTPMD_LOG(ERR, "Failed to detach port %u\n", port_id);
+	if (rte_dev_remove(dev) != 0) {
+		TESTPMD_LOG(ERR, "Failed to detach device %s\n", dev->name);
 		return;
 	}
 
+	for (sibling = 0; sibling < RTE_MAX_ETHPORTS; sibling++) {
+		if (rte_eth_devices[sibling].device != dev)
+			continue;
+		/* reset mapping between old ports and removed device */
+		rte_eth_devices[sibling].device = NULL;
+		if (ports[sibling].port_status != RTE_PORT_CLOSED) {
+			/* sibling ports are forced to be closed */
+			ports[sibling].port_status = RTE_PORT_CLOSED;
+			printf("Port %u is closed\n", sibling);
+		}
+	}
+
 	remove_unused_fwd_ports();
 
-	printf("Port %u is detached. Now total ports is %d\n",
-			port_id, nb_ports);
+	printf("Device of port %u is detached\n", port_id);
+	printf("Now total ports is %d\n", nb_ports);
 	printf("Done\n");
 	return;
 }
@@ -2406,7 +2427,7 @@ pmd_test_exit(void)
 			 */
 			device = rte_eth_devices[pt_id].device;
 			if (device && !strcmp(device->driver->name, "net_virtio_user"))
-				detach_port(pt_id);
+				detach_port_device(pt_id);
 		}
 	}
 
@@ -2518,7 +2539,7 @@ rmv_event_callback(void *arg)
 	stop_port(port_id);
 	no_link_check = org_no_link_check;
 	close_port(port_id);
-	detach_port(port_id);
+	detach_port_device(port_id);
 	if (need_to_start)
 		start_packet_forwarding(0);
 }
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 3da728cad..646e44940 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -712,7 +712,7 @@ void stop_port(portid_t pid);
 void close_port(portid_t pid);
 void reset_port(portid_t pid);
 void attach_port(char *identifier);
-void detach_port(portid_t port_id);
+void detach_port_device(portid_t port_id);
 int all_ports_stopped(void);
 int port_is_stopped(portid_t port_id);
 int port_is_started(portid_t port_id);
-- 
2.19.0

Re: [PATCH 1/5] app/testpmd: check not detaching device twice

[Iremonger, Bernard]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, October 24, 2018 2:41 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; ophirmu@mellanox.com; wisamm@mellanox.com; Yigit,
> Ferruh <ferruh.yigit@intel.com>; arybchenko@solarflare.com
> Subject: [PATCH 1/5] app/testpmd: check not detaching device twice
> 
> The command "port detach" is removing the EAL rte_device of the ethdev port
> specified as parameter.
> The function name and some comments are updated to make clear that we are
> detaching the whole device.
> 
> After detaching, the pointer, which maps a port to its device, is reset. This way,
> it is possible to check whether a port is still associated to a (not removed)
> device.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>

[PATCH 2/5] app/testpmd: merge ports list update functions

[Thomas Monjalon]

The arrays ports_ids and fwd_ports_ids require the same kind
of update when some ports are removed or added.

The functions update_fwd_ports() and remove_unused_fwd_ports()
are merged in the new function remove_invalid_ports().
The part for adding new port is moved into setup_attached_port().

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test-pmd/testpmd.c | 74 +++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 52 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 56e408cdf..21627ecb6 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1609,31 +1609,6 @@ launch_packet_forwarding(lcore_function_t *pkt_fwd_on_lcore)
 	}
 }
 
-/*
- * Update the forward ports list.
- */
-void
-update_fwd_ports(portid_t new_pid)
-{
-	unsigned int i;
-	unsigned int new_nb_fwd_ports = 0;
-	int move = 0;
-
-	for (i = 0; i < nb_fwd_ports; ++i) {
-		if (port_id_is_invalid(fwd_ports_ids[i], DISABLED_WARN))
-			move = 1;
-		else if (move)
-			fwd_ports_ids[new_nb_fwd_ports++] = fwd_ports_ids[i];
-		else
-			new_nb_fwd_ports++;
-	}
-	if (new_pid < RTE_MAX_ETHPORTS)
-		fwd_ports_ids[new_nb_fwd_ports++] = new_pid;
-
-	nb_fwd_ports = new_nb_fwd_ports;
-	nb_cfg_ports = new_nb_fwd_ports;
-}
-
 /*
  * Launch packet forwarding configuration.
  */
@@ -2188,28 +2163,25 @@ stop_port(portid_t pid)
 }
 
 static void
-remove_unused_fwd_ports(void)
+remove_invalid_ports_in(portid_t *array, portid_t *total)
 {
-	int i;
-	int last_port_idx = nb_ports - 1;
+	portid_t i;
+	portid_t new_total = 0;
 
-	for (i = 0; i <= last_port_idx; i++) { /* iterate in ports_ids */
-		if (rte_eth_devices[ports_ids[i]].state != RTE_ETH_DEV_UNUSED)
-			continue;
-		/* skip unused ports at the end */
-		while (i <= last_port_idx &&
-				rte_eth_devices[ports_ids[last_port_idx]].state
-				== RTE_ETH_DEV_UNUSED)
-			last_port_idx--;
-		if (last_port_idx < i)
-			break;
-		/* overwrite unused port with last valid port */
-		ports_ids[i] = ports_ids[last_port_idx];
-		/* decrease ports count */
-		last_port_idx--;
-	}
-	nb_ports = rte_eth_dev_count_avail();
-	update_fwd_ports(RTE_MAX_ETHPORTS);
+	for (i = 0; i < *total; i++)
+		if (!port_id_is_invalid(array[i], DISABLED_WARN)) {
+			array[new_total] = array[i];
+			new_total++;
+		}
+	*total = new_total;
+}
+
+static void
+remove_invalid_ports(void)
+{
+	remove_invalid_ports_in(ports_ids, &nb_ports);
+	remove_invalid_ports_in(fwd_ports_ids, &nb_fwd_ports);
+	nb_cfg_ports = nb_fwd_ports;
 }
 
 void
@@ -2254,7 +2226,7 @@ close_port(portid_t pid)
 			port_flow_flush(pi);
 		rte_eth_dev_close(pi);
 
-		remove_unused_fwd_ports();
+		remove_invalid_ports();
 
 		if (rte_atomic16_cmpset(&(port->port_status),
 			RTE_PORT_HANDLING, RTE_PORT_CLOSED) == 0)
@@ -2339,13 +2311,11 @@ setup_attached_port(portid_t pi)
 	reconfig(pi, socket_id);
 	rte_eth_promiscuous_enable(pi);
 
-	ports_ids[nb_ports] = pi;
-	nb_ports = rte_eth_dev_count_avail();
-
+	ports_ids[nb_ports++] = pi;
+	fwd_ports_ids[nb_fwd_ports++] = pi;
+	nb_cfg_ports = nb_fwd_ports;
 	ports[pi].port_status = RTE_PORT_STOPPED;
 
-	update_fwd_ports(pi);
-
 	printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports);
 	printf("Done\n");
 }
@@ -2391,7 +2361,7 @@ detach_port_device(portid_t port_id)
 		}
 	}
 
-	remove_unused_fwd_ports();
+	remove_invalid_ports();
 
 	printf("Device of port %u is detached\n", port_id);
 	printf("Now total ports is %d\n", nb_ports);
-- 
2.19.0

Re: [PATCH 2/5] app/testpmd: merge ports list update functions

[Iremonger, Bernard]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, October 24, 2018 2:41 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; ophirmu@mellanox.com; wisamm@mellanox.com; Yigit,
> Ferruh <ferruh.yigit@intel.com>; arybchenko@solarflare.com
> Subject: [PATCH 2/5] app/testpmd: merge ports list update functions
> 
> The arrays ports_ids and fwd_ports_ids require the same kind of update when
> some ports are removed or added.
> 
> The functions update_fwd_ports() and remove_unused_fwd_ports() are merged
> in the new function remove_invalid_ports().
> The part for adding new port is moved into setup_attached_port().
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>

[PATCH 3/5] app/testpmd: check not configuring port twice

[Thomas Monjalon]

It is possible to request probing of a device twice,
and possibly get new ports for this device.
However, the ports which were already probed and setup
must not be setup again. That's why it is checked whether
the port is already part of fwd_ports_ids array at the beginning
of the function setup_attached_port().

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test-pmd/testpmd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 21627ecb6..72b91adf5 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2295,8 +2295,11 @@ attach_port(char *identifier)
 		return;
 	}
 
-	RTE_ETH_FOREACH_MATCHING_DEV(pi, identifier, &iterator)
+	RTE_ETH_FOREACH_MATCHING_DEV(pi, identifier, &iterator) {
+		if (port_is_forwarding(pi))
+			continue; /* port was already attached before */
 		setup_attached_port(pi);
+	}
 }
 
 static void
-- 
2.19.0

Re: [PATCH 3/5] app/testpmd: check not configuring port twice

[Iremonger, Bernard]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, October 24, 2018 2:41 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; ophirmu@mellanox.com; wisamm@mellanox.com; Yigit,
> Ferruh <ferruh.yigit@intel.com>; arybchenko@solarflare.com
> Subject: [PATCH 3/5] app/testpmd: check not configuring port twice
> 
> It is possible to request probing of a device twice, and possibly get new ports for
> this device.

./devtools/check-git-log.sh -1
Wrong beginning of commit message:
        It is possible to request probing of a device twice,

> However, the ports which were already probed and setup must not be setup
> again. That's why it is checked whether the port is already part of fwd_ports_ids
> array at the beginning of the function setup_attached_port().
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Otherwise

Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>

[PATCH 4/5] app/testpmd: move ethdev events registration

[Thomas Monjalon]

The callback for ethdev events was registered on port start,
so it was missing some events.

It is now registered at the beginning of the main function.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test-pmd/testpmd.c | 72 ++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 30 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 72b91adf5..6eb416e7a 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -345,6 +345,21 @@ uint8_t rmv_interrupt = 1; /* enabled by default */
 
 uint8_t hot_plug = 0; /**< hotplug disabled by default. */
 
+/* Pretty printing of ethdev events */
+static const char * const eth_event_desc[] = {
+	[RTE_ETH_EVENT_UNKNOWN] = "unknown",
+	[RTE_ETH_EVENT_INTR_LSC] = "LSC",
+	[RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
+	[RTE_ETH_EVENT_INTR_RESET] = "interrupt reset",
+	[RTE_ETH_EVENT_VF_MBOX] = "VF mbox",
+	[RTE_ETH_EVENT_IPSEC] = "IPsec",
+	[RTE_ETH_EVENT_MACSEC] = "MACsec",
+	[RTE_ETH_EVENT_INTR_RMV] = "device removal",
+	[RTE_ETH_EVENT_NEW] = "device probed",
+	[RTE_ETH_EVENT_DESTROY] = "device released",
+	[RTE_ETH_EVENT_MAX] = NULL,
+};
+
 /*
  * Display or mask ether events
  * Default to all events except VF_MBOX
@@ -1934,7 +1949,6 @@ start_port(portid_t pid)
 	queueid_t qi;
 	struct rte_port *port;
 	struct ether_addr mac_addr;
-	enum rte_eth_event_type event_type;
 
 	if (port_id_is_invalid(pid, ENABLED_WARN))
 		return 0;
@@ -2090,20 +2104,6 @@ start_port(portid_t pid)
 		need_check_link_status = 1;
 	}
 
-	for (event_type = RTE_ETH_EVENT_UNKNOWN;
-	     event_type < RTE_ETH_EVENT_MAX;
-	     event_type++) {
-		diag = rte_eth_dev_callback_register(RTE_ETH_ALL,
-						event_type,
-						eth_event_callback,
-						NULL);
-		if (diag) {
-			printf("Failed to setup even callback for event %d\n",
-				event_type);
-			return -1;
-		}
-	}
-
 	if (need_check_link_status == 1 && !no_link_check)
 		check_all_ports_link_status(RTE_PORT_ALL);
 	else if (need_check_link_status == 0)
@@ -2522,20 +2522,6 @@ static int
 eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 		  void *ret_param)
 {
-	static const char * const event_desc[] = {
-		[RTE_ETH_EVENT_UNKNOWN] = "Unknown",
-		[RTE_ETH_EVENT_INTR_LSC] = "LSC",
-		[RTE_ETH_EVENT_QUEUE_STATE] = "Queue state",
-		[RTE_ETH_EVENT_INTR_RESET] = "Interrupt reset",
-		[RTE_ETH_EVENT_VF_MBOX] = "VF Mbox",
-		[RTE_ETH_EVENT_IPSEC] = "IPsec",
-		[RTE_ETH_EVENT_MACSEC] = "MACsec",
-		[RTE_ETH_EVENT_INTR_RMV] = "device removal",
-		[RTE_ETH_EVENT_NEW] = "device probed",
-		[RTE_ETH_EVENT_DESTROY] = "device released",
-		[RTE_ETH_EVENT_MAX] = NULL,
-	};
-
 	RTE_SET_USED(param);
 	RTE_SET_USED(ret_param);
 
@@ -2545,7 +2531,7 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 		fflush(stderr);
 	} else if (event_print_mask & (UINT32_C(1) << type)) {
 		printf("\nPort %" PRIu16 ": %s event\n", port_id,
-			event_desc[type]);
+			eth_event_desc[type]);
 		fflush(stdout);
 	}
 
@@ -2564,6 +2550,28 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 	return 0;
 }
 
+static int
+register_eth_event_callback(void)
+{
+	int ret;
+	enum rte_eth_event_type event;
+
+	for (event = RTE_ETH_EVENT_UNKNOWN;
+			event < RTE_ETH_EVENT_MAX; event++) {
+		ret = rte_eth_dev_callback_register(RTE_ETH_ALL,
+				event,
+				eth_event_callback,
+				NULL);
+		if (ret != 0) {
+			TESTPMD_LOG(ERR, "Failed to register callback for "
+					"%s event\n", eth_event_desc[event]);
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
 /* This function is used by the interrupt thread */
 static void
 eth_dev_event_callback(const char *device_name, enum rte_dev_event_type type,
@@ -3048,6 +3056,10 @@ main(int argc, char** argv)
 		rte_panic("Cannot register log type");
 	rte_log_set_level(testpmd_logtype, RTE_LOG_DEBUG);
 
+	ret = register_eth_event_callback();
+	if (ret != 0)
+		rte_panic("Cannot register for ethdev events");
+
 #ifdef RTE_LIBRTE_PDUMP
 	/* initialize packet capture framework */
 	rte_pdump_init(NULL);
-- 
2.19.0

Re: [PATCH 4/5] app/testpmd: move ethdev events registration

[Iremonger, Bernard]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, October 24, 2018 2:41 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; ophirmu@mellanox.com; wisamm@mellanox.com; Yigit,
> Ferruh <ferruh.yigit@intel.com>; arybchenko@solarflare.com
> Subject: [PATCH 4/5] app/testpmd: move ethdev events registration
> 
> The callback for ethdev events was registered on port start, so it was missing
> some events.
> 
> It is now registered at the beginning of the main function.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  app/test-pmd/testpmd.c | 72 ++++++++++++++++++++++++------------------
>  1 file changed, 42 insertions(+), 30 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 72b91adf5..6eb416e7a 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -345,6 +345,21 @@ uint8_t rmv_interrupt = 1; /* enabled by default */
> 
>  uint8_t hot_plug = 0; /**< hotplug disabled by default. */
> 
> +/* Pretty printing of ethdev events */
> +static const char * const eth_event_desc[] = {
> +	[RTE_ETH_EVENT_UNKNOWN] = "unknown",
> +	[RTE_ETH_EVENT_INTR_LSC] = "LSC",

How about replacing "LSC" with "interrupt link status change"

> +	[RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
> +	[RTE_ETH_EVENT_INTR_RESET] = "interrupt reset",
> +	[RTE_ETH_EVENT_VF_MBOX] = "VF mbox",
> +	[RTE_ETH_EVENT_IPSEC] = "IPsec",
> +	[RTE_ETH_EVENT_MACSEC] = "MACsec",
> +	[RTE_ETH_EVENT_INTR_RMV] = "device removal",

How about replacing "device removal" with "interrupt device removal"

> +	[RTE_ETH_EVENT_NEW] = "device probed",
> +	[RTE_ETH_EVENT_DESTROY] = "device released",
> +	[RTE_ETH_EVENT_MAX] = NULL,
> +};
> +
>  /*
>   * Display or mask ether events
>   * Default to all events except VF_MBOX @@ -1934,7 +1949,6 @@
> start_port(portid_t pid)
>  	queueid_t qi;
>  	struct rte_port *port;
>  	struct ether_addr mac_addr;
> -	enum rte_eth_event_type event_type;
> 
>  	if (port_id_is_invalid(pid, ENABLED_WARN))
>  		return 0;
> @@ -2090,20 +2104,6 @@ start_port(portid_t pid)
>  		need_check_link_status = 1;
>  	}
> 
> -	for (event_type = RTE_ETH_EVENT_UNKNOWN;
> -	     event_type < RTE_ETH_EVENT_MAX;
> -	     event_type++) {
> -		diag = rte_eth_dev_callback_register(RTE_ETH_ALL,
> -						event_type,
> -						eth_event_callback,
> -						NULL);
> -		if (diag) {
> -			printf("Failed to setup even callback for event %d\n",
> -				event_type);
> -			return -1;
> -		}
> -	}
> -
>  	if (need_check_link_status == 1 && !no_link_check)
>  		check_all_ports_link_status(RTE_PORT_ALL);
>  	else if (need_check_link_status == 0)
> @@ -2522,20 +2522,6 @@ static int
>  eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void
> *param,
>  		  void *ret_param)
>  {
> -	static const char * const event_desc[] = {
> -		[RTE_ETH_EVENT_UNKNOWN] = "Unknown",
> -		[RTE_ETH_EVENT_INTR_LSC] = "LSC",
> -		[RTE_ETH_EVENT_QUEUE_STATE] = "Queue state",
> -		[RTE_ETH_EVENT_INTR_RESET] = "Interrupt reset",
> -		[RTE_ETH_EVENT_VF_MBOX] = "VF Mbox",
> -		[RTE_ETH_EVENT_IPSEC] = "IPsec",
> -		[RTE_ETH_EVENT_MACSEC] = "MACsec",
> -		[RTE_ETH_EVENT_INTR_RMV] = "device removal",
> -		[RTE_ETH_EVENT_NEW] = "device probed",
> -		[RTE_ETH_EVENT_DESTROY] = "device released",
> -		[RTE_ETH_EVENT_MAX] = NULL,
> -	};
> -
>  	RTE_SET_USED(param);
>  	RTE_SET_USED(ret_param);
> 
> @@ -2545,7 +2531,7 @@ eth_event_callback(portid_t port_id, enum
> rte_eth_event_type type, void *param,
>  		fflush(stderr);
>  	} else if (event_print_mask & (UINT32_C(1) << type)) {
>  		printf("\nPort %" PRIu16 ": %s event\n", port_id,
> -			event_desc[type]);
> +			eth_event_desc[type]);
>  		fflush(stdout);
>  	}
> 
> @@ -2564,6 +2550,28 @@ eth_event_callback(portid_t port_id, enum
> rte_eth_event_type type, void *param,
>  	return 0;
>  }
> 
> +static int
> +register_eth_event_callback(void)
> +{
> +	int ret;
> +	enum rte_eth_event_type event;
> +
> +	for (event = RTE_ETH_EVENT_UNKNOWN;
> +			event < RTE_ETH_EVENT_MAX; event++) {
> +		ret = rte_eth_dev_callback_register(RTE_ETH_ALL,
> +				event,
> +				eth_event_callback,
> +				NULL);
> +		if (ret != 0) {
> +			TESTPMD_LOG(ERR, "Failed to register callback for "
> +					"%s event\n", eth_event_desc[event]);
> +			return -1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  /* This function is used by the interrupt thread */  static void
> eth_dev_event_callback(const char *device_name, enum rte_dev_event_type
> type, @@ -3048,6 +3056,10 @@ main(int argc, char** argv)
>  		rte_panic("Cannot register log type");
>  	rte_log_set_level(testpmd_logtype, RTE_LOG_DEBUG);
> 
> +	ret = register_eth_event_callback();
> +	if (ret != 0)
> +		rte_panic("Cannot register for ethdev events");
> +
>  #ifdef RTE_LIBRTE_PDUMP
>  	/* initialize packet capture framework */
>  	rte_pdump_init(NULL);
> --
> 2.19.0

Regards,

Bernard

Re: [PATCH 4/5] app/testpmd: move ethdev events registration

[Thomas Monjalon]

24/10/2018 17:55, Iremonger, Bernard:
> Hi Thomas,
> 
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > +/* Pretty printing of ethdev events */
> > +static const char * const eth_event_desc[] = {
> > +	[RTE_ETH_EVENT_UNKNOWN] = "unknown",
> > +	[RTE_ETH_EVENT_INTR_LSC] = "LSC",
> 
> How about replacing "LSC" with "interrupt link status change"

When it is printed, "event" is appended.
So I think "interrupt" is a bit too much.
OK for "link state change"?

> > +	[RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
> > +	[RTE_ETH_EVENT_INTR_RESET] = "interrupt reset",
> > +	[RTE_ETH_EVENT_VF_MBOX] = "VF mbox",
> > +	[RTE_ETH_EVENT_IPSEC] = "IPsec",
> > +	[RTE_ETH_EVENT_MACSEC] = "MACsec",
> > +	[RTE_ETH_EVENT_INTR_RMV] = "device removal",
> 
> How about replacing "device removal" with "interrupt device removal"

For same reason, I think "device removal" is enough.
It will be printed as "device removal event".

Re: [PATCH 4/5] app/testpmd: move ethdev events registration

[Iremonger, Bernard]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, October 24, 2018 8:55 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; dev@dpdk.org; ophirmu@mellanox.com;
> wisamm@mellanox.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> arybchenko@solarflare.com
> Subject: Re: [PATCH 4/5] app/testpmd: move ethdev events registration
> 
> 24/10/2018 17:55, Iremonger, Bernard:
> > Hi Thomas,
> >
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > +/* Pretty printing of ethdev events */ static const char * const
> > > +eth_event_desc[] = {
> > > +	[RTE_ETH_EVENT_UNKNOWN] = "unknown",
> > > +	[RTE_ETH_EVENT_INTR_LSC] = "LSC",
> >
> > How about replacing "LSC" with "interrupt link status change"
> 
> When it is printed, "event" is appended.
> So I think "interrupt" is a bit too much.
> OK for "link state change"?

Yes,
 
> > > +	[RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
> > > +	[RTE_ETH_EVENT_INTR_RESET] = "interrupt reset",

Should "interrupt" be dropped from "interrupt reset" too for consistency? 

> > > +	[RTE_ETH_EVENT_VF_MBOX] = "VF mbox",
> > > +	[RTE_ETH_EVENT_IPSEC] = "IPsec",
> > > +	[RTE_ETH_EVENT_MACSEC] = "MACsec",
> > > +	[RTE_ETH_EVENT_INTR_RMV] = "device removal",
> >
> > How about replacing "device removal" with "interrupt device removal"
> 
> For same reason, I think "device removal" is enough.
> It will be printed as "device removal event".
> 
> 
Regards,

Bernard.

Re: [PATCH 4/5] app/testpmd: move ethdev events registration

[Thomas Monjalon]

25/10/2018 10:54, Iremonger, Bernard:
> Hi Thomas,
> 
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 24/10/2018 17:55, Iremonger, Bernard:
> > > Hi Thomas,
> > >
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > +/* Pretty printing of ethdev events */ static const char * const
> > > > +eth_event_desc[] = {
> > > > +	[RTE_ETH_EVENT_UNKNOWN] = "unknown",
> > > > +	[RTE_ETH_EVENT_INTR_LSC] = "LSC",
> > >
> > > How about replacing "LSC" with "interrupt link status change"
> > 
> > When it is printed, "event" is appended.
> > So I think "interrupt" is a bit too much.
> > OK for "link state change"?
> 
> Yes,
>  
> > > > +	[RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
> > > > +	[RTE_ETH_EVENT_INTR_RESET] = "interrupt reset",
> 
> Should "interrupt" be dropped from "interrupt reset" too for consistency? 

Yes, you're right.

> > > > +	[RTE_ETH_EVENT_VF_MBOX] = "VF mbox",
> > > > +	[RTE_ETH_EVENT_IPSEC] = "IPsec",
> > > > +	[RTE_ETH_EVENT_MACSEC] = "MACsec",
> > > > +	[RTE_ETH_EVENT_INTR_RMV] = "device removal",
> > >
> > > How about replacing "device removal" with "interrupt device removal"
> > 
> > For same reason, I think "device removal" is enough.
> > It will be printed as "device removal event".

I will send a v2 today.
Thanks for the review.

[PATCH 5/5] app/testpmd: setup attached ports on probe event

[Thomas Monjalon]

After probing is done, each new port must be setup.
The new ports are currently guessed by iterating on ports
matching the devargs string used for probing.

When probing a port, it is possible that one more port probing
get triggered (e.g. PF is automatically probed when probing
a VF representor). Such automatic probing will be caught only on event.

The iterator loop may be replaced by a call from the event callback.
In order to be able to test both modes, a command is added
to choose between iterator and event modes.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test-pmd/cmdline.c                      | 57 +++++++++++++++++++++
 app/test-pmd/testpmd.c                      | 27 ++++++++--
 app/test-pmd/testpmd.h                      |  4 ++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  9 ++++
 4 files changed, 93 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index f041a01aa..148bd6366 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -280,6 +280,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set portlist (x[,y]*)\n"
 			"    Set the list of forwarding ports.\n\n"
 
+			"set port setup on (iterator|event)\n"
+			"    Select how attached port is retrieved for setup.\n\n"
+
 			"set tx loopback (port_id) (on|off)\n"
 			"    Enable or disable tx loopback.\n\n"
 
@@ -1249,6 +1252,59 @@ cmdline_parse_inst_t cmd_operate_specific_port = {
 	},
 };
 
+/* *** enable port setup (after attach) via iterator or event *** */
+struct cmd_set_port_setup_on_result {
+	cmdline_fixed_string_t set;
+	cmdline_fixed_string_t port;
+	cmdline_fixed_string_t setup;
+	cmdline_fixed_string_t on;
+	cmdline_fixed_string_t mode;
+};
+
+static void cmd_set_port_setup_on_parsed(void *parsed_result,
+				__attribute__((unused)) struct cmdline *cl,
+				__attribute__((unused)) void *data)
+{
+	struct cmd_set_port_setup_on_result *res = parsed_result;
+
+	if (strcmp(res->mode, "event") == 0)
+		setup_on_probe_event = true;
+	else if (strcmp(res->mode, "iterator") == 0)
+		setup_on_probe_event = false;
+	else
+		printf("Unknown mode\n");
+}
+
+cmdline_parse_token_string_t cmd_set_port_setup_on_set =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
+			set, "set");
+cmdline_parse_token_string_t cmd_set_port_setup_on_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
+			port, "port");
+cmdline_parse_token_string_t cmd_set_port_setup_on_setup =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
+			setup, "setup");
+cmdline_parse_token_string_t cmd_set_port_setup_on_on =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
+			on, "on");
+cmdline_parse_token_string_t cmd_set_port_setup_on_mode =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
+			mode, "iterator#event");
+
+cmdline_parse_inst_t cmd_set_port_setup_on = {
+	.f = cmd_set_port_setup_on_parsed,
+	.data = NULL,
+	.help_str = "set port setup on iterator|event",
+	.tokens = {
+		(void *)&cmd_set_port_setup_on_set,
+		(void *)&cmd_set_port_setup_on_port,
+		(void *)&cmd_set_port_setup_on_setup,
+		(void *)&cmd_set_port_setup_on_on,
+		(void *)&cmd_set_port_setup_on_mode,
+		NULL,
+	},
+};
+
 /* *** attach a specified port *** */
 struct cmd_operate_attach_port_result {
 	cmdline_fixed_string_t port;
@@ -17796,6 +17852,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_operate_specific_port,
 	(cmdline_parse_inst_t *)&cmd_operate_attach_port,
 	(cmdline_parse_inst_t *)&cmd_operate_detach_port,
+	(cmdline_parse_inst_t *)&cmd_set_port_setup_on,
 	(cmdline_parse_inst_t *)&cmd_config_speed_all,
 	(cmdline_parse_inst_t *)&cmd_config_speed_specific,
 	(cmdline_parse_inst_t *)&cmd_config_loopback_all,
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 6eb416e7a..abecb58bf 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -345,6 +345,9 @@ uint8_t rmv_interrupt = 1; /* enabled by default */
 
 uint8_t hot_plug = 0; /**< hotplug disabled by default. */
 
+/* After attach, port setup is called on event or by iterator */
+bool setup_on_probe_event = true;
+
 /* Pretty printing of ethdev events */
 static const char * const eth_event_desc[] = {
 	[RTE_ETH_EVENT_UNKNOWN] = "unknown",
@@ -2280,7 +2283,7 @@ reset_port(portid_t pid)
 void
 attach_port(char *identifier)
 {
-	portid_t pi = 0;
+	portid_t pi;
 	struct rte_dev_iterator iterator;
 
 	printf("Attaching a new port...\n");
@@ -2295,7 +2298,19 @@ attach_port(char *identifier)
 		return;
 	}
 
+	/* first attach mode: event */
+	if (setup_on_probe_event) {
+		/* new ports are detected on RTE_ETH_EVENT_NEW event */
+		for (pi = 0; pi < RTE_MAX_ETHPORTS; pi++)
+			if (ports[pi].port_status == RTE_PORT_HANDLING &&
+					ports[pi].need_setup != 0)
+				setup_attached_port(pi);
+		return;
+	}
+
+	/* second attach mode: iterator */
 	RTE_ETH_FOREACH_MATCHING_DEV(pi, identifier, &iterator) {
+		/* setup ports matching the devargs used for probing */
 		if (port_is_forwarding(pi))
 			continue; /* port was already attached before */
 		setup_attached_port(pi);
@@ -2317,6 +2332,7 @@ setup_attached_port(portid_t pi)
 	ports_ids[nb_ports++] = pi;
 	fwd_ports_ids[nb_fwd_ports++] = pi;
 	nb_cfg_ports = nb_fwd_ports;
+	ports[pi].need_setup = 0;
 	ports[pi].port_status = RTE_PORT_STOPPED;
 
 	printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports);
@@ -2535,11 +2551,14 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 		fflush(stdout);
 	}
 
-	if (port_id_is_invalid(port_id, DISABLED_WARN))
-		return 0;
-
 	switch (type) {
+	case RTE_ETH_EVENT_NEW:
+		ports[port_id].need_setup = 1;
+		ports[port_id].port_status = RTE_PORT_HANDLING;
+		break;
 	case RTE_ETH_EVENT_INTR_RMV:
+		if (port_id_is_invalid(port_id, DISABLED_WARN))
+			break;
 		if (rte_eal_alarm_set(100000,
 				rmv_event_callback, (void *)(intptr_t)port_id))
 			fprintf(stderr, "Could not set up deferred device removal\n");
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 646e44940..28f13f630 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -5,6 +5,8 @@
 #ifndef _TESTPMD_H_
 #define _TESTPMD_H_
 
+#include <stdbool.h>
+
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
 #include <rte_gro.h>
@@ -179,6 +181,7 @@ struct rte_port {
 	uint8_t                 tx_queue_stats_mapping_enabled;
 	uint8_t                 rx_queue_stats_mapping_enabled;
 	volatile uint16_t        port_status;    /**< port started or not */
+	uint8_t                 need_setup;     /**< port just attached */
 	uint8_t                 need_reconfig;  /**< need reconfiguring port or not */
 	uint8_t                 need_reconfig_queues; /**< need reconfiguring queues or not */
 	uint8_t                 rss_flag;   /**< enable rss or not */
@@ -326,6 +329,7 @@ extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
 extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter */
 extern uint32_t event_print_mask;
 /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
+extern bool setup_on_probe_event; /**< disabled by port setup-on iterator */
 extern uint8_t hot_plug; /**< enable by "--hot-plug" parameter */
 extern int do_mlockall; /**< set by "--mlockall" or "--no-mlockall" parameter */
 
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index d712bb657..8447f6c89 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -603,6 +603,15 @@ For example, to change the port forwarding:
    RX P=1/Q=0 (socket 0) -> TX P=3/Q=0 (socket 0) peer=02:00:00:00:00:03
    RX P=3/Q=0 (socket 0) -> TX P=1/Q=0 (socket 0) peer=02:00:00:00:00:02
 
+set port setup on
+~~~~~~~~~~~~~~~~~
+
+Select how to retrieve new ports created after "port attach" command.
+
+For each new port, a setup is done.
+It will find the probed ports via RTE_ETH_FOREACH_MATCHING_DEV loop
+in iterator mode, or via RTE_ETH_EVENT_NEW in event mode.
+
 set tx loopback
 ~~~~~~~~~~~~~~~
 
-- 
2.19.0

Re: [PATCH 5/5] app/testpmd: setup attached ports on probe event

[Iremonger, Bernard]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, October 24, 2018 2:41 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; ophirmu@mellanox.com; wisamm@mellanox.com; Yigit,
> Ferruh <ferruh.yigit@intel.com>; arybchenko@solarflare.com
> Subject: [PATCH 5/5] app/testpmd: setup attached ports on probe event
> 
> After probing is done, each new port must be setup.
> The new ports are currently guessed by iterating on ports matching the devargs
> string used for probing.
> 
> When probing a port, it is possible that one more port probing get triggered (e.g.
> PF is automatically probed when probing a VF representor). Such automatic
> probing will be caught only on event.
> 
> The iterator loop may be replaced by a call from the event callback.
> In order to be able to test both modes, a command is added to choose between
> iterator and event modes.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  app/test-pmd/cmdline.c                      | 57 +++++++++++++++++++++
>  app/test-pmd/testpmd.c                      | 27 ++++++++--
>  app/test-pmd/testpmd.h                      |  4 ++
>  doc/guides/testpmd_app_ug/testpmd_funcs.rst |  9 ++++
>  4 files changed, 93 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c index
> f041a01aa..148bd6366 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -280,6 +280,9 @@ static void cmd_help_long_parsed(void *parsed_result,
>  			"set portlist (x[,y]*)\n"
>  			"    Set the list of forwarding ports.\n\n"
> 
> +			"set port setup on (iterator|event)\n"
> +			"    Select how attached port is retrieved for setup.\n\n"
> +
>  			"set tx loopback (port_id) (on|off)\n"
>  			"    Enable or disable tx loopback.\n\n"
> 
> @@ -1249,6 +1252,59 @@ cmdline_parse_inst_t cmd_operate_specific_port =
> {
>  	},
>  };
> 
> +/* *** enable port setup (after attach) via iterator or event *** */
> +struct cmd_set_port_setup_on_result {
> +	cmdline_fixed_string_t set;
> +	cmdline_fixed_string_t port;
> +	cmdline_fixed_string_t setup;
> +	cmdline_fixed_string_t on;
> +	cmdline_fixed_string_t mode;
> +};
> +
> +static void cmd_set_port_setup_on_parsed(void *parsed_result,
> +				__attribute__((unused)) struct cmdline *cl,
> +				__attribute__((unused)) void *data) {
> +	struct cmd_set_port_setup_on_result *res = parsed_result;
> +
> +	if (strcmp(res->mode, "event") == 0)
> +		setup_on_probe_event = true;
> +	else if (strcmp(res->mode, "iterator") == 0)
> +		setup_on_probe_event = false;
> +	else
> +		printf("Unknown mode\n");
> +}
> +
> +cmdline_parse_token_string_t cmd_set_port_setup_on_set =
> +	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
> +			set, "set");
> +cmdline_parse_token_string_t cmd_set_port_setup_on_port =
> +	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
> +			port, "port");
> +cmdline_parse_token_string_t cmd_set_port_setup_on_setup =
> +	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
> +			setup, "setup");
> +cmdline_parse_token_string_t cmd_set_port_setup_on_on =
> +	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
> +			on, "on");
> +cmdline_parse_token_string_t cmd_set_port_setup_on_mode =
> +	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
> +			mode, "iterator#event");
> +
> +cmdline_parse_inst_t cmd_set_port_setup_on = {
> +	.f = cmd_set_port_setup_on_parsed,
> +	.data = NULL,
> +	.help_str = "set port setup on iterator|event",
> +	.tokens = {
> +		(void *)&cmd_set_port_setup_on_set,
> +		(void *)&cmd_set_port_setup_on_port,
> +		(void *)&cmd_set_port_setup_on_setup,
> +		(void *)&cmd_set_port_setup_on_on,
> +		(void *)&cmd_set_port_setup_on_mode,
> +		NULL,
> +	},
> +};
> +
>  /* *** attach a specified port *** */
>  struct cmd_operate_attach_port_result {
>  	cmdline_fixed_string_t port;
> @@ -17796,6 +17852,7 @@ cmdline_parse_ctx_t main_ctx[] = {
>  	(cmdline_parse_inst_t *)&cmd_operate_specific_port,
>  	(cmdline_parse_inst_t *)&cmd_operate_attach_port,
>  	(cmdline_parse_inst_t *)&cmd_operate_detach_port,
> +	(cmdline_parse_inst_t *)&cmd_set_port_setup_on,
>  	(cmdline_parse_inst_t *)&cmd_config_speed_all,
>  	(cmdline_parse_inst_t *)&cmd_config_speed_specific,
>  	(cmdline_parse_inst_t *)&cmd_config_loopback_all, diff --git
> a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 6eb416e7a..abecb58bf 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -345,6 +345,9 @@ uint8_t rmv_interrupt = 1; /* enabled by default */
> 
>  uint8_t hot_plug = 0; /**< hotplug disabled by default. */
> 
> +/* After attach, port setup is called on event or by iterator */ bool
> +setup_on_probe_event = true;
> +
>  /* Pretty printing of ethdev events */
>  static const char * const eth_event_desc[] = {
>  	[RTE_ETH_EVENT_UNKNOWN] = "unknown",
> @@ -2280,7 +2283,7 @@ reset_port(portid_t pid)  void  attach_port(char
> *identifier)  {
> -	portid_t pi = 0;
> +	portid_t pi;
>  	struct rte_dev_iterator iterator;
> 
>  	printf("Attaching a new port...\n");
> @@ -2295,7 +2298,19 @@ attach_port(char *identifier)
>  		return;
>  	}
> 
> +	/* first attach mode: event */
> +	if (setup_on_probe_event) {
> +		/* new ports are detected on RTE_ETH_EVENT_NEW event */
> +		for (pi = 0; pi < RTE_MAX_ETHPORTS; pi++)
> +			if (ports[pi].port_status == RTE_PORT_HANDLING &&
> +					ports[pi].need_setup != 0)
> +				setup_attached_port(pi);
> +		return;
> +	}
> +
> +	/* second attach mode: iterator */
>  	RTE_ETH_FOREACH_MATCHING_DEV(pi, identifier, &iterator) {
> +		/* setup ports matching the devargs used for probing */
>  		if (port_is_forwarding(pi))
>  			continue; /* port was already attached before */
>  		setup_attached_port(pi);
> @@ -2317,6 +2332,7 @@ setup_attached_port(portid_t pi)
>  	ports_ids[nb_ports++] = pi;
>  	fwd_ports_ids[nb_fwd_ports++] = pi;
>  	nb_cfg_ports = nb_fwd_ports;
> +	ports[pi].need_setup = 0;
>  	ports[pi].port_status = RTE_PORT_STOPPED;
> 
>  	printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports); @@
> -2535,11 +2551,14 @@ eth_event_callback(portid_t port_id, enum
> rte_eth_event_type type, void *param,
>  		fflush(stdout);
>  	}
> 
> -	if (port_id_is_invalid(port_id, DISABLED_WARN))
> -		return 0;
> -
>  	switch (type) {
> +	case RTE_ETH_EVENT_NEW:
> +		ports[port_id].need_setup = 1;
> +		ports[port_id].port_status = RTE_PORT_HANDLING;
> +		break;
>  	case RTE_ETH_EVENT_INTR_RMV:
> +		if (port_id_is_invalid(port_id, DISABLED_WARN))
> +			break;
>  		if (rte_eal_alarm_set(100000,
>  				rmv_event_callback, (void *)(intptr_t)port_id))
>  			fprintf(stderr, "Could not set up deferred device
> removal\n"); diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 646e44940..28f13f630 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -5,6 +5,8 @@
>  #ifndef _TESTPMD_H_
>  #define _TESTPMD_H_
> 
> +#include <stdbool.h>
> +
>  #include <rte_pci.h>
>  #include <rte_bus_pci.h>
>  #include <rte_gro.h>
> @@ -179,6 +181,7 @@ struct rte_port {
>  	uint8_t                 tx_queue_stats_mapping_enabled;
>  	uint8_t                 rx_queue_stats_mapping_enabled;
>  	volatile uint16_t        port_status;    /**< port started or not */
> +	uint8_t                 need_setup;     /**< port just attached */
>  	uint8_t                 need_reconfig;  /**< need reconfiguring port or not */
>  	uint8_t                 need_reconfig_queues; /**< need reconfiguring queues
> or not */
>  	uint8_t                 rss_flag;   /**< enable rss or not */
> @@ -326,6 +329,7 @@ extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-
> interrupt" parameter */  extern uint8_t rmv_interrupt; /**< disabled by "--no-
> rmv-interrupt" parameter */  extern uint32_t event_print_mask;  /**< set by "--
> print-event xxxx" and "--mask-event xxxx parameters */
> +extern bool setup_on_probe_event; /**< disabled by port setup-on
> +iterator */
>  extern uint8_t hot_plug; /**< enable by "--hot-plug" parameter */  extern int
> do_mlockall; /**< set by "--mlockall" or "--no-mlockall" parameter */
> 
> diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> index d712bb657..8447f6c89 100644
> --- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> +++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
> @@ -603,6 +603,15 @@ For example, to change the port forwarding:
>     RX P=1/Q=0 (socket 0) -> TX P=3/Q=0 (socket 0) peer=02:00:00:00:00:03
>     RX P=3/Q=0 (socket 0) -> TX P=1/Q=0 (socket 0) peer=02:00:00:00:00:02
> 
> +set port setup on
> +~~~~~~~~~~~~~~~~~
> +
> +Select how to retrieve new ports created after "port attach" command.
> +
> +For each new port, a setup is done.
> +It will find the probed ports via RTE_ETH_FOREACH_MATCHING_DEV loop in
> +iterator mode, or via RTE_ETH_EVENT_NEW in event mode.
> +

It would be useful to add an example command here (similar to other commands).

>  set tx loopback
>  ~~~~~~~~~~~~~~~
> 
> --
> 2.19.0

Regards,

Bernard.

Re: [PATCH 5/5] app/testpmd: setup attached ports on probe event

[Thomas Monjalon]

24/10/2018 18:13, Iremonger, Bernard:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > +set port setup on
> > +~~~~~~~~~~~~~~~~~
> > +
> > +Select how to retrieve new ports created after "port attach" command.
> > +
> > +For each new port, a setup is done.
> > +It will find the probed ports via RTE_ETH_FOREACH_MATCHING_DEV loop in
> > +iterator mode, or via RTE_ETH_EVENT_NEW in event mode.
> > +
> 
> It would be useful to add an example command here (similar to other commands).

Do you mean something like this?

	testpmd> set port setup on (iterator|event)

Re: [PATCH 5/5] app/testpmd: setup attached ports on probe event

[Iremonger, Bernard]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, October 24, 2018 8:58 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; dev@dpdk.org; ophirmu@mellanox.com;
> wisamm@mellanox.com; Yigit, Ferruh <ferruh.yigit@intel.com>;
> arybchenko@solarflare.com
> Subject: Re: [PATCH 5/5] app/testpmd: setup attached ports on probe event
> 
> 24/10/2018 18:13, Iremonger, Bernard:
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > +set port setup on
> > > +~~~~~~~~~~~~~~~~~
> > > +
> > > +Select how to retrieve new ports created after "port attach" command.
> > > +
> > > +For each new port, a setup is done.
> > > +It will find the probed ports via RTE_ETH_FOREACH_MATCHING_DEV loop
> > > +in iterator mode, or via RTE_ETH_EVENT_NEW in event mode.
> > > +
> >
> > It would be useful to add an example command here (similar to other
> commands).
> 
> Do you mean something like this?
> 
> 	testpmd> set port setup on (iterator|event)
> 

Yes.

Regards,

Bernard.

[PATCH v2 0/5] app/testpmd: improve attach/detach support

[Thomas Monjalon]

While working on EAL probe/remove and ethdev iterator/close,
some scenarios appeared to not be managed by testpmd, especially
because it was not designed for multi-ports devices:
  - configure dependent port (detected via event)
  - configuring twice (if already probed before)
  - detaching twice


Changes in v2 - Bernard review:
  - improve pretty names of printed events
  - add syntax of the new command in the guide


Thomas Monjalon (5):
  app/testpmd: check not detaching device twice
  app/testpmd: merge ports list update functions
  app/testpmd: check not configuring port twice
  app/testpmd: move ethdev events registration
  app/testpmd: setup attached ports on probe event

 app/test-pmd/cmdline.c                      |  59 +++++-
 app/test-pmd/testpmd.c                      | 213 +++++++++++---------
 app/test-pmd/testpmd.h                      |   6 +-
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  11 +
 4 files changed, 193 insertions(+), 96 deletions(-)

-- 
2.19.0

[PATCH v2 1/5] app/testpmd: check not detaching device twice

[Thomas Monjalon]

The command "port detach" is removing the EAL rte_device
of the ethdev port specified as parameter.
The function name and some comments are updated to make clear
that we are detaching the whole device.

After detaching, the pointer, which maps a port to its device,
is reset. This way, it is possible to check whether a port
is still associated to a (not removed) device.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 app/test-pmd/cmdline.c |  2 +-
 app/test-pmd/testpmd.c | 35 ++++++++++++++++++++++++++++-------
 app/test-pmd/testpmd.h |  2 +-
 3 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 4856d2365..e350c38a9 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1305,7 +1305,7 @@ static void cmd_operate_detach_port_parsed(void *parsed_result,
 	struct cmd_operate_detach_port_result *res = parsed_result;
 
 	if (!strcmp(res->keyword, "detach"))
-		detach_port(res->port_id);
+		detach_port_device(res->port_id);
 	else
 		printf("Unknown parameter\n");
 }
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 70c08223d..70d08d7d5 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2356,10 +2356,19 @@ setup_attached_port(portid_t pi)
 }
 
 void
-detach_port(portid_t port_id)
+detach_port_device(portid_t port_id)
 {
+	struct rte_device *dev;
+	portid_t sibling;
+
 	printf("Removing a device...\n");
 
+	dev = rte_eth_devices[port_id].device;
+	if (dev == NULL) {
+		printf("Device already removed\n");
+		return;
+	}
+
 	if (ports[port_id].port_status != RTE_PORT_CLOSED) {
 		if (ports[port_id].port_status != RTE_PORT_STOPPED) {
 			printf("Port not stopped\n");
@@ -2370,15 +2379,27 @@ detach_port(portid_t port_id)
 			port_flow_flush(port_id);
 	}
 
-	if (rte_dev_remove(rte_eth_devices[port_id].device) != 0) {
-		TESTPMD_LOG(ERR, "Failed to detach port %u\n", port_id);
+	if (rte_dev_remove(dev) != 0) {
+		TESTPMD_LOG(ERR, "Failed to detach device %s\n", dev->name);
 		return;
 	}
 
+	for (sibling = 0; sibling < RTE_MAX_ETHPORTS; sibling++) {
+		if (rte_eth_devices[sibling].device != dev)
+			continue;
+		/* reset mapping between old ports and removed device */
+		rte_eth_devices[sibling].device = NULL;
+		if (ports[sibling].port_status != RTE_PORT_CLOSED) {
+			/* sibling ports are forced to be closed */
+			ports[sibling].port_status = RTE_PORT_CLOSED;
+			printf("Port %u is closed\n", sibling);
+		}
+	}
+
 	remove_unused_fwd_ports();
 
-	printf("Port %u is detached. Now total ports is %d\n",
-			port_id, nb_ports);
+	printf("Device of port %u is detached\n", port_id);
+	printf("Now total ports is %d\n", nb_ports);
 	printf("Done\n");
 	return;
 }
@@ -2411,7 +2432,7 @@ pmd_test_exit(void)
 			 */
 			device = rte_eth_devices[pt_id].device;
 			if (device && !strcmp(device->driver->name, "net_virtio_user"))
-				detach_port(pt_id);
+				detach_port_device(pt_id);
 		}
 	}
 
@@ -2523,7 +2544,7 @@ rmv_event_callback(void *arg)
 	stop_port(port_id);
 	no_link_check = org_no_link_check;
 	close_port(port_id);
-	detach_port(port_id);
+	detach_port_device(port_id);
 	if (need_to_start)
 		start_packet_forwarding(0);
 }
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index ca2320c46..e0f86ee84 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -777,7 +777,7 @@ void stop_port(portid_t pid);
 void close_port(portid_t pid);
 void reset_port(portid_t pid);
 void attach_port(char *identifier);
-void detach_port(portid_t port_id);
+void detach_port_device(portid_t port_id);
 int all_ports_stopped(void);
 int port_is_stopped(portid_t port_id);
 int port_is_started(portid_t port_id);
-- 
2.19.0

[PATCH v2 2/5] app/testpmd: merge ports list update functions

[Thomas Monjalon]

The arrays ports_ids and fwd_ports_ids require the same kind
of update when some ports are removed or added.

The functions update_fwd_ports() and remove_unused_fwd_ports()
are merged in the new function remove_invalid_ports().
The part for adding new port is moved into setup_attached_port().

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 app/test-pmd/testpmd.c | 74 +++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 52 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 70d08d7d5..dd6e6eacd 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1614,31 +1614,6 @@ launch_packet_forwarding(lcore_function_t *pkt_fwd_on_lcore)
 	}
 }
 
-/*
- * Update the forward ports list.
- */
-void
-update_fwd_ports(portid_t new_pid)
-{
-	unsigned int i;
-	unsigned int new_nb_fwd_ports = 0;
-	int move = 0;
-
-	for (i = 0; i < nb_fwd_ports; ++i) {
-		if (port_id_is_invalid(fwd_ports_ids[i], DISABLED_WARN))
-			move = 1;
-		else if (move)
-			fwd_ports_ids[new_nb_fwd_ports++] = fwd_ports_ids[i];
-		else
-			new_nb_fwd_ports++;
-	}
-	if (new_pid < RTE_MAX_ETHPORTS)
-		fwd_ports_ids[new_nb_fwd_ports++] = new_pid;
-
-	nb_fwd_ports = new_nb_fwd_ports;
-	nb_cfg_ports = new_nb_fwd_ports;
-}
-
 /*
  * Launch packet forwarding configuration.
  */
@@ -2193,28 +2168,25 @@ stop_port(portid_t pid)
 }
 
 static void
-remove_unused_fwd_ports(void)
+remove_invalid_ports_in(portid_t *array, portid_t *total)
 {
-	int i;
-	int last_port_idx = nb_ports - 1;
+	portid_t i;
+	portid_t new_total = 0;
 
-	for (i = 0; i <= last_port_idx; i++) { /* iterate in ports_ids */
-		if (rte_eth_devices[ports_ids[i]].state != RTE_ETH_DEV_UNUSED)
-			continue;
-		/* skip unused ports at the end */
-		while (i <= last_port_idx &&
-				rte_eth_devices[ports_ids[last_port_idx]].state
-				== RTE_ETH_DEV_UNUSED)
-			last_port_idx--;
-		if (last_port_idx < i)
-			break;
-		/* overwrite unused port with last valid port */
-		ports_ids[i] = ports_ids[last_port_idx];
-		/* decrease ports count */
-		last_port_idx--;
-	}
-	nb_ports = rte_eth_dev_count_avail();
-	update_fwd_ports(RTE_MAX_ETHPORTS);
+	for (i = 0; i < *total; i++)
+		if (!port_id_is_invalid(array[i], DISABLED_WARN)) {
+			array[new_total] = array[i];
+			new_total++;
+		}
+	*total = new_total;
+}
+
+static void
+remove_invalid_ports(void)
+{
+	remove_invalid_ports_in(ports_ids, &nb_ports);
+	remove_invalid_ports_in(fwd_ports_ids, &nb_fwd_ports);
+	nb_cfg_ports = nb_fwd_ports;
 }
 
 void
@@ -2259,7 +2231,7 @@ close_port(portid_t pid)
 			port_flow_flush(pi);
 		rte_eth_dev_close(pi);
 
-		remove_unused_fwd_ports();
+		remove_invalid_ports();
 
 		if (rte_atomic16_cmpset(&(port->port_status),
 			RTE_PORT_HANDLING, RTE_PORT_CLOSED) == 0)
@@ -2344,13 +2316,11 @@ setup_attached_port(portid_t pi)
 	reconfig(pi, socket_id);
 	rte_eth_promiscuous_enable(pi);
 
-	ports_ids[nb_ports] = pi;
-	nb_ports = rte_eth_dev_count_avail();
-
+	ports_ids[nb_ports++] = pi;
+	fwd_ports_ids[nb_fwd_ports++] = pi;
+	nb_cfg_ports = nb_fwd_ports;
 	ports[pi].port_status = RTE_PORT_STOPPED;
 
-	update_fwd_ports(pi);
-
 	printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports);
 	printf("Done\n");
 }
@@ -2396,7 +2366,7 @@ detach_port_device(portid_t port_id)
 		}
 	}
 
-	remove_unused_fwd_ports();
+	remove_invalid_ports();
 
 	printf("Device of port %u is detached\n", port_id);
 	printf("Now total ports is %d\n", nb_ports);
-- 
2.19.0

[PATCH v2 3/5] app/testpmd: check not configuring port twice

[Thomas Monjalon]

It is possible to request probing of a device twice,
and possibly get new ports for this device.
However, the ports which were already probed and setup
must not be setup again. That's why it is checked whether
the port is already part of fwd_ports_ids array at the beginning
of the function setup_attached_port().

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 app/test-pmd/testpmd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index dd6e6eacd..5706686fc 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2300,8 +2300,11 @@ attach_port(char *identifier)
 		return;
 	}
 
-	RTE_ETH_FOREACH_MATCHING_DEV(pi, identifier, &iterator)
+	RTE_ETH_FOREACH_MATCHING_DEV(pi, identifier, &iterator) {
+		if (port_is_forwarding(pi))
+			continue; /* port was already attached before */
 		setup_attached_port(pi);
+	}
 }
 
 static void
-- 
2.19.0

[PATCH v2 4/5] app/testpmd: move ethdev events registration

[Thomas Monjalon]

The callback for ethdev events was registered on port start,
so it was missing some events.

It is now registered at the beginning of the main function.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test-pmd/testpmd.c | 72 ++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 30 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 5706686fc..d4ab90b45 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -345,6 +345,21 @@ uint8_t rmv_interrupt = 1; /* enabled by default */
 
 uint8_t hot_plug = 0; /**< hotplug disabled by default. */
 
+/* Pretty printing of ethdev events */
+static const char * const eth_event_desc[] = {
+	[RTE_ETH_EVENT_UNKNOWN] = "unknown",
+	[RTE_ETH_EVENT_INTR_LSC] = "link state change",
+	[RTE_ETH_EVENT_QUEUE_STATE] = "queue state",
+	[RTE_ETH_EVENT_INTR_RESET] = "reset",
+	[RTE_ETH_EVENT_VF_MBOX] = "VF mbox",
+	[RTE_ETH_EVENT_IPSEC] = "IPsec",
+	[RTE_ETH_EVENT_MACSEC] = "MACsec",
+	[RTE_ETH_EVENT_INTR_RMV] = "device removal",
+	[RTE_ETH_EVENT_NEW] = "device probed",
+	[RTE_ETH_EVENT_DESTROY] = "device released",
+	[RTE_ETH_EVENT_MAX] = NULL,
+};
+
 /*
  * Display or mask ether events
  * Default to all events except VF_MBOX
@@ -1939,7 +1954,6 @@ start_port(portid_t pid)
 	queueid_t qi;
 	struct rte_port *port;
 	struct ether_addr mac_addr;
-	enum rte_eth_event_type event_type;
 
 	if (port_id_is_invalid(pid, ENABLED_WARN))
 		return 0;
@@ -2095,20 +2109,6 @@ start_port(portid_t pid)
 		need_check_link_status = 1;
 	}
 
-	for (event_type = RTE_ETH_EVENT_UNKNOWN;
-	     event_type < RTE_ETH_EVENT_MAX;
-	     event_type++) {
-		diag = rte_eth_dev_callback_register(RTE_ETH_ALL,
-						event_type,
-						eth_event_callback,
-						NULL);
-		if (diag) {
-			printf("Failed to setup even callback for event %d\n",
-				event_type);
-			return -1;
-		}
-	}
-
 	if (need_check_link_status == 1 && !no_link_check)
 		check_all_ports_link_status(RTE_PORT_ALL);
 	else if (need_check_link_status == 0)
@@ -2527,20 +2527,6 @@ static int
 eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 		  void *ret_param)
 {
-	static const char * const event_desc[] = {
-		[RTE_ETH_EVENT_UNKNOWN] = "Unknown",
-		[RTE_ETH_EVENT_INTR_LSC] = "LSC",
-		[RTE_ETH_EVENT_QUEUE_STATE] = "Queue state",
-		[RTE_ETH_EVENT_INTR_RESET] = "Interrupt reset",
-		[RTE_ETH_EVENT_VF_MBOX] = "VF Mbox",
-		[RTE_ETH_EVENT_IPSEC] = "IPsec",
-		[RTE_ETH_EVENT_MACSEC] = "MACsec",
-		[RTE_ETH_EVENT_INTR_RMV] = "device removal",
-		[RTE_ETH_EVENT_NEW] = "device probed",
-		[RTE_ETH_EVENT_DESTROY] = "device released",
-		[RTE_ETH_EVENT_MAX] = NULL,
-	};
-
 	RTE_SET_USED(param);
 	RTE_SET_USED(ret_param);
 
@@ -2550,7 +2536,7 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 		fflush(stderr);
 	} else if (event_print_mask & (UINT32_C(1) << type)) {
 		printf("\nPort %" PRIu16 ": %s event\n", port_id,
-			event_desc[type]);
+			eth_event_desc[type]);
 		fflush(stdout);
 	}
 
@@ -2569,6 +2555,28 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 	return 0;
 }
 
+static int
+register_eth_event_callback(void)
+{
+	int ret;
+	enum rte_eth_event_type event;
+
+	for (event = RTE_ETH_EVENT_UNKNOWN;
+			event < RTE_ETH_EVENT_MAX; event++) {
+		ret = rte_eth_dev_callback_register(RTE_ETH_ALL,
+				event,
+				eth_event_callback,
+				NULL);
+		if (ret != 0) {
+			TESTPMD_LOG(ERR, "Failed to register callback for "
+					"%s event\n", eth_event_desc[event]);
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
 /* This function is used by the interrupt thread */
 static void
 eth_dev_event_callback(const char *device_name, enum rte_dev_event_type type,
@@ -3053,6 +3061,10 @@ main(int argc, char** argv)
 		rte_panic("Cannot register log type");
 	rte_log_set_level(testpmd_logtype, RTE_LOG_DEBUG);
 
+	ret = register_eth_event_callback();
+	if (ret != 0)
+		rte_panic("Cannot register for ethdev events");
+
 #ifdef RTE_LIBRTE_PDUMP
 	/* initialize packet capture framework */
 	rte_pdump_init(NULL);
-- 
2.19.0

Re: [PATCH v2 4/5] app/testpmd: move ethdev events registration

[Iremonger, Bernard]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, October 25, 2018 4:11 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; ophirmu@mellanox.com; wisamm@mellanox.com; Yigit,
> Ferruh <ferruh.yigit@intel.com>; arybchenko@solarflare.com
> Subject: [PATCH v2 4/5] app/testpmd: move ethdev events registration
> 
> The callback for ethdev events was registered on port start, so it was missing
> some events.
> 
> It is now registered at the beginning of the main function.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>

[PATCH v2 5/5] app/testpmd: setup attached ports on probe event

[Thomas Monjalon]

After probing is done, each new port must be setup.
The new ports are currently guessed by iterating on ports
matching the devargs string used for probing.

When probing a port, it is possible that one more port probing
get triggered (e.g. PF is automatically probed when probing
a VF representor). Such automatic probing will be caught only on event.

The iterator loop may be replaced by a call from the event callback.
In order to be able to test both modes, a command is added
to choose between iterator and event modes.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 app/test-pmd/cmdline.c                      | 57 +++++++++++++++++++++
 app/test-pmd/testpmd.c                      | 27 ++++++++--
 app/test-pmd/testpmd.h                      |  4 ++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst | 11 ++++
 4 files changed, 95 insertions(+), 4 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index e350c38a9..1050fde96 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -280,6 +280,9 @@ static void cmd_help_long_parsed(void *parsed_result,
 			"set portlist (x[,y]*)\n"
 			"    Set the list of forwarding ports.\n\n"
 
+			"set port setup on (iterator|event)\n"
+			"    Select how attached port is retrieved for setup.\n\n"
+
 			"set tx loopback (port_id) (on|off)\n"
 			"    Enable or disable tx loopback.\n\n"
 
@@ -1249,6 +1252,59 @@ cmdline_parse_inst_t cmd_operate_specific_port = {
 	},
 };
 
+/* *** enable port setup (after attach) via iterator or event *** */
+struct cmd_set_port_setup_on_result {
+	cmdline_fixed_string_t set;
+	cmdline_fixed_string_t port;
+	cmdline_fixed_string_t setup;
+	cmdline_fixed_string_t on;
+	cmdline_fixed_string_t mode;
+};
+
+static void cmd_set_port_setup_on_parsed(void *parsed_result,
+				__attribute__((unused)) struct cmdline *cl,
+				__attribute__((unused)) void *data)
+{
+	struct cmd_set_port_setup_on_result *res = parsed_result;
+
+	if (strcmp(res->mode, "event") == 0)
+		setup_on_probe_event = true;
+	else if (strcmp(res->mode, "iterator") == 0)
+		setup_on_probe_event = false;
+	else
+		printf("Unknown mode\n");
+}
+
+cmdline_parse_token_string_t cmd_set_port_setup_on_set =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
+			set, "set");
+cmdline_parse_token_string_t cmd_set_port_setup_on_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
+			port, "port");
+cmdline_parse_token_string_t cmd_set_port_setup_on_setup =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
+			setup, "setup");
+cmdline_parse_token_string_t cmd_set_port_setup_on_on =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
+			on, "on");
+cmdline_parse_token_string_t cmd_set_port_setup_on_mode =
+	TOKEN_STRING_INITIALIZER(struct cmd_set_port_setup_on_result,
+			mode, "iterator#event");
+
+cmdline_parse_inst_t cmd_set_port_setup_on = {
+	.f = cmd_set_port_setup_on_parsed,
+	.data = NULL,
+	.help_str = "set port setup on iterator|event",
+	.tokens = {
+		(void *)&cmd_set_port_setup_on_set,
+		(void *)&cmd_set_port_setup_on_port,
+		(void *)&cmd_set_port_setup_on_setup,
+		(void *)&cmd_set_port_setup_on_on,
+		(void *)&cmd_set_port_setup_on_mode,
+		NULL,
+	},
+};
+
 /* *** attach a specified port *** */
 struct cmd_operate_attach_port_result {
 	cmdline_fixed_string_t port;
@@ -18529,6 +18585,7 @@ cmdline_parse_ctx_t main_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_operate_specific_port,
 	(cmdline_parse_inst_t *)&cmd_operate_attach_port,
 	(cmdline_parse_inst_t *)&cmd_operate_detach_port,
+	(cmdline_parse_inst_t *)&cmd_set_port_setup_on,
 	(cmdline_parse_inst_t *)&cmd_config_speed_all,
 	(cmdline_parse_inst_t *)&cmd_config_speed_specific,
 	(cmdline_parse_inst_t *)&cmd_config_loopback_all,
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index d4ab90b45..9c0edcaed 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -345,6 +345,9 @@ uint8_t rmv_interrupt = 1; /* enabled by default */
 
 uint8_t hot_plug = 0; /**< hotplug disabled by default. */
 
+/* After attach, port setup is called on event or by iterator */
+bool setup_on_probe_event = true;
+
 /* Pretty printing of ethdev events */
 static const char * const eth_event_desc[] = {
 	[RTE_ETH_EVENT_UNKNOWN] = "unknown",
@@ -2285,7 +2288,7 @@ reset_port(portid_t pid)
 void
 attach_port(char *identifier)
 {
-	portid_t pi = 0;
+	portid_t pi;
 	struct rte_dev_iterator iterator;
 
 	printf("Attaching a new port...\n");
@@ -2300,7 +2303,19 @@ attach_port(char *identifier)
 		return;
 	}
 
+	/* first attach mode: event */
+	if (setup_on_probe_event) {
+		/* new ports are detected on RTE_ETH_EVENT_NEW event */
+		for (pi = 0; pi < RTE_MAX_ETHPORTS; pi++)
+			if (ports[pi].port_status == RTE_PORT_HANDLING &&
+					ports[pi].need_setup != 0)
+				setup_attached_port(pi);
+		return;
+	}
+
+	/* second attach mode: iterator */
 	RTE_ETH_FOREACH_MATCHING_DEV(pi, identifier, &iterator) {
+		/* setup ports matching the devargs used for probing */
 		if (port_is_forwarding(pi))
 			continue; /* port was already attached before */
 		setup_attached_port(pi);
@@ -2322,6 +2337,7 @@ setup_attached_port(portid_t pi)
 	ports_ids[nb_ports++] = pi;
 	fwd_ports_ids[nb_fwd_ports++] = pi;
 	nb_cfg_ports = nb_fwd_ports;
+	ports[pi].need_setup = 0;
 	ports[pi].port_status = RTE_PORT_STOPPED;
 
 	printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports);
@@ -2540,11 +2556,14 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 		fflush(stdout);
 	}
 
-	if (port_id_is_invalid(port_id, DISABLED_WARN))
-		return 0;
-
 	switch (type) {
+	case RTE_ETH_EVENT_NEW:
+		ports[port_id].need_setup = 1;
+		ports[port_id].port_status = RTE_PORT_HANDLING;
+		break;
 	case RTE_ETH_EVENT_INTR_RMV:
+		if (port_id_is_invalid(port_id, DISABLED_WARN))
+			break;
 		if (rte_eal_alarm_set(100000,
 				rmv_event_callback, (void *)(intptr_t)port_id))
 			fprintf(stderr, "Could not set up deferred device removal\n");
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index e0f86ee84..3ff11e644 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -5,6 +5,8 @@
 #ifndef _TESTPMD_H_
 #define _TESTPMD_H_
 
+#include <stdbool.h>
+
 #include <rte_pci.h>
 #include <rte_bus_pci.h>
 #include <rte_gro.h>
@@ -179,6 +181,7 @@ struct rte_port {
 	uint8_t                 tx_queue_stats_mapping_enabled;
 	uint8_t                 rx_queue_stats_mapping_enabled;
 	volatile uint16_t        port_status;    /**< port started or not */
+	uint8_t                 need_setup;     /**< port just attached */
 	uint8_t                 need_reconfig;  /**< need reconfiguring port or not */
 	uint8_t                 need_reconfig_queues; /**< need reconfiguring queues or not */
 	uint8_t                 rss_flag;   /**< enable rss or not */
@@ -329,6 +332,7 @@ extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
 extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter */
 extern uint32_t event_print_mask;
 /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
+extern bool setup_on_probe_event; /**< disabled by port setup-on iterator */
 extern uint8_t hot_plug; /**< enable by "--hot-plug" parameter */
 extern int do_mlockall; /**< set by "--mlockall" or "--no-mlockall" parameter */
 
diff --git a/doc/guides/testpmd_app_ug/testpmd_funcs.rst b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
index d5a1a73a7..e23079b6d 100644
--- a/doc/guides/testpmd_app_ug/testpmd_funcs.rst
+++ b/doc/guides/testpmd_app_ug/testpmd_funcs.rst
@@ -609,6 +609,17 @@ For example, to change the port forwarding:
    RX P=1/Q=0 (socket 0) -> TX P=3/Q=0 (socket 0) peer=02:00:00:00:00:03
    RX P=3/Q=0 (socket 0) -> TX P=1/Q=0 (socket 0) peer=02:00:00:00:00:02
 
+set port setup on
+~~~~~~~~~~~~~~~~~
+
+Select how to retrieve new ports created after "port attach" command::
+
+   testpmd> set port setup on (iterator|event)
+
+For each new port, a setup is done.
+It will find the probed ports via RTE_ETH_FOREACH_MATCHING_DEV loop
+in iterator mode, or via RTE_ETH_EVENT_NEW in event mode.
+
 set tx loopback
 ~~~~~~~~~~~~~~~
 
-- 
2.19.0

Re: [PATCH v2 5/5] app/testpmd: setup attached ports on probe event

[Iremonger, Bernard]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, October 25, 2018 4:11 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Cc: dev@dpdk.org; ophirmu@mellanox.com; wisamm@mellanox.com; Yigit,
> Ferruh <ferruh.yigit@intel.com>; arybchenko@solarflare.com
> Subject: [PATCH v2 5/5] app/testpmd: setup attached ports on probe event
> 
> After probing is done, each new port must be setup.
> The new ports are currently guessed by iterating on ports matching the devargs
> string used for probing.
> 
> When probing a port, it is possible that one more port probing get triggered (e.g.
> PF is automatically probed when probing a VF representor). Such automatic
> probing will be caught only on event.
> 
> The iterator loop may be replaced by a call from the event callback.
> In order to be able to test both modes, a command is added to choose between
> iterator and event modes.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>

Re: [PATCH v2 0/5] app/testpmd: improve attach/detach support

[Ferruh Yigit]

On 10/25/2018 4:11 PM, Thomas Monjalon wrote:
> While working on EAL probe/remove and ethdev iterator/close,
> some scenarios appeared to not be managed by testpmd, especially
> because it was not designed for multi-ports devices:
>   - configure dependent port (detected via event)
>   - configuring twice (if already probed before)
>   - detaching twice
> 
> 
> Changes in v2 - Bernard review:
>   - improve pretty names of printed events
>   - add syntax of the new command in the guide
> 
> 
> Thomas Monjalon (5):
>   app/testpmd: check not detaching device twice
>   app/testpmd: merge ports list update functions
>   app/testpmd: check not configuring port twice
>   app/testpmd: move ethdev events registration
>   app/testpmd: setup attached ports on probe event

Series applied to dpdk-next-net/master, thanks.