diff mbox series

[v2,10/11] examples/l3fwd: add graceful teardown for eventdevice

Message ID 20191204144345.5736-11-pbhagavatula@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Jerin Jacob
Headers
Series example/l3fwd: introduce event device support |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Pavan Nikhilesh Bhagavatula Dec. 4, 2019, 2:43 p.m. UTC 
  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

Ananyev, Konstantin Jan. 3, 2020, 1:52 p.m. UTC | #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);
                }
?

> +		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
Pavan Nikhilesh Bhagavatula Jan. 6, 2020, 4:50 a.m. UTC | #2 
>> 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
Ananyev, Konstantin Jan. 6, 2020, 12:12 p.m. UTC | #3 
> >> 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
Pavan Nikhilesh Bhagavatula Jan. 7, 2020, 4:28 p.m. UTC | #4 
>-----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
diff mbox series

Patch

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");
+		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");