[RFC 01/13] app/testpmd: revert auto attach/detach

[RFC 01/13] app/testpmd: revert auto attach/detach

[Stephen Hemminger]

Revert "app/testpmd: add port attach/detach for multiple process"
This reverts commit 994635edb2c038e64617bcf2790a8cd326c3e8e0.

This commit breaks using pdump and other secondary processes that
create there own devices. The patch makes testpmd grab any new
hotplug device and configure it.

It may also break failsafe and netvsc PMD handling of VF devices.
But I don't have access to test that part.

The patch is flawed in concept, and needs to go.

Bugzilla ID: 1695
Fixes: 994635edb2c0 ("app/testpmd: add port attach/detach for multiple process")
Cc: lihuisong@huawei.com
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 app/test-pmd/testpmd.c | 77 +++++++++++-------------------------------
 1 file changed, 20 insertions(+), 57 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index b5f0c02261..7f4e3b434d 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -705,7 +705,7 @@ eth_dev_set_mtu_mp(uint16_t port_id, uint16_t mtu)
 }
 
 /* Forward function declarations */
-static void setup_attached_port(void *arg);
+static void setup_attached_port(portid_t pi);
 static void check_all_ports_link_status(uint32_t port_mask);
 static int eth_event_callback(portid_t port_id,
 			      enum rte_eth_event_type type,
@@ -2906,7 +2906,6 @@ start_port(portid_t pid)
 		at_least_one_port_exist = true;
 
 		port = &ports[pi];
-
 		if (port->port_status == RTE_PORT_STOPPED) {
 			port->port_status = RTE_PORT_HANDLING;
 			all_ports_already_started = false;
@@ -3254,7 +3253,6 @@ 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;
-	printf("Now total ports is %d\n", nb_ports);
 }
 
 static void
@@ -3427,11 +3425,14 @@ attach_port(char *identifier)
 		return;
 	}
 
-	/* First attach mode: event
-	 * New port flag is updated on RTE_ETH_EVENT_NEW event
-	 */
+	/* first attach mode: event */
 	if (setup_on_probe_event) {
-		goto out;
+		/* 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 */
@@ -3439,17 +3440,13 @@ attach_port(char *identifier)
 		/* setup ports matching the devargs used for probing */
 		if (port_is_forwarding(pi))
 			continue; /* port was already attached before */
-		setup_attached_port((void *)(intptr_t)pi);
+		setup_attached_port(pi);
 	}
-out:
-	printf("Port %s is attached.\n", identifier);
-	printf("Done\n");
 }
 
 static void
-setup_attached_port(void *arg)
+setup_attached_port(portid_t pi)
 {
-	portid_t pi = (intptr_t)arg;
 	unsigned int socket_id;
 	int ret;
 
@@ -3464,8 +3461,14 @@ setup_attached_port(void *arg)
 			"Error during enabling promiscuous mode for port %u: %s - ignore\n",
 			pi, rte_strerror(-ret));
 
+	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);
+	printf("Done\n");
 }
 
 static void
@@ -3495,8 +3498,10 @@ detach_device(struct rte_device *dev)
 		TESTPMD_LOG(ERR, "Failed to detach device %s\n", rte_dev_name(dev));
 		return;
 	}
+	remove_invalid_ports();
 
 	printf("Device is detached\n");
+	printf("Now total ports is %d\n", nb_ports);
 	printf("Done\n");
 	return;
 }
@@ -3728,25 +3733,7 @@ rmv_port_callback(void *arg)
 		struct rte_device *device = dev_info.device;
 		close_port(port_id);
 		detach_device(device); /* might be already removed or have more ports */
-		remove_invalid_ports();
-	}
-	if (need_to_start)
-		start_packet_forwarding(0);
-}
-
-static void
-remove_invalid_ports_callback(void *arg)
-{
-	portid_t port_id = (intptr_t)arg;
-	int need_to_start = 0;
-
-	if (!test_done && port_is_forwarding(port_id)) {
-		need_to_start = 1;
-		stop_packet_forwarding();
 	}
-
-	remove_invalid_ports();
-
 	if (need_to_start)
 		start_packet_forwarding(0);
 }
