>-----Original Message----- >From: Muhammad Bilal <m.bi...@emumba.com> >Sent: Tuesday, June 2, 2020 5:57 PM >To: Sunil Kumar Kori <sk...@marvell.com> >Cc: declan.dohe...@intel.com; tomasz.kante...@intel.com; Pavan Nikhilesh >Bhagavatula <pbhagavat...@marvell.com>; dev@dpdk.org; >jgraj...@cisco.com; vipin.vargh...@intel.com >Subject: Re: [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case >of error > >On Tue, May 19, 2020 at 2:35 PM Sunil Kumar Kori <sk...@marvell.com> >wrote: >> >> >-----Original Message----- >> >From: Muhammad Bilal <m.bi...@emumba.com> >> >Sent: Tuesday, May 19, 2020 2:25 PM >> >To: declan.dohe...@intel.com; tomasz.kante...@intel.com; Pavan >> >Nikhilesh Bhagavatula <pbhagavat...@marvell.com>; Sunil Kumar Kori >> ><sk...@marvell.com> >> >Cc: dev@dpdk.org; Muhammad Bilal <m.bi...@emumba.com> >> >Subject: [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in >> >case of error >> > >> >External Email >> > >> >--------------------------------------------------------------------- >> >- Freeing the resources and call rte_eal_cleanup in case of error >> >exit. >> >Signed-off-by: Muhammad Bilal <m.bi...@emumba.com> >> >--- >> > examples/l2fwd-event/main.c | 43 >> >++++++++++++++++++++++++++++++------- >> > 1 file changed, 35 insertions(+), 8 deletions(-) >> > >> >diff --git a/examples/l2fwd-event/main.c >> >b/examples/l2fwd-event/main.c index 9593ef11e..442a664e9 100644 >> >--- a/examples/l2fwd-event/main.c >> >+++ b/examples/l2fwd-event/main.c >> >@@ -556,13 +556,26 @@ signal_handler(int signum) >> > } >> > } >> > >> >+static void >> >+stop_and_close_eth_dev(uint16_t portid) { >> >+ RTE_ETH_FOREACH_DEV(portid) { >> >+ printf("Closing port %d...", portid); >> >+ rte_eth_dev_stop(portid); >> >+ rte_eth_dev_close(portid); >> >+ printf(" Done\n"); >> >+ } >> >+ rte_eal_cleanup(); >> >+} >> >+ >> > int >> > main(int argc, char **argv) >> > { >> > struct l2fwd_resources *rsrc; >> > uint16_t nb_ports_available = 0; >> > uint32_t nb_ports_in_mask = 0; >> >- uint16_t port_id, last_port; >> >+ uint16_t port_id = 0; >> >+ uint16_t last_port; >> > uint32_t nb_mbufs; >> > uint16_t nb_ports; >> > int i, ret; >> >@@ -581,20 +594,26 @@ main(int argc, char **argv) >> > >> > /* parse application arguments (after the EAL ones) */ >> > ret = l2fwd_event_parse_args(argc, argv, rsrc); >> >- if (ret < 0) >> >+ if (ret < 0) { >> >+ stop_and_close_eth_dev(port_id); >> > rte_panic("Invalid L2FWD arguments\n"); >> >+ } >> > >Yes you are right we should use rte_exit instead of rte_panic, as rte_exit >internally calls rte_eal_cleanup function. >> IMO, up to this point only eal_init is done so rte_eal_cleanup will be >sufficient for this. >> Also another way to handle this, use rte_exit instead rte_panic. >> rte_exit internally calls rte_eal_cleanup. Refer l2fwd. >> >> Also I think, it is better to release the relevant resources on error. >Here I'm solving the problem reported in bugzilla id 437. The bug was that if >we use --vdev=net_memif with l2fwd application (and with its other variants) >then a socket is created by memif PMD, after rte_eal_init function has run >successfully. And if an error occurs then the application exits without freeing >the resources (socket). On running the application 2nd time we get an error of >"socket already exists". >As in the previous version of patch " >https://urldefense.proofpoint.com/v2/url?u=http- >3A__patches.dpdk.org_patch_70081_&d=DwIBaQ&c=nKjWec2b6R0mOyPaz7x >tfQ&r=dXeXaAMkP5COgn1zxHMyaF1_d9IIuq6vHQO6NrIPjaE&m=XKcRI2e7sMv >Y0nGabnBQl_Q8meL03FXFAjeNGdCV91A&s=TKq1J0W3QbnkeuG4c63payDWc >Pc4zTg4DumA95RVzwg&e= " it was recommended to clean the resources >when an error occurs. > >Here only using rte_eal_cleanup() is not solving the problem as using memif >PMD is creating a socket and it's only cleaned when >rte_eth_dev_close(portid) function is called. so that's why using it along with >rte_exit or rte_panic.
Understood but I am only thinking from user's perspective that user didn't do rte_eth_dev_configure and related APIs so closing the device using rte_eth_dev_close does not look good. May be other's (eal and memif PMD owners) can suggest something better. Please redirect this query to them for suggestions also. >> >> > printf("MAC updating %s\n", rsrc->mac_updating ? "enabled" : >> > "disabled"); >> > >> > nb_ports = rte_eth_dev_count_avail(); >> >- if (nb_ports == 0) >> >+ if (nb_ports == 0) { >> >+ stop_and_close_eth_dev(port_id); >> > rte_panic("No Ethernet ports - bye\n"); >> >+ } >> > >> Same as above. >> >> > /* check port mask to possible port mask */ >> >- if (rsrc->enabled_port_mask & ~((1 << nb_ports) - 1)) >> >+ if (rsrc->enabled_port_mask & ~((1 << nb_ports) - 1)) { >> >+ stop_and_close_eth_dev(port_id); >> > rte_panic("Invalid portmask; possible (0x%x)\n", >> > (1 << nb_ports) - 1); >> >+ } >> > >> Same as above. >> >> > if (!rsrc->port_pairs) { >> > last_port = 0; >> >@@ -621,8 +640,10 @@ main(int argc, char **argv) >> > rsrc->dst_ports[last_port] = last_port; >> > } >> > } else { >> >- if (check_port_pair_config(rsrc) < 0) >> >+ if (check_port_pair_config(rsrc) < 0) { >> >+ stop_and_close_eth_dev(port_id); >> > rte_panic("Invalid port pair config\n"); >> >+ } >> > } >> > >> > nb_mbufs = RTE_MAX(nb_ports * (RTE_TEST_RX_DESC_DEFAULT + >@@ >> >-634,12 +655,16 @@ main(int argc, char **argv) >> > rsrc->pktmbuf_pool = rte_pktmbuf_pool_create("mbuf_pool", >> > nb_mbufs, MEMPOOL_CACHE_SIZE, 0, >> > RTE_MBUF_DEFAULT_BUF_SIZE, rte_socket_id()); >> >- if (rsrc->pktmbuf_pool == NULL) >> >+ if (rsrc->pktmbuf_pool == NULL) { >> >+ stop_and_close_eth_dev(port_id); >> > rte_panic("Cannot init mbuf pool\n"); >> >+ } >> > >> > nb_ports_available = l2fwd_event_init_ports(rsrc); >> >- if (!nb_ports_available) >> >+ if (!nb_ports_available) { >> >+ stop_and_close_eth_dev(port_id); >> > rte_panic("All available ports are disabled. Please >> >set portmask.\n"); >> >+ } >> > >> > /* Configure eventdev parameters if required */ >> > if (rsrc->event_mode) >> >@@ -659,9 +684,11 @@ main(int argc, char **argv) >> > continue; >> > >> > ret = rte_eth_dev_start(port_id); >> >- if (ret < 0) >> >+ if (ret < 0) { >> >+ stop_and_close_eth_dev(port_id); >> > rte_panic("rte_eth_dev_start:err=%d, >> >port=%u\n", ret, >> > port_id); >> >+ } >> > } >> > >> > if (rsrc->event_mode) >> >-- >> >2.17.1 >>