* [RFC 01/13] app/testpmd: revert auto attach/detach [not found] <20250411234927.114568-1-stephen@networkplumber.org> @ 2025-04-11 23:44 ` Stephen Hemminger 2025-04-15 13:28 ` lihuisong (C) 0 siblings, 1 reply; 3+ messages in thread From: Stephen Hemminger @ 2025-04-11 23:44 UTC (permalink / raw) To: dev Cc: Stephen Hemminger, lihuisong, stable, Aman Singh, Chengwen Feng, Dongdong Liu 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC 01/13] app/testpmd: revert auto attach/detach 2025-04-11 23:44 ` [RFC 01/13] app/testpmd: revert auto attach/detach Stephen Hemminger @ 2025-04-15 13:28 ` lihuisong (C) 2025-04-16 0:06 ` Stephen Hemminger 0 siblings, 1 reply; 3+ messages in thread From: lihuisong (C) @ 2025-04-15 13:28 UTC (permalink / raw) To: Stephen Hemminger, dev; +Cc: stable, Aman Singh, Chengwen Feng, Dongdong Liu 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; ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC 01/13] app/testpmd: revert auto attach/detach 2025-04-15 13:28 ` lihuisong (C) @ 2025-04-16 0:06 ` Stephen Hemminger 0 siblings, 0 replies; 3+ messages in thread From: Stephen Hemminger @ 2025-04-16 0:06 UTC (permalink / raw) To: lihuisong (C); +Cc: dev, stable, Aman Singh, Chengwen Feng, Dongdong Liu On Tue, 15 Apr 2025 21:28:07 +0800 "lihuisong (C)" <lihuisong@huawei.com> wrote: > 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 > For now I am going to revert the testpmd change. Come back when you have something that works without breaking pdump application. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-04-16 0:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20250411234927.114568-1-stephen@networkplumber.org>
2025-04-11 23:44 ` [RFC 01/13] app/testpmd: revert auto attach/detach Stephen Hemminger
2025-04-15 13:28 ` lihuisong (C)
2025-04-16 0:06 ` Stephen Hemminger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).