@@ -3772,23 +3759,8 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 
 	switch (type) {
 	case RTE_ETH_EVENT_NEW:
-		/* The port in ports_id and fwd_ports_ids is always valid
-		 * from index 0 ~ (nb_ports - 1) due to updating their
-		 * position when one port is detached or removed.
-		 */
-		ports_ids[nb_ports++] = port_id;
-		fwd_ports_ids[nb_fwd_ports++] = port_id;
-		nb_cfg_ports = nb_fwd_ports;
-		printf("Port %d is probed. Now total ports is %d\n", port_id, nb_ports);
-
-		if (setup_on_probe_event) {
-			ports[port_id].need_setup = 1;
-			ports[port_id].port_status = RTE_PORT_HANDLING;
-		}
-		/* Can't initialize port directly in new event. */
-		if (rte_eal_alarm_set(100000, setup_attached_port,
-				      (void *)(intptr_t)port_id))
-			fprintf(stderr, "Could not set up deferred task to setup this attached port.\n");
+		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))
@@ -3801,15 +3773,6 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
 	case RTE_ETH_EVENT_DESTROY:
 		ports[port_id].port_status = RTE_PORT_CLOSED;
 		printf("Port %u is closed\n", port_id);
-		/*
-		 * Defer to remove port id due to the reason that the ethdev
-		 * state is changed from 'ATTACHED' to 'UNUSED' only after the
-		 * event callback finished. Otherwise this port id can not be
-		 * removed.
-		 */
-		if (rte_eal_alarm_set(100000, remove_invalid_ports_callback,
-				      (void *)(intptr_t)port_id))
-			fprintf(stderr, "Could not set up deferred task to remove this port id.\n");
 		break;
 	case RTE_ETH_EVENT_RX_AVAIL_THRESH: {
 		uint16_t rxq_id;
-- 
2.47.2

Re: [RFC 01/13] app/testpmd: revert auto attach/detach

[lihuisong (C)]

Hi Stephen,

The main cause of cpfl driver attach failure is the added alarm in new 
event callback to setup port automatically.
It's a question of when to set up the new port. Please see the 
discussion[1]. I have a stupid method, but I'm not very willing to do that.
For the Bugzilla id1695, I'll take a look at it.

[1] https://bugs.dpdk.org/show_bug.cgi?id=1671

/Huisong

在 2025/4/12 7:44, Stephen Hemminger 写道:
> Revert "app/testpmd: add port attach/detach for multiple process"
> This reverts commit 994635edb2c038e64617bcf2790a8cd326c3e8e0.
>
> This commit breaks using pdump and other secondary processes that
> create there own devices. The patch makes testpmd grab any new
> hotplug device and configure it.
>
> It may also break failsafe and netvsc PMD handling of VF devices.
> But I don't have access to test that part.
>
> The patch is flawed in concept, and needs to go.
>
> Bugzilla ID: 1695
> Fixes: 994635edb2c0 ("app/testpmd: add port attach/detach for multiple process")
> Cc: lihuisong@huawei.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>   app/test-pmd/testpmd.c | 77 +++++++++++-------------------------------
>   1 file changed, 20 insertions(+), 57 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index b5f0c02261..7f4e3b434d 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -705,7 +705,7 @@ eth_dev_set_mtu_mp(uint16_t port_id, uint16_t mtu)
>   }
>   
>   /* Forward function declarations */
> -static void setup_attached_port(void *arg);
> +static void setup_attached_port(portid_t pi);
>   static void check_all_ports_link_status(uint32_t port_mask);
>   static int eth_event_callback(portid_t port_id,
>   			      enum rte_eth_event_type type,
> @@ -2906,7 +2906,6 @@ start_port(portid_t pid)
>   		at_least_one_port_exist = true;
>   
>   		port = &ports[pi];
> -
>   		if (port->port_status == RTE_PORT_STOPPED) {
>   			port->port_status = RTE_PORT_HANDLING;
>   			all_ports_already_started = false;
> @@ -3254,7 +3253,6 @@ 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;
> -	printf("Now total ports is %d\n", nb_ports);
>   }
>   
>   static void
> @@ -3427,11 +3425,14 @@ attach_port(char *identifier)
>   		return;
>   	}
>   
> -	/* First attach mode: event
> -	 * New port flag is updated on RTE_ETH_EVENT_NEW event
> -	 */
> +	/* first attach mode: event */
>   	if (setup_on_probe_event) {
> -		goto out;
> +		/* 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 */
> @@ -3439,17 +3440,13 @@ attach_port(char *identifier)
>   		/* setup ports matching the devargs used for probing */
>   		if (port_is_forwarding(pi))
>   			continue; /* port was already attached before */
> -		setup_attached_port((void *)(intptr_t)pi);
> +		setup_attached_port(pi);
>   	}
> -out:
> -	printf("Port %s is attached.\n", identifier);
> -	printf("Done\n");
>   }
>   
>   static void
> -setup_attached_port(void *arg)
> +setup_attached_port(portid_t pi)
>   {
> -	portid_t pi = (intptr_t)arg;
>   	unsigned int socket_id;
>   	int ret;
>   
> @@ -3464,8 +3461,14 @@ setup_attached_port(void *arg)
>   			"Error during enabling promiscuous mode for port %u: %s - ignore\n",
>   			pi, rte_strerror(-ret));
>   
> +	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);
> +	printf("Done\n");
>   }
>   
>   static void
> @@ -3495,8 +3498,10 @@ detach_device(struct rte_device *dev)
>   		TESTPMD_LOG(ERR, "Failed to detach device %s\n", rte_dev_name(dev));
>   		return;
>   	}
> +	remove_invalid_ports();
>   
>   	printf("Device is detached\n");
> +	printf("Now total ports is %d\n", nb_ports);
>   	printf("Done\n");
>   	return;
>   }
> @@ -3728,25 +3733,7 @@ rmv_port_callback(void *arg)
>   		struct rte_device *device = dev_info.device;
>   		close_port(port_id);
>   		detach_device(device); /* might be already removed or have more ports */
> -		remove_invalid_ports();
> -	}
> -	if (need_to_start)
> -		start_packet_forwarding(0);
> -}
> -
> -static void
> -remove_invalid_ports_callback(void *arg)
> -{
> -	portid_t port_id = (intptr_t)arg;
> -	int need_to_start = 0;
> -
> -	if (!test_done && port_is_forwarding(port_id)) {
> -		need_to_start = 1;
> -		stop_packet_forwarding();
>   	}
> -
> -	remove_invalid_ports();
> -
>   	if (need_to_start)
>   		start_packet_forwarding(0);
>   }
> @@ -3772,23 +3759,8 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>   
>   	switch (type) {
>   	case RTE_ETH_EVENT_NEW:
> -		/* The port in ports_id and fwd_ports_ids is always valid
> -		 * from index 0 ~ (nb_ports - 1) due to updating their
> -		 * position when one port is detached or removed.
> -		 */
> -		ports_ids[nb_ports++] = port_id;
> -		fwd_ports_ids[nb_fwd_ports++] = port_id;
> -		nb_cfg_ports = nb_fwd_ports;
> -		printf("Port %d is probed. Now total ports is %d\n", port_id, nb_ports);
> -
> -		if (setup_on_probe_event) {
> -			ports[port_id].need_setup = 1;
> -			ports[port_id].port_status = RTE_PORT_HANDLING;
> -		}
> -		/* Can't initialize port directly in new event. */
> -		if (rte_eal_alarm_set(100000, setup_attached_port,
> -				      (void *)(intptr_t)port_id))
> -			fprintf(stderr, "Could not set up deferred task to setup this attached port.\n");
> +		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))
> @@ -3801,15 +3773,6 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>   	case RTE_ETH_EVENT_DESTROY:
>   		ports[port_id].port_status = RTE_PORT_CLOSED;
>   		printf("Port %u is closed\n", port_id);
> -		/*
> -		 * Defer to remove port id due to the reason that the ethdev
> -		 * state is changed from 'ATTACHED' to 'UNUSED' only after the
> -		 * event callback finished. Otherwise this port id can not be
> -		 * removed.
> -		 */
> -		if (rte_eal_alarm_set(100000, remove_invalid_ports_callback,
> -				      (void *)(intptr_t)port_id))
> -			fprintf(stderr, "Could not set up deferred task to remove this port id.\n");
>   		break;
>   	case RTE_ETH_EVENT_RX_AVAIL_THRESH: {
>   		uint16_t rxq_id;