* [dpdk-dev] [PATCH 2/5] examples/l2fwd-jobstats: free resources in case of error
2020-05-19 8:54 [dpdk-dev] [PATCH 1/5] examples/l2fwd-event: free resources in case of error Muhammad Bilal
@ 2020-05-19 8:54 ` Muhammad Bilal
2020-05-19 8:54 ` [dpdk-dev] [PATCH 3/5] examples/l2fwd-keepalive: " Muhammad Bilal
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Muhammad Bilal @ 2020-05-19 8:54 UTC (permalink / raw)
To: declan.doherty, tomasz.kantecki, pbhagavatula, skori; +Cc: dev, Muhammad Bilal
Freeing the resources and call rte_eal_cleanup in case of error exit.
Signed-off-by: Muhammad Bilal <m.bilal@emumba.com>
---
examples/l2fwd-jobstats/main.c | 75 +++++++++++++++++++++++++++-------
1 file changed, 60 insertions(+), 15 deletions(-)
diff --git a/examples/l2fwd-jobstats/main.c b/examples/l2fwd-jobstats/main.c
index 396fd89db..e74e16113 100644
--- a/examples/l2fwd-jobstats/main.c
+++ b/examples/l2fwd-jobstats/main.c
@@ -739,6 +739,18 @@ check_all_ports_link_status(uint32_t port_mask)
}
}
+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)
{
@@ -749,7 +761,8 @@ main(int argc, char **argv)
char name[RTE_JOBSTATS_NAMESIZE];
uint16_t nb_ports;
uint16_t nb_ports_available = 0;
- uint16_t portid, last_port;
+ uint16_t portid = 0;
+ uint16_t last_port;
uint8_t i;
/* init EAL */
@@ -761,8 +774,10 @@ main(int argc, char **argv)
/* parse application arguments (after the EAL ones) */
ret = l2fwd_parse_args(argc, argv);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Invalid L2FWD arguments\n");
+ }
rte_timer_subsystem_init();
@@ -773,12 +788,16 @@ main(int argc, char **argv)
l2fwd_pktmbuf_pool =
rte_pktmbuf_pool_create("mbuf_pool", NB_MBUF, 32,
0, RTE_MBUF_DEFAULT_BUF_SIZE, rte_socket_id());
- if (l2fwd_pktmbuf_pool == NULL)
+ if (l2fwd_pktmbuf_pool == NULL) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Cannot init mbuf pool\n");
+ }
nb_ports = rte_eth_dev_count_avail();
- if (nb_ports == 0)
+ if (nb_ports == 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
+ }
/* reset l2fwd_dst_ports */
for (portid = 0; portid < RTE_MAX_ETHPORTS; portid++)
@@ -820,8 +839,10 @@ main(int argc, char **argv)
lcore_queue_conf[rx_lcore_id].n_rx_port ==
l2fwd_rx_queue_per_lcore) {
rx_lcore_id++;
- if (rx_lcore_id >= RTE_MAX_LCORE)
+ if (rx_lcore_id >= RTE_MAX_LCORE) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Not enough cores\n");
+ }
}
if (qconf != &lcore_queue_conf[rx_lcore_id])
@@ -852,32 +873,40 @@ main(int argc, char **argv)
fflush(stdout);
ret = rte_eth_dev_info_get(portid, &dev_info);
- if (ret != 0)
+ if (ret != 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"Error during getting device (port %u) info: %s\n",
portid, strerror(-ret));
+ }
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_MBUF_FAST_FREE)
local_port_conf.txmode.offloads |=
DEV_TX_OFFLOAD_MBUF_FAST_FREE;
ret = rte_eth_dev_configure(portid, 1, 1, &local_port_conf);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Cannot configure device: err=%d, port=%u\n",
ret, portid);
+ }
ret = rte_eth_dev_adjust_nb_rx_tx_desc(portid, &nb_rxd,
&nb_txd);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"Cannot adjust number of descriptors: err=%d, port=%u\n",
ret, portid);
+ }
ret = rte_eth_macaddr_get(portid,
&l2fwd_ports_eth_addr[portid]);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"Cannot get MAC address: err=%d, port=%u\n",
ret, portid);
+ }
/* init one RX queue */
fflush(stdout);
@@ -887,9 +916,11 @@ main(int argc, char **argv)
rte_eth_dev_socket_id(portid),
&rxq_conf,
l2fwd_pktmbuf_pool);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "rte_eth_rx_queue_setup:err=%d, port=%u\n",
ret, portid);
+ }
/* init one TX queue on each port */
txq_conf = dev_info.default_txconf;
@@ -898,39 +929,48 @@ main(int argc, char **argv)
ret = rte_eth_tx_queue_setup(portid, 0, nb_txd,
rte_eth_dev_socket_id(portid),
&txq_conf);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"rte_eth_tx_queue_setup:err=%d, port=%u\n",
ret, portid);
+ }
/* Initialize TX buffers */
tx_buffer[portid] = rte_zmalloc_socket("tx_buffer",
RTE_ETH_TX_BUFFER_SIZE(MAX_PKT_BURST), 0,
rte_eth_dev_socket_id(portid));
- if (tx_buffer[portid] == NULL)
+ if (tx_buffer[portid] == NULL) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Cannot allocate buffer for tx on port %u\n",
portid);
+ }
rte_eth_tx_buffer_init(tx_buffer[portid], MAX_PKT_BURST);
ret = rte_eth_tx_buffer_set_err_callback(tx_buffer[portid],
rte_eth_tx_buffer_count_callback,
&port_statistics[portid].dropped);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"Cannot set error callback for tx buffer on port %u\n",
portid);
+ }
/* Start device */
ret = rte_eth_dev_start(portid);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "rte_eth_dev_start:err=%d, port=%u\n",
ret, portid);
+ }
printf("done:\n");
ret = rte_eth_promiscuous_enable(portid);
if (ret != 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"rte_eth_promiscuous_enable:err=%s, port=%u\n",
rte_strerror(-ret), portid);
@@ -952,6 +992,7 @@ main(int argc, char **argv)
}
if (!nb_ports_available) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"All available ports are disabled. Please set portmask.\n");
}
@@ -965,8 +1006,10 @@ main(int argc, char **argv)
rte_spinlock_init(&qconf->lock);
- if (rte_jobstats_context_init(&qconf->jobs_context) != 0)
+ if (rte_jobstats_context_init(&qconf->jobs_context) != 0) {
+ stop_and_close_eth_dev(portid);
rte_panic("Jobs stats context for core %u init failed\n", lcore_id);
+ }
if (qconf->n_rx_port == 0) {
RTE_LOG(INFO, L2FWD,
@@ -985,6 +1028,7 @@ main(int argc, char **argv)
lcore_id, &l2fwd_flush_job, NULL);
if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(1, "Failed to reset flush job timer for lcore %u: %s",
lcore_id, rte_strerror(-ret));
}
@@ -1007,6 +1051,7 @@ main(int argc, char **argv)
&l2fwd_fwd_job, (void *)(uintptr_t)i);
if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(1, "Failed to reset lcore %u port %u job timer: %s",
lcore_id, qconf->rx_port_list[i], rte_strerror(-ret));
}
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread* [dpdk-dev] [PATCH 3/5] examples/l2fwd-keepalive: free resources in case of error
2020-05-19 8:54 [dpdk-dev] [PATCH 1/5] examples/l2fwd-event: free resources in case of error Muhammad Bilal
2020-05-19 8:54 ` [dpdk-dev] [PATCH 2/5] examples/l2fwd-jobstats: " Muhammad Bilal
@ 2020-05-19 8:54 ` Muhammad Bilal
2020-05-19 8:54 ` [dpdk-dev] [PATCH 4/5] examples/l2fwd-cat: " Muhammad Bilal
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Muhammad Bilal @ 2020-05-19 8:54 UTC (permalink / raw)
To: declan.doherty, tomasz.kantecki, pbhagavatula, skori; +Cc: dev, Muhammad Bilal
Freeing the resources and call rte_eal_cleanup in case of error exit.
Signed-off-by: Muhammad Bilal <m.bilal@emumba.com>
---
examples/l2fwd-keepalive/main.c | 97 +++++++++++++++++++++++++--------
1 file changed, 74 insertions(+), 23 deletions(-)
diff --git a/examples/l2fwd-keepalive/main.c b/examples/l2fwd-keepalive/main.c
index b7585d55e..04038711f 100644
--- a/examples/l2fwd-keepalive/main.c
+++ b/examples/l2fwd-keepalive/main.c
@@ -526,6 +526,18 @@ relay_core_state(void *ptr_data, const int id_core,
id_core, core_state, last_alive);
}
+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)
{
@@ -533,7 +545,8 @@ main(int argc, char **argv)
int ret;
uint16_t nb_ports;
uint16_t nb_ports_available = 0;
- uint16_t portid, last_port;
+ uint16_t portid = 0;
+ uint16_t last_port;
unsigned lcore_id, rx_lcore_id;
unsigned nb_ports_in_mask = 0;
unsigned int total_nb_mbufs;
@@ -559,21 +572,28 @@ main(int argc, char **argv)
/* parse application arguments (after the EAL ones) */
ret = l2fwd_parse_args(argc, argv);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Invalid L2FWD arguments\n");
+ }
nb_ports = rte_eth_dev_count_avail();
if (nb_ports == 0)
rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
/* create the mbuf pool */
- total_nb_mbufs = NB_MBUF_PER_PORT * nb_ports;
-
- l2fwd_pktmbuf_pool = rte_pktmbuf_pool_create("mbuf_pool",
- total_nb_mbufs, 32, 0, RTE_MBUF_DEFAULT_BUF_SIZE,
- rte_socket_id());
- if (l2fwd_pktmbuf_pool == NULL)
+ l2fwd_pktmbuf_pool = rte_pktmbuf_pool_create("mbuf_pool", NB_MBUF, 32,
+ 0, RTE_MBUF_DEFAULT_BUF_SIZE, rte_socket_id());
+ if (l2fwd_pktmbuf_pool == NULL) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Cannot init mbuf pool\n");
+ }
+
+ nb_ports = rte_eth_dev_count_avail();
+ if (nb_ports == 0) {
+ stop_and_close_eth_dev(portid);
+ rte_exit(EXIT_FAILURE, "No Ethernet ports - bye\n");
+ }
/* reset l2fwd_dst_ports */
for (portid = 0; portid < RTE_MAX_ETHPORTS; portid++)
@@ -615,8 +635,10 @@ main(int argc, char **argv)
lcore_queue_conf[rx_lcore_id].n_rx_port ==
l2fwd_rx_queue_per_lcore) {
rx_lcore_id++;
- if (rx_lcore_id >= RTE_MAX_LCORE)
+ if (rx_lcore_id >= RTE_MAX_LCORE) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Not enough cores\n");
+ }
}
if (qconf != &lcore_queue_conf[rx_lcore_id])
@@ -648,33 +670,41 @@ main(int argc, char **argv)
fflush(stdout);
ret = rte_eth_dev_info_get(portid, &dev_info);
- if (ret != 0)
+ if (ret != 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"Error during getting device (port %u) info: %s\n",
portid, strerror(-ret));
+ }
if (dev_info.tx_offload_capa & DEV_TX_OFFLOAD_MBUF_FAST_FREE)
local_port_conf.txmode.offloads |=
DEV_TX_OFFLOAD_MBUF_FAST_FREE;
ret = rte_eth_dev_configure(portid, 1, 1, &local_port_conf);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"Cannot configure device: err=%d, port=%u\n",
ret, portid);
+ }
ret = rte_eth_dev_adjust_nb_rx_tx_desc(portid, &nb_rxd,
&nb_txd);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"Cannot adjust number of descriptors: err=%d, port=%u\n",
ret, portid);
+ }
ret = rte_eth_macaddr_get(portid,
&l2fwd_ports_eth_addr[portid]);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"Cannot mac address: err=%d, port=%u\n",
ret, portid);
+ }
/* init one RX queue */
fflush(stdout);
@@ -684,10 +714,12 @@ main(int argc, char **argv)
rte_eth_dev_socket_id(portid),
&rxq_conf,
l2fwd_pktmbuf_pool);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"rte_eth_rx_queue_setup:err=%d, port=%u\n",
ret, portid);
+ }
/* init one TX queue on each port */
fflush(stdout);
@@ -696,41 +728,51 @@ main(int argc, char **argv)
ret = rte_eth_tx_queue_setup(portid, 0, nb_txd,
rte_eth_dev_socket_id(portid),
&txq_conf);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"rte_eth_tx_queue_setup:err=%d, port=%u\n",
ret, portid);
+ }
/* Initialize TX buffers */
tx_buffer[portid] = rte_zmalloc_socket("tx_buffer",
RTE_ETH_TX_BUFFER_SIZE(MAX_PKT_BURST), 0,
rte_eth_dev_socket_id(portid));
- if (tx_buffer[portid] == NULL)
+ if (tx_buffer[portid] == NULL) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Cannot allocate buffer for tx on port %u\n",
portid);
+ }
rte_eth_tx_buffer_init(tx_buffer[portid], MAX_PKT_BURST);
ret = rte_eth_tx_buffer_set_err_callback(tx_buffer[portid],
rte_eth_tx_buffer_count_callback,
&port_statistics[portid].dropped);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"Cannot set error callback for tx buffer on port %u\n",
portid);
+ }
/* Start device */
ret = rte_eth_dev_start(portid);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"rte_eth_dev_start:err=%d, port=%u\n",
ret, portid);
+ }
ret = rte_eth_promiscuous_enable(portid);
- if (ret != 0)
+ if (ret != 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"rte_eth_promiscuous_enable:err=%s, port=%u\n",
rte_strerror(-ret), portid);
+ }
printf("Port %u, MAC address: "
"%02X:%02X:%02X:%02X:%02X:%02X\n\n",
@@ -747,6 +789,7 @@ main(int argc, char **argv)
}
if (!nb_ports_available) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"All available ports are disabled. Please set portmask.\n");
}
@@ -761,13 +804,17 @@ main(int argc, char **argv)
ka_shm = NULL;
if (check_period > 0) {
ka_shm = rte_keepalive_shm_create();
- if (ka_shm == NULL)
+ if (ka_shm == NULL) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"rte_keepalive_shm_create() failed");
+ }
rte_global_keepalive_info =
rte_keepalive_create(&dead_core, ka_shm);
- if (rte_global_keepalive_info == NULL)
+ if (rte_global_keepalive_info == NULL) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "init_keep_alive() failed");
+ }
rte_keepalive_register_relay_callback(rte_global_keepalive_info,
relay_core_state, ka_shm);
rte_timer_init(&hb_timer);
@@ -778,8 +825,10 @@ main(int argc, char **argv)
(void(*)(struct rte_timer*, void*))
&rte_keepalive_dispatch_pings,
rte_global_keepalive_info
- ) != 0 )
+ ) != 0 ) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Keepalive setup failure.\n");
+ }
}
if (timer_period > 0) {
if (rte_timer_reset(&stats_timer,
@@ -787,8 +836,10 @@ main(int argc, char **argv)
PERIODICAL,
rte_lcore_id(),
&print_stats, NULL
- ) != 0 )
+ ) != 0 ) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Stats setup failure.\n");
+ }
}
/* launch per-lcore init on every slave lcore */
RTE_LCORE_FOREACH_SLAVE(lcore_id) {
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread* [dpdk-dev] [PATCH 4/5] examples/l2fwd-cat: free resources in case of error
2020-05-19 8:54 [dpdk-dev] [PATCH 1/5] examples/l2fwd-event: free resources in case of error Muhammad Bilal
2020-05-19 8:54 ` [dpdk-dev] [PATCH 2/5] examples/l2fwd-jobstats: " Muhammad Bilal
2020-05-19 8:54 ` [dpdk-dev] [PATCH 3/5] examples/l2fwd-keepalive: " Muhammad Bilal
@ 2020-05-19 8:54 ` Muhammad Bilal
2020-05-19 8:54 ` [dpdk-dev] [PATCH 5/5] examples/l2fwd-crypto: " Muhammad Bilal
2020-05-19 9:34 ` [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: " Sunil Kumar Kori
4 siblings, 0 replies; 15+ messages in thread
From: Muhammad Bilal @ 2020-05-19 8:54 UTC (permalink / raw)
To: declan.doherty, tomasz.kantecki, pbhagavatula, skori; +Cc: dev, Muhammad Bilal
Freeing the resources and call rte_eal_cleanup in case of error exit.
Signed-off-by: Muhammad Bilal <m.bilal@emumba.com>
---
examples/l2fwd-cat/l2fwd-cat.c | 32 ++++++++++++++++++++++++++------
1 file changed, 26 insertions(+), 6 deletions(-)
diff --git a/examples/l2fwd-cat/l2fwd-cat.c b/examples/l2fwd-cat/l2fwd-cat.c
index 45a497c08..06eeae9ae 100644
--- a/examples/l2fwd-cat/l2fwd-cat.c
+++ b/examples/l2fwd-cat/l2fwd-cat.c
@@ -147,6 +147,18 @@ lcore_main(void)
}
}
+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();
+}
+
/*
* The main function, which does initialization and calls the per-lcore
* functions.
@@ -156,7 +168,7 @@ main(int argc, char *argv[])
{
struct rte_mempool *mbuf_pool;
unsigned nb_ports;
- uint16_t portid;
+ uint16_t portid = 0;
/* Initialize the Environment Abstraction Layer (EAL). */
int ret = rte_eal_init(argc, argv);
@@ -171,30 +183,38 @@ main(int argc, char *argv[])
* Please see l2fwd-cat documentation for more info.
*/
ret = cat_init(argc, argv);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "PQOS: L3CA init failed!\n");
+ }
argc -= ret;
argv += ret;
/* Check that there is an even number of ports to send/receive on. */
nb_ports = rte_eth_dev_count_avail();
- if (nb_ports < 2 || (nb_ports & 1))
+ if (nb_ports < 2 || (nb_ports & 1)) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Error: number of ports must be even\n");
+ }
/* Creates a new mempool in memory to hold the mbufs. */
mbuf_pool = rte_pktmbuf_pool_create("MBUF_POOL", NUM_MBUFS * nb_ports,
MBUF_CACHE_SIZE, 0, RTE_MBUF_DEFAULT_BUF_SIZE, rte_socket_id());
- if (mbuf_pool == NULL)
+ if (mbuf_pool == NULL) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Cannot create mbuf pool\n");
+ }
/* Initialize all ports. */
RTE_ETH_FOREACH_DEV(portid)
- if (port_init(portid, mbuf_pool) != 0)
+ if (port_init(portid, mbuf_pool) != 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Cannot init port %"PRIu16 "\n",
portid);
-
+ }
+
if (rte_lcore_count() > 1)
printf("\nWARNING: Too many lcores enabled. Only 1 used.\n");
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread* [dpdk-dev] [PATCH 5/5] examples/l2fwd-crypto: free resources in case of error
2020-05-19 8:54 [dpdk-dev] [PATCH 1/5] examples/l2fwd-event: free resources in case of error Muhammad Bilal
` (2 preceding siblings ...)
2020-05-19 8:54 ` [dpdk-dev] [PATCH 4/5] examples/l2fwd-cat: " Muhammad Bilal
@ 2020-05-19 8:54 ` Muhammad Bilal
2020-05-19 9:34 ` [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: " Sunil Kumar Kori
4 siblings, 0 replies; 15+ messages in thread
From: Muhammad Bilal @ 2020-05-19 8:54 UTC (permalink / raw)
To: declan.doherty, tomasz.kantecki, pbhagavatula, skori; +Cc: dev, Muhammad Bilal
Freeing the resources and call rte_eal_cleanup in case of error exit.
Signed-off-by: Muhammad Bilal <m.bilal@emumba.com>
---
examples/l2fwd-crypto/main.c | 54 ++++++++++++++++++++++++++++--------
1 file changed, 43 insertions(+), 11 deletions(-)
diff --git a/examples/l2fwd-crypto/main.c b/examples/l2fwd-crypto/main.c
index fcb55c370..169f011f8 100644
--- a/examples/l2fwd-crypto/main.c
+++ b/examples/l2fwd-crypto/main.c
@@ -2660,6 +2660,18 @@ reserve_key_memory(struct l2fwd_crypto_options *options)
options->aad.phys_addr = rte_malloc_virt2iova(options->aad.data);
}
+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)
{
@@ -2667,7 +2679,7 @@ main(int argc, char **argv)
struct l2fwd_crypto_options options;
uint8_t nb_cryptodevs, cdev_id;
- uint16_t portid;
+ uint16_t portid = 0;
unsigned lcore_id, rx_lcore_id = 0;
int ret, enabled_cdevcount, enabled_portcount;
uint8_t enabled_cdevs[RTE_CRYPTO_MAX_DEVS] = {0};
@@ -2684,8 +2696,10 @@ main(int argc, char **argv)
/* parse application arguments (after the EAL ones) */
ret = l2fwd_crypto_parse_args(&options, argc, argv);
- if (ret < 0)
+ if (ret < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Invalid L2FWD-CRYPTO arguments\n");
+ }
printf("MAC updating %s\n",
options.mac_updating ? "enabled" : "disabled");
@@ -2694,20 +2708,26 @@ main(int argc, char **argv)
l2fwd_pktmbuf_pool = rte_pktmbuf_pool_create("mbuf_pool", NB_MBUF, 512,
sizeof(struct rte_crypto_op),
RTE_MBUF_DEFAULT_BUF_SIZE, rte_socket_id());
- if (l2fwd_pktmbuf_pool == NULL)
+ if (l2fwd_pktmbuf_pool == NULL) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Cannot create mbuf pool\n");
+ }
/* create crypto op pool */
l2fwd_crypto_op_pool = rte_crypto_op_pool_create("crypto_op_pool",
RTE_CRYPTO_OP_TYPE_SYMMETRIC, NB_MBUF, 128, MAXIMUM_IV_LENGTH,
rte_socket_id());
- if (l2fwd_crypto_op_pool == NULL)
+ if (l2fwd_crypto_op_pool == NULL) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Cannot create crypto op pool\n");
+ }
/* Enable Ethernet ports */
enabled_portcount = initialize_ports(&options);
- if (enabled_portcount < 1)
+ if (enabled_portcount < 1) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Failed to initial Ethernet ports\n");
+ }
/* Initialize the port/queue configuration of each logical core */
RTE_ETH_FOREACH_DEV(portid) {
@@ -2719,9 +2739,11 @@ main(int argc, char **argv)
if (options.single_lcore && qconf == NULL) {
while (rte_lcore_is_enabled(rx_lcore_id) == 0) {
rx_lcore_id++;
- if (rx_lcore_id >= RTE_MAX_LCORE)
+ if (rx_lcore_id >= RTE_MAX_LCORE) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"Not enough cores\n");
+ }
}
} else if (!options.single_lcore) {
/* get the lcore_id for this port */
@@ -2729,9 +2751,11 @@ main(int argc, char **argv)
lcore_queue_conf[rx_lcore_id].nb_rx_ports ==
options.nb_ports_per_lcore) {
rx_lcore_id++;
- if (rx_lcore_id >= RTE_MAX_LCORE)
+ if (rx_lcore_id >= RTE_MAX_LCORE) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"Not enough cores\n");
+ }
}
}
@@ -2748,13 +2772,17 @@ main(int argc, char **argv)
/* Enable Crypto devices */
enabled_cdevcount = initialize_cryptodevs(&options, enabled_portcount,
enabled_cdevs);
- if (enabled_cdevcount < 0)
+ if (enabled_cdevcount < 0) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Failed to initialize crypto devices\n");
+ }
- if (enabled_cdevcount < enabled_portcount)
+ if (enabled_cdevcount < enabled_portcount) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE, "Number of capable crypto devices (%d) "
"has to be more or equal to number of ports (%d)\n",
enabled_cdevcount, enabled_portcount);
+ }
nb_cryptodevs = rte_cryptodev_count();
@@ -2769,9 +2797,11 @@ main(int argc, char **argv)
if (options.single_lcore && qconf == NULL) {
while (rte_lcore_is_enabled(rx_lcore_id) == 0) {
rx_lcore_id++;
- if (rx_lcore_id >= RTE_MAX_LCORE)
+ if (rx_lcore_id >= RTE_MAX_LCORE) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"Not enough cores\n");
+ }
}
} else if (!options.single_lcore) {
/* get the lcore_id for this port */
@@ -2779,9 +2809,11 @@ main(int argc, char **argv)
lcore_queue_conf[rx_lcore_id].nb_crypto_devs ==
options.nb_ports_per_lcore) {
rx_lcore_id++;
- if (rx_lcore_id >= RTE_MAX_LCORE)
+ if (rx_lcore_id >= RTE_MAX_LCORE) {
+ stop_and_close_eth_dev(portid);
rte_exit(EXIT_FAILURE,
"Not enough cores\n");
+ }
}
}
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case of error
2020-05-19 8:54 [dpdk-dev] [PATCH 1/5] examples/l2fwd-event: free resources in case of error Muhammad Bilal
` (3 preceding siblings ...)
2020-05-19 8:54 ` [dpdk-dev] [PATCH 5/5] examples/l2fwd-crypto: " Muhammad Bilal
@ 2020-05-19 9:34 ` Sunil Kumar Kori
2020-06-02 12:27 ` Muhammad Bilal
4 siblings, 1 reply; 15+ messages in thread
From: Sunil Kumar Kori @ 2020-05-19 9:34 UTC (permalink / raw)
To: Muhammad Bilal, declan.doherty, tomasz.kantecki,
Pavan Nikhilesh Bhagavatula
Cc: dev
>-----Original Message-----
>From: Muhammad Bilal <m.bilal@emumba.com>
>Sent: Tuesday, May 19, 2020 2:25 PM
>To: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan Nikhilesh
>Bhagavatula <pbhagavatula@marvell.com>; Sunil Kumar Kori
><skori@marvell.com>
>Cc: dev@dpdk.org; Muhammad Bilal <m.bilal@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.bilal@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");
>+ }
>
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.
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case of error
2020-05-19 9:34 ` [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: " Sunil Kumar Kori
@ 2020-06-02 12:27 ` Muhammad Bilal
2020-06-10 10:01 ` Sunil Kumar Kori
0 siblings, 1 reply; 15+ messages in thread
From: Muhammad Bilal @ 2020-06-02 12:27 UTC (permalink / raw)
To: Sunil Kumar Kori
Cc: declan.doherty, tomasz.kantecki, Pavan Nikhilesh Bhagavatula,
dev, jgrajcia, vipin.varghese
On Tue, May 19, 2020 at 2:35 PM Sunil Kumar Kori <skori@marvell.com> wrote:
>
> >-----Original Message-----
> >From: Muhammad Bilal <m.bilal@emumba.com>
> >Sent: Tuesday, May 19, 2020 2:25 PM
> >To: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan Nikhilesh
> >Bhagavatula <pbhagavatula@marvell.com>; Sunil Kumar Kori
> ><skori@marvell.com>
> >Cc: dev@dpdk.org; Muhammad Bilal <m.bilal@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.bilal@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 "
http://patches.dpdk.org/patch/70081/ " 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.
>
> > 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
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case of error
2020-06-02 12:27 ` Muhammad Bilal
@ 2020-06-10 10:01 ` Sunil Kumar Kori
2020-06-15 12:00 ` Muhammad Bilal
2020-06-16 11:47 ` Jakub Grajciar -X (jgrajcia - PANTHEON TECH SRO at Cisco)
0 siblings, 2 replies; 15+ messages in thread
From: Sunil Kumar Kori @ 2020-06-10 10:01 UTC (permalink / raw)
To: Muhammad Bilal
Cc: declan.doherty, tomasz.kantecki, Pavan Nikhilesh Bhagavatula,
dev, jgrajcia, vipin.varghese
>-----Original Message-----
>From: Muhammad Bilal <m.bilal@emumba.com>
>Sent: Tuesday, June 2, 2020 5:57 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan Nikhilesh
>Bhagavatula <pbhagavatula@marvell.com>; dev@dpdk.org;
>jgrajcia@cisco.com; vipin.varghese@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 <skori@marvell.com>
>wrote:
>>
>> >-----Original Message-----
>> >From: Muhammad Bilal <m.bilal@emumba.com>
>> >Sent: Tuesday, May 19, 2020 2:25 PM
>> >To: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan
>> >Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Sunil Kumar Kori
>> ><skori@marvell.com>
>> >Cc: dev@dpdk.org; Muhammad Bilal <m.bilal@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.bilal@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
>>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case of error
2020-06-10 10:01 ` Sunil Kumar Kori
@ 2020-06-15 12:00 ` Muhammad Bilal
2020-06-16 5:53 ` Sunil Kumar Kori
2020-06-16 11:47 ` Jakub Grajciar -X (jgrajcia - PANTHEON TECH SRO at Cisco)
1 sibling, 1 reply; 15+ messages in thread
From: Muhammad Bilal @ 2020-06-15 12:00 UTC (permalink / raw)
To: Sunil Kumar Kori
Cc: declan.doherty, tomasz.kantecki, Pavan Nikhilesh Bhagavatula,
dev, jgrajcia, vipin.varghese
On Wed, Jun 10, 2020 at 3:01 PM Sunil Kumar Kori <skori@marvell.com> wrote:
>
> >-----Original Message-----
> >From: Muhammad Bilal <m.bilal@emumba.com>
> >Sent: Tuesday, June 2, 2020 5:57 PM
> >To: Sunil Kumar Kori <skori@marvell.com>
> >Cc: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan Nikhilesh
> >Bhagavatula <pbhagavatula@marvell.com>; dev@dpdk.org;
> >jgrajcia@cisco.com; vipin.varghese@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 <skori@marvell.com>
> >wrote:
> >>
> >> >-----Original Message-----
> >> >From: Muhammad Bilal <m.bilal@emumba.com>
> >> >Sent: Tuesday, May 19, 2020 2:25 PM
> >> >To: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan
> >> >Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Sunil Kumar Kori
> >> ><skori@marvell.com>
> >> >Cc: dev@dpdk.org; Muhammad Bilal <m.bilal@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.bilal@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.
Maintainer also recommended to call rte_eth_dev_close() for this
problem in bug No. 437
Here : https://bugs.dpdk.org/show_bug.cgi?id=437#c1
>
> >>
> >> > 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
> >>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case of error
2020-06-15 12:00 ` Muhammad Bilal
@ 2020-06-16 5:53 ` Sunil Kumar Kori
2020-06-16 10:14 ` Muhammad Bilal
0 siblings, 1 reply; 15+ messages in thread
From: Sunil Kumar Kori @ 2020-06-16 5:53 UTC (permalink / raw)
To: Muhammad Bilal
Cc: declan.doherty, tomasz.kantecki, Pavan Nikhilesh Bhagavatula,
dev, jgrajcia, vipin.varghese
>-----Original Message-----
>From: Muhammad Bilal <m.bilal@emumba.com>
>Sent: Monday, June 15, 2020 5:30 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan Nikhilesh
>Bhagavatula <pbhagavatula@marvell.com>; dev@dpdk.org;
>jgrajcia@cisco.com; vipin.varghese@intel.com
>Subject: Re: [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case
>of error
>
>On Wed, Jun 10, 2020 at 3:01 PM Sunil Kumar Kori <skori@marvell.com>
>wrote:
>>
>> >-----Original Message-----
>> >From: Muhammad Bilal <m.bilal@emumba.com>
>> >Sent: Tuesday, June 2, 2020 5:57 PM
>> >To: Sunil Kumar Kori <skori@marvell.com>
>> >Cc: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan
>> >Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; dev@dpdk.org;
>> >jgrajcia@cisco.com; vipin.varghese@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 <skori@marvell.com>
>> >wrote:
>> >>
>> >> >-----Original Message-----
>> >> >From: Muhammad Bilal <m.bilal@emumba.com>
>> >> >Sent: Tuesday, May 19, 2020 2:25 PM
>> >> >To: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan
>> >> >Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Sunil Kumar Kori
>> >> ><skori@marvell.com>
>> >> >Cc: dev@dpdk.org; Muhammad Bilal <m.bilal@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.bilal@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=nKjWec2b6R0mOyPaz7
>x
>>
>>tfQ&r=dXeXaAMkP5COgn1zxHMyaF1_d9IIuq6vHQO6NrIPjaE&m=XKcRI2e7sM
>v
>>
>>Y0nGabnBQl_Q8meL03FXFAjeNGdCV91A&s=TKq1J0W3QbnkeuG4c63payDW
>c
>> >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.
>Maintainer also recommended to call rte_eth_dev_close() for this problem in
>bug No. 437 Here : https://urldefense.proofpoint.com/v2/url?u=https-
>3A__bugs.dpdk.org_show-5Fbug.cgi-3Fid-3D437-
>23c1&d=DwIBaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=dXeXaAMkP5COgn1zxHMy
>aF1_d9IIuq6vHQO6NrIPjaE&m=C6PYaZqbOP-IfP6yK-
>nkxpSLonovGalGzl9QJ03qBmU&s=SEiTU3-
>EhOV5r16Kk9ZbW67E_dzuHvls4Fn41WyfCN0&e=
In mentioned link, I see that Vipin has also suggested to clean this resource in memif PMD
But did not found any reason to drop that idea. I am still in favor of this.
Also as a query, I see that changes are only l2fwd and its variants.
Is this problem not applicable to other applications where memif is used ?
>>
>> >>
>> >> > 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
>> >>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case of error
2020-06-16 5:53 ` Sunil Kumar Kori
@ 2020-06-16 10:14 ` Muhammad Bilal
2020-06-16 10:38 ` Sunil Kumar Kori
0 siblings, 1 reply; 15+ messages in thread
From: Muhammad Bilal @ 2020-06-16 10:14 UTC (permalink / raw)
To: Sunil Kumar Kori
Cc: declan.doherty, tomasz.kantecki, Pavan Nikhilesh Bhagavatula,
dev, jgrajcia, vipin.varghese
On Tue, Jun 16, 2020 at 10:53 AM Sunil Kumar Kori <skori@marvell.com> wrote:
>
> >-----Original Message-----
> >From: Muhammad Bilal <m.bilal@emumba.com>
> >Sent: Monday, June 15, 2020 5:30 PM
> >To: Sunil Kumar Kori <skori@marvell.com>
> >Cc: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan Nikhilesh
> >Bhagavatula <pbhagavatula@marvell.com>; dev@dpdk.org;
> >jgrajcia@cisco.com; vipin.varghese@intel.com
> >Subject: Re: [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case
> >of error
> >
> >On Wed, Jun 10, 2020 at 3:01 PM Sunil Kumar Kori <skori@marvell.com>
> >wrote:
> >>
> >> >-----Original Message-----
> >> >From: Muhammad Bilal <m.bilal@emumba.com>
> >> >Sent: Tuesday, June 2, 2020 5:57 PM
> >> >To: Sunil Kumar Kori <skori@marvell.com>
> >> >Cc: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan
> >> >Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; dev@dpdk.org;
> >> >jgrajcia@cisco.com; vipin.varghese@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 <skori@marvell.com>
> >> >wrote:
> >> >>
> >> >> >-----Original Message-----
> >> >> >From: Muhammad Bilal <m.bilal@emumba.com>
> >> >> >Sent: Tuesday, May 19, 2020 2:25 PM
> >> >> >To: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan
> >> >> >Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Sunil Kumar Kori
> >> >> ><skori@marvell.com>
> >> >> >Cc: dev@dpdk.org; Muhammad Bilal <m.bilal@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.bilal@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=nKjWec2b6R0mOyPaz7
> >x
> >>
> >>tfQ&r=dXeXaAMkP5COgn1zxHMyaF1_d9IIuq6vHQO6NrIPjaE&m=XKcRI2e7sM
> >v
> >>
> >>Y0nGabnBQl_Q8meL03FXFAjeNGdCV91A&s=TKq1J0W3QbnkeuG4c63payDW
> >c
> >> >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.
> >Maintainer also recommended to call rte_eth_dev_close() for this problem in
> >bug No. 437 Here : https://urldefense.proofpoint.com/v2/url?u=https-
> >3A__bugs.dpdk.org_show-5Fbug.cgi-3Fid-3D437-
> >23c1&d=DwIBaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=dXeXaAMkP5COgn1zxHMy
> >aF1_d9IIuq6vHQO6NrIPjaE&m=C6PYaZqbOP-IfP6yK-
> >nkxpSLonovGalGzl9QJ03qBmU&s=SEiTU3-
> >EhOV5r16Kk9ZbW67E_dzuHvls4Fn41WyfCN0&e=
>
> In mentioned link, I see that Vipin has also suggested to clean this resource in memif PMD
> But did not found any reason to drop that idea. I am still in favor of this.
You are right, Vipin initially recommended change in Memif PMD but
later agreed upon cleaning the resources in applications.
> Also as a query, I see that changes are only l2fwd and its variants.
> Is this problem not applicable to other applications where memif is used ?
This problem is applicable to other applications where memif is used.
We are working on other applications as time permits, Some patches
already submitted e.g:
http://patches.dpdk.org/project/dpdk/list/?series=10458
>
> >>
> >> >>
> >> >> > 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
> >> >>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case of error
2020-06-16 10:14 ` Muhammad Bilal
@ 2020-06-16 10:38 ` Sunil Kumar Kori
2023-06-12 16:56 ` Stephen Hemminger
0 siblings, 1 reply; 15+ messages in thread
From: Sunil Kumar Kori @ 2020-06-16 10:38 UTC (permalink / raw)
To: Muhammad Bilal
Cc: declan.doherty, tomasz.kantecki, Pavan Nikhilesh Bhagavatula,
dev, jgrajcia, vipin.varghese
>-----Original Message-----
>From: Muhammad Bilal <m.bilal@emumba.com>
>Sent: Tuesday, June 16, 2020 3:44 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan Nikhilesh
>Bhagavatula <pbhagavatula@marvell.com>; dev@dpdk.org;
>jgrajcia@cisco.com; vipin.varghese@intel.com
>Subject: Re: [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case
>of error
>
>On Tue, Jun 16, 2020 at 10:53 AM Sunil Kumar Kori <skori@marvell.com>
>wrote:
>>
>> >-----Original Message-----
>> >From: Muhammad Bilal <m.bilal@emumba.com>
>> >Sent: Monday, June 15, 2020 5:30 PM
>> >To: Sunil Kumar Kori <skori@marvell.com>
>> >Cc: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan
>> >Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; dev@dpdk.org;
>> >jgrajcia@cisco.com; vipin.varghese@intel.com
>> >Subject: Re: [EXT] [PATCH 1/5] examples/l2fwd-event: free resources
>> >in case of error
>> >
>> >On Wed, Jun 10, 2020 at 3:01 PM Sunil Kumar Kori <skori@marvell.com>
>> >wrote:
>> >>
>> >> >-----Original Message-----
>> >> >From: Muhammad Bilal <m.bilal@emumba.com>
>> >> >Sent: Tuesday, June 2, 2020 5:57 PM
>> >> >To: Sunil Kumar Kori <skori@marvell.com>
>> >> >Cc: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan
>> >> >Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; dev@dpdk.org;
>> >> >jgrajcia@cisco.com; vipin.varghese@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
>> >> ><skori@marvell.com>
>> >> >wrote:
>> >> >>
>> >> >> >-----Original Message-----
>> >> >> >From: Muhammad Bilal <m.bilal@emumba.com>
>> >> >> >Sent: Tuesday, May 19, 2020 2:25 PM
>> >> >> >To: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan
>> >> >> >Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Sunil Kumar
>> >> >> >Kori <skori@marvell.com>
>> >> >> >Cc: dev@dpdk.org; Muhammad Bilal <m.bilal@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.bilal@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=nKjWec2b6R0mOyPaz
>7
>> >x
>> >>
>>
>>>tfQ&r=dXeXaAMkP5COgn1zxHMyaF1_d9IIuq6vHQO6NrIPjaE&m=XKcRI2e7s
>M
>> >v
>> >>
>>
>>>Y0nGabnBQl_Q8meL03FXFAjeNGdCV91A&s=TKq1J0W3QbnkeuG4c63payD
>W
>> >c
>> >> >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.
>> >Maintainer also recommended to call rte_eth_dev_close() for this
>> >problem in bug No. 437 Here :
>> >https://urldefense.proofpoint.com/v2/url?u=https-
>> >3A__bugs.dpdk.org_show-5Fbug.cgi-3Fid-3D437-
>>
>>23c1&d=DwIBaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=dXeXaAMkP5COgn1zxHM
>y
>> >aF1_d9IIuq6vHQO6NrIPjaE&m=C6PYaZqbOP-IfP6yK-
>> >nkxpSLonovGalGzl9QJ03qBmU&s=SEiTU3-
>> >EhOV5r16Kk9ZbW67E_dzuHvls4Fn41WyfCN0&e=
>>
>> In mentioned link, I see that Vipin has also suggested to clean this
>> resource in memif PMD But did not found any reason to drop that idea. I am
>still in favor of this.
>
>You are right, Vipin initially recommended change in Memif PMD but later
>agreed upon cleaning the resources in applications.
>
Okay.
>> Also as a query, I see that changes are only l2fwd and its variants.
>> Is this problem not applicable to other applications where memif is used ?
>
>This problem is applicable to other applications where memif is used.
>We are working on other applications as time permits, Some patches already
>submitted e.g:
>https://urldefense.proofpoint.com/v2/url?u=http-
>3A__patches.dpdk.org_project_dpdk_list_-3Fseries-
>3D10458&d=DwIBaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=dXeXaAMkP5COgn1zx
>HMyaF1_d9IIuq6vHQO6NrIPjaE&m=v5R9KG76zcLqWJF7H_FNAliRW3Peorns7b
>O9tYEPZ8g&s=HjKnDn7I8abChgl_c1uoMfy8hCEb09geYL7sTHoatNY&e=
>
>>
>> >>
>> >> >>
>> >> >> > 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
>> >> >>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case of error
2020-06-16 10:38 ` Sunil Kumar Kori
@ 2023-06-12 16:56 ` Stephen Hemminger
2023-06-12 17:17 ` Sunil Kumar Kori
0 siblings, 1 reply; 15+ messages in thread
From: Stephen Hemminger @ 2023-06-12 16:56 UTC (permalink / raw)
To: Sunil Kumar Kori
Cc: Muhammad Bilal, declan.doherty, tomasz.kantecki,
Pavan Nikhilesh Bhagavatula, dev, jgrajcia, vipin.varghese
On Tue, 16 Jun 2020 10:38:01 +0000
Sunil Kumar Kori <skori@marvell.com> wrote:
> >> >> >>
> >> >> >> 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".
Bottom line: Fix memif, don't wallpaper over the bug by fixing the examples.
Other user applications, can and do crash. The driver needs to be robust
and handle that.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case of error
2023-06-12 16:56 ` Stephen Hemminger
@ 2023-06-12 17:17 ` Sunil Kumar Kori
0 siblings, 0 replies; 15+ messages in thread
From: Sunil Kumar Kori @ 2023-06-12 17:17 UTC (permalink / raw)
To: Muhammad Bilal
Cc: Stephen Hemminger, declan.doherty, tomasz.kantecki,
Pavan Nikhilesh Bhagavatula, dev, jgrajcia, vipin.varghese
Previous mail was destined to me which I believe is not correct.
Updating "To recipient".
Regards
Sunil Kumar Kori
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday, June 12, 2023 10:27 PM
> To: Sunil Kumar Kori <skori@marvell.com>
> Cc: Muhammad Bilal <m.bilal@emumba.com>; declan.doherty@intel.com;
> tomasz.kantecki@intel.com; Pavan Nikhilesh Bhagavatula
> <pbhagavatula@marvell.com>; dev@dpdk.org; jgrajcia@cisco.com;
> vipin.varghese@intel.com
> Subject: Re: [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: free
> resources in case of error
>
> On Tue, 16 Jun 2020 10:38:01 +0000
> Sunil Kumar Kori <skori@marvell.com> wrote:
>
> > >> >> >>
> > >> >> >> 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".
>
> Bottom line: Fix memif, don't wallpaper over the bug by fixing the examples.
>
> Other user applications, can and do crash. The driver needs to be robust and
> handle that.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [dpdk-dev] [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case of error
2020-06-10 10:01 ` Sunil Kumar Kori
2020-06-15 12:00 ` Muhammad Bilal
@ 2020-06-16 11:47 ` Jakub Grajciar -X (jgrajcia - PANTHEON TECH SRO at Cisco)
1 sibling, 0 replies; 15+ messages in thread
From: Jakub Grajciar -X (jgrajcia - PANTHEON TECH SRO at Cisco) @ 2020-06-16 11:47 UTC (permalink / raw)
To: Sunil Kumar Kori, Muhammad Bilal
Cc: declan.doherty, tomasz.kantecki, Pavan Nikhilesh Bhagavatula,
dev, vipin.varghese
> -----Original Message-----
> From: Sunil Kumar Kori <skori@marvell.com>
> Sent: Wednesday, June 10, 2020 12:01 PM
> To: Muhammad Bilal <m.bilal@emumba.com>
> Cc: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan Nikhilesh
> Bhagavatula <pbhagavatula@marvell.com>; dev@dpdk.org; Jakub Grajciar -X
> (jgrajcia - PANTHEON TECH SRO at Cisco) <jgrajcia@cisco.com>;
> vipin.varghese@intel.com
> Subject: RE: [EXT] [PATCH 1/5] examples/l2fwd-event: free resources in case
> of error
>
> >-----Original Message-----
> >From: Muhammad Bilal <m.bilal@emumba.com>
> >Sent: Tuesday, June 2, 2020 5:57 PM
> >To: Sunil Kumar Kori <skori@marvell.com>
> >Cc: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan
> >Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; dev@dpdk.org;
> >jgrajcia@cisco.com; vipin.varghese@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 <skori@marvell.com>
> >wrote:
> >>
> >> >-----Original Message-----
> >> >From: Muhammad Bilal <m.bilal@emumba.com>
> >> >Sent: Tuesday, May 19, 2020 2:25 PM
> >> >To: declan.doherty@intel.com; tomasz.kantecki@intel.com; Pavan
> >> >Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Sunil Kumar Kori
> >> ><skori@marvell.com>
> >> >Cc: dev@dpdk.org; Muhammad Bilal <m.bilal@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.bilal@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=nKjWec2b6R0mOyPa
> z7x
> >tfQ&r=dXeXaAMkP5COgn1zxHMyaF1_d9IIuq6vHQO6NrIPjaE&m=XKcRI2e7
> sMv
> >Y0nGabnBQl_Q8meL03FXFAjeNGdCV91A&s=TKq1J0W3QbnkeuG4c63payD
> Wc
> >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.
>
So if calling ret_eth_dev_close() makes sense after rte_eth_dev_configure() the socket initialization can be moved to the configure function. One more thought: struct rte_vdev_driver holds .probe and .remove functions. Shouldn't .remove be called before exiting if .probe was called? This would close the socket in case of memif PMD.
> >>
> >> > 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
> >>
^ permalink raw reply [flat|nested] 15+ messages in thread