[v2,10/11] examples/l3fwd: add graceful teardown for eventdevice
Checks
Commit Message
From: Pavan Nikhilesh <pbhagavatula@marvell.com>
Add graceful teardown that addresses both event mode and poll mode.
Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
examples/l3fwd/main.c | 49 ++++++++++++++++++++++++++++++-------------
1 file changed, 34 insertions(+), 15 deletions(-)
Comments
> Add graceful teardown that addresses both event mode and poll mode.
>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> ---
> examples/l3fwd/main.c | 49 ++++++++++++++++++++++++++++++-------------
> 1 file changed, 34 insertions(+), 15 deletions(-)
>
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 0ae64dd41..68998f42c 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -920,7 +920,7 @@ main(int argc, char **argv)
> struct lcore_conf *qconf;
> struct rte_eth_dev_info dev_info;
> struct rte_eth_txconf *txconf;
> - int ret;
> + int i, ret;
> unsigned nb_ports;
> uint16_t queueid, portid;
> unsigned lcore_id;
> @@ -1195,27 +1195,46 @@ main(int argc, char **argv)
> }
> }
>
> -
> check_all_ports_link_status(enabled_port_mask);
>
> ret = 0;
> /* launch per-lcore init on every lcore */
> rte_eal_mp_remote_launch(l3fwd_lkp.main_loop, NULL, CALL_MASTER);
> - RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> - if (rte_eal_wait_lcore(lcore_id) < 0) {
> - ret = -1;
> - break;
> + if (evt_rsrc->enabled) {
> + for (i = 0; i < evt_rsrc->rx_adptr.nb_rx_adptr; i++)
> + rte_event_eth_rx_adapter_stop(
> + evt_rsrc->rx_adptr.rx_adptr[i]);
> + for (i = 0; i < evt_rsrc->tx_adptr.nb_tx_adptr; i++)
> + rte_event_eth_tx_adapter_stop(
> + evt_rsrc->tx_adptr.tx_adptr[i]);
> +
> + RTE_ETH_FOREACH_DEV(portid) {
> + if ((enabled_port_mask & (1 << portid)) == 0)
> + continue;
> + rte_eth_dev_stop(portid);
> }
> - }
>
> - /* stop ports */
> - RTE_ETH_FOREACH_DEV(portid) {
> - if ((enabled_port_mask & (1 << portid)) == 0)
> - continue;
> - printf("Closing port %d...", portid);
> - rte_eth_dev_stop(portid);
> - rte_eth_dev_close(portid);
> - printf(" Done\n");
Why to stop ports *before* making sure all lcores are stopped?
Shouldn't that peace of code be identical for both poll and event mode?
Something like:
rte_eal_mp_wait_lcore();
RTE_ETH_FOREACH_DEV(portid) {
if ((enabled_port_mask & (1 << portid)) == 0)
continue;
rte_eth_dev_stop(portid);
rte_eth_dev_close(portid);
}
?
> + rte_eal_mp_wait_lcore();
> + RTE_ETH_FOREACH_DEV(portid) {
> + if ((enabled_port_mask & (1 << portid)) == 0)
> + continue;
> + rte_eth_dev_close(portid);
> + }
> +
> + rte_event_dev_stop(evt_rsrc->event_d_id);
> + rte_event_dev_close(evt_rsrc->event_d_id);
> +
> + } else {
> + rte_eal_mp_wait_lcore();
> +
> + RTE_ETH_FOREACH_DEV(portid) {
> + if ((enabled_port_mask & (1 << portid)) == 0)
> + continue;
> + printf("Closing port %d...", portid);
> + rte_eth_dev_stop(portid);
> + rte_eth_dev_close(portid);
> + printf(" Done\n");
> + }
> }
> printf("Bye...\n");
>
> --
> 2.17.1
>> Add graceful teardown that addresses both event mode and poll
>mode.
>>
>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> ---
>> examples/l3fwd/main.c | 49 ++++++++++++++++++++++++++++++-
>------------
>> 1 file changed, 34 insertions(+), 15 deletions(-)
>>
>> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
>> index 0ae64dd41..68998f42c 100644
>> --- a/examples/l3fwd/main.c
>> +++ b/examples/l3fwd/main.c
>> @@ -920,7 +920,7 @@ main(int argc, char **argv)
>> struct lcore_conf *qconf;
>> struct rte_eth_dev_info dev_info;
>> struct rte_eth_txconf *txconf;
>> - int ret;
>> + int i, ret;
>> unsigned nb_ports;
>> uint16_t queueid, portid;
>> unsigned lcore_id;
>> @@ -1195,27 +1195,46 @@ main(int argc, char **argv)
>> }
>> }
>>
>> -
>> check_all_ports_link_status(enabled_port_mask);
>>
>> ret = 0;
>> /* launch per-lcore init on every lcore */
>> rte_eal_mp_remote_launch(l3fwd_lkp.main_loop, NULL,
>CALL_MASTER);
>> - RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>> - if (rte_eal_wait_lcore(lcore_id) < 0) {
>> - ret = -1;
>> - break;
>> + if (evt_rsrc->enabled) {
>> + for (i = 0; i < evt_rsrc->rx_adptr.nb_rx_adptr; i++)
>> + rte_event_eth_rx_adapter_stop(
>> + evt_rsrc->rx_adptr.rx_adptr[i]);
>> + for (i = 0; i < evt_rsrc->tx_adptr.nb_tx_adptr; i++)
>> + rte_event_eth_tx_adapter_stop(
>> + evt_rsrc->tx_adptr.tx_adptr[i]);
>> +
>> + RTE_ETH_FOREACH_DEV(portid) {
>> + if ((enabled_port_mask & (1 << portid)) == 0)
>> + continue;
>> + rte_eth_dev_stop(portid);
>> }
>> - }
>>
>> - /* stop ports */
>> - RTE_ETH_FOREACH_DEV(portid) {
>> - if ((enabled_port_mask & (1 << portid)) == 0)
>> - continue;
>> - printf("Closing port %d...", portid);
>> - rte_eth_dev_stop(portid);
>> - rte_eth_dev_close(portid);
>> - printf(" Done\n");
>
>Why to stop ports *before* making sure all lcores are stopped?
>Shouldn't that peace of code be identical for both poll and event mode?
>Something like:
>rte_eal_mp_wait_lcore();
>
> RTE_ETH_FOREACH_DEV(portid) {
> if ((enabled_port_mask & (1 << portid)) == 0)
> continue;
> rte_eth_dev_stop(portid);
> rte_eth_dev_close(portid);
> }
>?
>
Event dev spec requires stopping producers before consumers else we might run into
deadlock in some cases.
>> + rte_eal_mp_wait_lcore();
>> + RTE_ETH_FOREACH_DEV(portid) {
>> + if ((enabled_port_mask & (1 << portid)) == 0)
>> + continue;
>> + rte_eth_dev_close(portid);
>> + }
>> +
>> + rte_event_dev_stop(evt_rsrc->event_d_id);
>> + rte_event_dev_close(evt_rsrc->event_d_id);
>> +
>> + } else {
>> + rte_eal_mp_wait_lcore();
>> +
>> + RTE_ETH_FOREACH_DEV(portid) {
>> + if ((enabled_port_mask & (1 << portid)) == 0)
>> + continue;
>> + printf("Closing port %d...", portid);
>> + rte_eth_dev_stop(portid);
>> + rte_eth_dev_close(portid);
>> + printf(" Done\n");
>> + }
>> }
>> printf("Bye...\n");
>>
>> --
>> 2.17.1
> >> Add graceful teardown that addresses both event mode and poll
> >mode.
> >>
> >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >> ---
> >> examples/l3fwd/main.c | 49 ++++++++++++++++++++++++++++++-
> >------------
> >> 1 file changed, 34 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> >> index 0ae64dd41..68998f42c 100644
> >> --- a/examples/l3fwd/main.c
> >> +++ b/examples/l3fwd/main.c
> >> @@ -920,7 +920,7 @@ main(int argc, char **argv)
> >> struct lcore_conf *qconf;
> >> struct rte_eth_dev_info dev_info;
> >> struct rte_eth_txconf *txconf;
> >> - int ret;
> >> + int i, ret;
> >> unsigned nb_ports;
> >> uint16_t queueid, portid;
> >> unsigned lcore_id;
> >> @@ -1195,27 +1195,46 @@ main(int argc, char **argv)
> >> }
> >> }
> >>
> >> -
> >> check_all_ports_link_status(enabled_port_mask);
> >>
> >> ret = 0;
> >> /* launch per-lcore init on every lcore */
> >> rte_eal_mp_remote_launch(l3fwd_lkp.main_loop, NULL,
> >CALL_MASTER);
> >> - RTE_LCORE_FOREACH_SLAVE(lcore_id) {
> >> - if (rte_eal_wait_lcore(lcore_id) < 0) {
> >> - ret = -1;
> >> - break;
> >> + if (evt_rsrc->enabled) {
> >> + for (i = 0; i < evt_rsrc->rx_adptr.nb_rx_adptr; i++)
> >> + rte_event_eth_rx_adapter_stop(
> >> + evt_rsrc->rx_adptr.rx_adptr[i]);
> >> + for (i = 0; i < evt_rsrc->tx_adptr.nb_tx_adptr; i++)
> >> + rte_event_eth_tx_adapter_stop(
> >> + evt_rsrc->tx_adptr.tx_adptr[i]);
> >> +
> >> + RTE_ETH_FOREACH_DEV(portid) {
> >> + if ((enabled_port_mask & (1 << portid)) == 0)
> >> + continue;
> >> + rte_eth_dev_stop(portid);
> >> }
> >> - }
> >>
> >> - /* stop ports */
> >> - RTE_ETH_FOREACH_DEV(portid) {
> >> - if ((enabled_port_mask & (1 << portid)) == 0)
> >> - continue;
> >> - printf("Closing port %d...", portid);
> >> - rte_eth_dev_stop(portid);
> >> - rte_eth_dev_close(portid);
> >> - printf(" Done\n");
> >
> >Why to stop ports *before* making sure all lcores are stopped?
> >Shouldn't that peace of code be identical for both poll and event mode?
> >Something like:
> >rte_eal_mp_wait_lcore();
> >
> > RTE_ETH_FOREACH_DEV(portid) {
> > if ((enabled_port_mask & (1 << portid)) == 0)
> > continue;
> > rte_eth_dev_stop(portid);
> > rte_eth_dev_close(portid);
> > }
> >?
> >
>
> Event dev spec requires stopping producers before consumers else we might run into
> deadlock in some cases.
Ok... but for TX path wouldn't core be a producer?
Also for that wouldn't rte_event_eth_(rx|tx)_adapter_stop(0 be enough?
I am not familiar with event-dev spec at all, so forgive for possibly dumb questions 😉
>
> >> + rte_eal_mp_wait_lcore();
> >> + RTE_ETH_FOREACH_DEV(portid) {
> >> + if ((enabled_port_mask & (1 << portid)) == 0)
> >> + continue;
> >> + rte_eth_dev_close(portid);
> >> + }
> >> +
> >> + rte_event_dev_stop(evt_rsrc->event_d_id);
> >> + rte_event_dev_close(evt_rsrc->event_d_id);
> >> +
> >> + } else {
> >> + rte_eal_mp_wait_lcore();
> >> +
> >> + RTE_ETH_FOREACH_DEV(portid) {
> >> + if ((enabled_port_mask & (1 << portid)) == 0)
> >> + continue;
> >> + printf("Closing port %d...", portid);
> >> + rte_eth_dev_stop(portid);
> >> + rte_eth_dev_close(portid);
> >> + printf(" Done\n");
> >> + }
> >> }
> >> printf("Bye...\n");
> >>
> >> --
> >> 2.17.1
>-----Original Message-----
>From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>Sent: Monday, January 6, 2020 5:43 PM
>To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>; Jerin
>Jacob Kollanukkaran <jerinj@marvell.com>; Kovacevic, Marko
><marko.kovacevic@intel.com>; Ori Kam <orika@mellanox.com>;
>Richardson, Bruce <bruce.richardson@intel.com>; Nicolau, Radu
><radu.nicolau@intel.com>; Akhil Goyal <akhil.goyal@nxp.com>;
>Kantecki, Tomasz <tomasz.kantecki@intel.com>; Sunil Kumar Kori
><skori@marvell.com>
>Cc: dev@dpdk.org
>Subject: [EXT] RE: [dpdk-dev] [PATCH v2 10/11] examples/l3fwd: add
>graceful teardown for eventdevice
>
>External Email
>
>----------------------------------------------------------------------
>
>> >> Add graceful teardown that addresses both event mode and poll
>> >mode.
>> >>
>> >> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>> >> ---
>> >> examples/l3fwd/main.c | 49
>++++++++++++++++++++++++++++++-
>> >------------
>> >> 1 file changed, 34 insertions(+), 15 deletions(-)
>> >>
>> >> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
>> >> index 0ae64dd41..68998f42c 100644
>> >> --- a/examples/l3fwd/main.c
>> >> +++ b/examples/l3fwd/main.c
>> >> @@ -920,7 +920,7 @@ main(int argc, char **argv)
>> >> struct lcore_conf *qconf;
>> >> struct rte_eth_dev_info dev_info;
>> >> struct rte_eth_txconf *txconf;
>> >> - int ret;
>> >> + int i, ret;
>> >> unsigned nb_ports;
>> >> uint16_t queueid, portid;
>> >> unsigned lcore_id;
>> >> @@ -1195,27 +1195,46 @@ main(int argc, char **argv)
>> >> }
>> >> }
>> >>
>> >> -
>> >> check_all_ports_link_status(enabled_port_mask);
>> >>
>> >> ret = 0;
>> >> /* launch per-lcore init on every lcore */
>> >> rte_eal_mp_remote_launch(l3fwd_lkp.main_loop, NULL,
>> >CALL_MASTER);
>> >> - RTE_LCORE_FOREACH_SLAVE(lcore_id) {
>> >> - if (rte_eal_wait_lcore(lcore_id) < 0) {
>> >> - ret = -1;
>> >> - break;
>> >> + if (evt_rsrc->enabled) {
>> >> + for (i = 0; i < evt_rsrc->rx_adptr.nb_rx_adptr; i++)
>> >> + rte_event_eth_rx_adapter_stop(
>> >> + evt_rsrc->rx_adptr.rx_adptr[i]);
>> >> + for (i = 0; i < evt_rsrc->tx_adptr.nb_tx_adptr; i++)
>> >> + rte_event_eth_tx_adapter_stop(
>> >> + evt_rsrc->tx_adptr.tx_adptr[i]);
>> >> +
>> >> + RTE_ETH_FOREACH_DEV(portid) {
>> >> + if ((enabled_port_mask & (1 << portid)) == 0)
>> >> + continue;
>> >> + rte_eth_dev_stop(portid);
>> >> }
>> >> - }
>> >>
>> >> - /* stop ports */
>> >> - RTE_ETH_FOREACH_DEV(portid) {
>> >> - if ((enabled_port_mask & (1 << portid)) == 0)
>> >> - continue;
>> >> - printf("Closing port %d...", portid);
>> >> - rte_eth_dev_stop(portid);
>> >> - rte_eth_dev_close(portid);
>> >> - printf(" Done\n");
>> >
>> >Why to stop ports *before* making sure all lcores are stopped?
>> >Shouldn't that peace of code be identical for both poll and event
>mode?
>> >Something like:
>> >rte_eal_mp_wait_lcore();
>> >
>> > RTE_ETH_FOREACH_DEV(portid) {
>> > if ((enabled_port_mask & (1 << portid)) == 0)
>> > continue;
>> > rte_eth_dev_stop(portid);
>> > rte_eth_dev_close(portid);
>> > }
>> >?
>> >
>>
>> Event dev spec requires stopping producers before consumers else
>we might run into
>> deadlock in some cases.
>
>Ok... but for TX path wouldn't core be a producer?
Not in all cases, true in case of SW event device and implementation defined in HW event devices.
Also both the cases of Tx poll on force_quit in case Tx path fails so that cores can exit.
if (flags & L3FWD_EVENT_TX_ENQ) {
ev.queue_id = tx_q_id;
ev.op = RTE_EVENT_OP_FORWARD;
while (rte_event_enqueue_burst(event_d_id, event_p_id,
&ev, 1) && !force_quit)
;
}
if (flags & L3FWD_EVENT_TX_DIRECT) {
rte_event_eth_tx_adapter_txq_set(mbuf, 0);
while (!rte_event_eth_tx_adapter_enqueue(event_d_id,
event_p_id, &ev, 1, 0) &&
!force_quit)
;
}
>Also for that wouldn't rte_event_eth_(rx|tx)_adapter_stop(0 be
>enough?
In case of HW event devices the above might be nop as control lies with driver/net.
And since anyway we need to stop ethernet device we might as well do it here.
>I am not familiar with event-dev spec at all, so forgive for possibly dumb
>questions 😉
😊
>
>>
>> >> + rte_eal_mp_wait_lcore();
>> >> + RTE_ETH_FOREACH_DEV(portid) {
>> >> + if ((enabled_port_mask & (1 << portid)) == 0)
>> >> + continue;
>> >> + rte_eth_dev_close(portid);
>> >> + }
>> >> +
>> >> + rte_event_dev_stop(evt_rsrc->event_d_id);
>> >> + rte_event_dev_close(evt_rsrc->event_d_id);
>> >> +
>> >> + } else {
>> >> + rte_eal_mp_wait_lcore();
>> >> +
>> >> + RTE_ETH_FOREACH_DEV(portid) {
>> >> + if ((enabled_port_mask & (1 << portid)) == 0)
>> >> + continue;
>> >> + printf("Closing port %d...", portid);
>> >> + rte_eth_dev_stop(portid);
>> >> + rte_eth_dev_close(portid);
>> >> + printf(" Done\n");
>> >> + }
>> >> }
>> >> printf("Bye...\n");
>> >>
>> >> --
>> >> 2.17.1
@@ -920,7 +920,7 @@ main(int argc, char **argv)
struct lcore_conf *qconf;
struct rte_eth_dev_info dev_info;
struct rte_eth_txconf *txconf;
- int ret;
+ int i, ret;
unsigned nb_ports;
uint16_t queueid, portid;
unsigned lcore_id;
@@ -1195,27 +1195,46 @@ main(int argc, char **argv)
}
}
-
check_all_ports_link_status(enabled_port_mask);
ret = 0;
/* launch per-lcore init on every lcore */
rte_eal_mp_remote_launch(l3fwd_lkp.main_loop, NULL, CALL_MASTER);
- RTE_LCORE_FOREACH_SLAVE(lcore_id) {
- if (rte_eal_wait_lcore(lcore_id) < 0) {
- ret = -1;
- break;
+ if (evt_rsrc->enabled) {
+ for (i = 0; i < evt_rsrc->rx_adptr.nb_rx_adptr; i++)
+ rte_event_eth_rx_adapter_stop(
+ evt_rsrc->rx_adptr.rx_adptr[i]);
+ for (i = 0; i < evt_rsrc->tx_adptr.nb_tx_adptr; i++)
+ rte_event_eth_tx_adapter_stop(
+ evt_rsrc->tx_adptr.tx_adptr[i]);
+
+ RTE_ETH_FOREACH_DEV(portid) {
+ if ((enabled_port_mask & (1 << portid)) == 0)
+ continue;
+ rte_eth_dev_stop(portid);
}
- }
- /* stop ports */
- RTE_ETH_FOREACH_DEV(portid) {
- if ((enabled_port_mask & (1 << portid)) == 0)
- continue;
- printf("Closing port %d...", portid);
- rte_eth_dev_stop(portid);
- rte_eth_dev_close(portid);
- printf(" Done\n");
+ rte_eal_mp_wait_lcore();
+ RTE_ETH_FOREACH_DEV(portid) {
+ if ((enabled_port_mask & (1 << portid)) == 0)
+ continue;
+ rte_eth_dev_close(portid);
+ }
+
+ rte_event_dev_stop(evt_rsrc->event_d_id);
+ rte_event_dev_close(evt_rsrc->event_d_id);
+
+ } else {
+ rte_eal_mp_wait_lcore();
+
+ RTE_ETH_FOREACH_DEV(portid) {
+ if ((enabled_port_mask & (1 << portid)) == 0)
+ continue;
+ printf("Closing port %d...", portid);
+ rte_eth_dev_stop(portid);
+ rte_eth_dev_close(portid);
+ printf(" Done\n");
+ }
}
printf("Bye...\n");