[v2,4/4] app/testpmd: show example to handle hot unplug
diff mbox series

Message ID 1529668268-7462-5-git-send-email-jia.guo@intel.com
State Superseded, archived
Delegated to: Thomas Monjalon
Headers show
Series
  • hot plug failure handle mechanism
Related show

Checks

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

Commit Message

Jeff Guo June 22, 2018, 11:51 a.m. UTC
Use testpmd for example, to show how an application smoothly handle
failure when device being hot unplug. If app have enabled the device event
monitor and register the hot plug event’s callback before running, once
app detect the removal event, the callback would be called. It will first
stop the packet forwarding, then stop the port, close the port, and finally
detach the port to remove the device out from the device lists.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v2->v1(v21):
rebase testpmd code
---
 app/test-pmd/testpmd.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

Comments

Iremonger, Bernard June 26, 2018, 10:06 a.m. UTC | #1
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Jeff Guo
> Sent: Friday, June 22, 2018 12:51 PM
> To: stephen@networkplumber.org; Richardson, Bruce
> <bruce.richardson@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>; Ananyev,
> Konstantin <konstantin.ananyev@intel.com>; gaetan.rivet@6wind.com; Wu,
> Jingjing <jingjing.wu@intel.com>; thomas@monjalon.net;
> motih@mellanox.com; matan@mellanox.com; Van Haaren, Harry
> <harry.van.haaren@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; He,
> Shaopeng <shaopeng.he@intel.com>
> Cc: jblunck@infradead.org; shreyansh.jain@nxp.com; dev@dpdk.org; Guo, Jia
> <jia.guo@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> Subject: [dpdk-dev] [PATCH v2 4/4] app/testpmd: show example to handle hot
> unplug
> 
> Use testpmd for example, to show how an application smoothly handle failure
> when device being hot unplug. If app have enabled the device event monitor and
> register the hot plug event’s callback before running, once app detect the
> removal event, the callback would be called. It will first stop the packet
> forwarding, then stop the port, close the port, and finally detach the port to
> remove the device out from the device lists.
> 
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---

Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>
Matan Azrad June 26, 2018, 11:58 a.m. UTC | #2
Hi Jeff

Please see comments...

From: Jeff Guo
> Sent: Friday, June 22, 2018 2:51 PM
> To: stephen@networkplumber.org; bruce.richardson@intel.com;
> ferruh.yigit@intel.com; konstantin.ananyev@intel.com;
> gaetan.rivet@6wind.com; jingjing.wu@intel.com; Thomas Monjalon
> <thomas@monjalon.net>; Mordechay Haimovsky <motih@mellanox.com>;
> Matan Azrad <matan@mellanox.com>; harry.van.haaren@intel.com;
> qi.z.zhang@intel.com; shaopeng.he@intel.com
> Cc: jblunck@infradead.org; shreyansh.jain@nxp.com; dev@dpdk.org;
> jia.guo@intel.com; helin.zhang@intel.com
> Subject: [PATCH v2 4/4] app/testpmd: show example to handle hot unplug
> 
> Use testpmd for example, to show how an application smoothly handle failure
> when device being hot unplug. If app have enabled the device event monitor
> and register the hot plug event’s callback before running, once app detect the
> removal event, the callback would be called. It will first stop the packet
> forwarding, then stop the port, close the port, and finally detach the port to
> remove the device out from the device lists.
> 
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v2->v1(v21):
> rebase testpmd code
> ---
>  app/test-pmd/testpmd.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 24c1998..286f242 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -1951,9 +1951,10 @@ eth_dev_event_callback_unregister(void)
>  void
>  attach_port(char *identifier)
>  {
> -	portid_t pi = 0;
>  	unsigned int socket_id;
> 
> +	portid_t pi = rte_eth_dev_count_avail();
> +
>  	printf("Attaching a new port...\n");
> 
>  	if (identifier == NULL) {
> @@ -2125,16 +2126,25 @@ check_all_ports_link_status(uint32_t port_mask)
> static void  rmv_event_callback(void *arg)  {

There is a race between ethdev RMV event to the EAL remove event, I think the application must synchronize it if both are configured.

> +	struct rte_eth_dev *dev;
> +
>  	int need_to_start = 0;
>  	int org_no_link_check = no_link_check;
>  	portid_t port_id = (intptr_t)arg;
> 
>  	RTE_ETH_VALID_PORTID_OR_RET(port_id);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (dev->state == RTE_ETH_DEV_UNUSED)
> +		return;

Can you explain why do you check the state?
Doesn't RTE_ETH_VALID_PORTID_OR_RET do it?

> +	printf("removing device %s\n", dev->device->name);
> 
>  	if (!test_done && port_is_forwarding(port_id)) {
>  		need_to_start = 1;
>  		stop_packet_forwarding();
>  	}
> +
>  	no_link_check = 1;
>  	stop_port(port_id);
>  	no_link_check = org_no_link_check;
> @@ -2196,6 +2206,9 @@ static void
>  eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
>  			     __rte_unused void *arg)
>  {
> +	uint16_t port_id;
> +	int ret;
> +
>  	if (type >= RTE_DEV_EVENT_MAX) {
>  		fprintf(stderr, "%s called upon invalid event %d\n",
>  			__func__, type);
> @@ -2206,9 +2219,12 @@ eth_dev_event_callback(char *device_name, enum
> rte_dev_event_type type,
>  	case RTE_DEV_EVENT_REMOVE:
>  		RTE_LOG(ERR, EAL, "The device: %s has been removed!\n",
>  			device_name);
> -		/* TODO: After finish failure handle, begin to stop
> -		 * packet forward, stop port, close port, detach port.
> -		 */
> +		ret = rte_eth_dev_get_port_by_name(device_name, &port_id);
> +		if (ret) {
> +			printf("can not get port by device %s!\n",
> device_name);
> +			return;
> +		}
> +		rmv_event_callback((void *)(intptr_t)port_id);
>  		break;
>  	case RTE_DEV_EVENT_ADD:
>  		RTE_LOG(ERR, EAL, "The device: %s has been added!\n", @@ -
> 2736,7 +2752,6 @@ main(int argc, char** argv)
>  			return -1;
>  		}
>  		eth_dev_event_callback_register();
> -
>  	}
> 
>  	if (start_port(RTE_PORT_ALL) != 0)
> --
> 2.7.4
Jeff Guo June 26, 2018, 3:33 p.m. UTC | #3
hi, matan

thanks for your review, see comment.

On 6/26/2018 7:58 PM, Matan Azrad wrote:
> Hi Jeff
>
> Please see comments...
>
> From: Jeff Guo
>> Sent: Friday, June 22, 2018 2:51 PM
>> To: stephen@networkplumber.org; bruce.richardson@intel.com;
>> ferruh.yigit@intel.com; konstantin.ananyev@intel.com;
>> gaetan.rivet@6wind.com; jingjing.wu@intel.com; Thomas Monjalon
>> <thomas@monjalon.net>; Mordechay Haimovsky <motih@mellanox.com>;
>> Matan Azrad <matan@mellanox.com>; harry.van.haaren@intel.com;
>> qi.z.zhang@intel.com; shaopeng.he@intel.com
>> Cc: jblunck@infradead.org; shreyansh.jain@nxp.com; dev@dpdk.org;
>> jia.guo@intel.com; helin.zhang@intel.com
>> Subject: [PATCH v2 4/4] app/testpmd: show example to handle hot unplug
>>
>> Use testpmd for example, to show how an application smoothly handle failure
>> when device being hot unplug. If app have enabled the device event monitor
>> and register the hot plug event’s callback before running, once app detect the
>> removal event, the callback would be called. It will first stop the packet
>> forwarding, then stop the port, close the port, and finally detach the port to
>> remove the device out from the device lists.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>> v2->v1(v21):
>> rebase testpmd code
>> ---
>>   app/test-pmd/testpmd.c | 25 ++++++++++++++++++++-----
>>   1 file changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> 24c1998..286f242 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -1951,9 +1951,10 @@ eth_dev_event_callback_unregister(void)
>>   void
>>   attach_port(char *identifier)
>>   {
>> -	portid_t pi = 0;
>>   	unsigned int socket_id;
>>
>> +	portid_t pi = rte_eth_dev_count_avail();
>> +
>>   	printf("Attaching a new port...\n");
>>
>>   	if (identifier == NULL) {
>> @@ -2125,16 +2126,25 @@ check_all_ports_link_status(uint32_t port_mask)
>> static void  rmv_event_callback(void *arg)  {
> There is a race between ethdev RMV event to the EAL remove event, I think the application must synchronize it if both are configured.

Is this race will affect the device detaching? what is the side effect 
and what is your propose to synchronize it, and i still think about 
that.....

>> +	struct rte_eth_dev *dev;
>> +
>>   	int need_to_start = 0;
>>   	int org_no_link_check = no_link_check;
>>   	portid_t port_id = (intptr_t)arg;
>>
>>   	RTE_ETH_VALID_PORTID_OR_RET(port_id);
>> +	dev = &rte_eth_devices[port_id];
>> +
>> +	if (dev->state == RTE_ETH_DEV_UNUSED)
>> +		return;
> Can you explain why do you check the state?
> Doesn't RTE_ETH_VALID_PORTID_OR_RET do it?

correct, i check that it is no used here. thank info.

>> +	printf("removing device %s\n", dev->device->name);
>>
>>   	if (!test_done && port_is_forwarding(port_id)) {
>>   		need_to_start = 1;
>>   		stop_packet_forwarding();
>>   	}
>> +
>>   	no_link_check = 1;
>>   	stop_port(port_id);
>>   	no_link_check = org_no_link_check;
>> @@ -2196,6 +2206,9 @@ static void
>>   eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
>>   			     __rte_unused void *arg)
>>   {
>> +	uint16_t port_id;
>> +	int ret;
>> +
>>   	if (type >= RTE_DEV_EVENT_MAX) {
>>   		fprintf(stderr, "%s called upon invalid event %d\n",
>>   			__func__, type);
>> @@ -2206,9 +2219,12 @@ eth_dev_event_callback(char *device_name, enum
>> rte_dev_event_type type,
>>   	case RTE_DEV_EVENT_REMOVE:
>>   		RTE_LOG(ERR, EAL, "The device: %s has been removed!\n",
>>   			device_name);
>> -		/* TODO: After finish failure handle, begin to stop
>> -		 * packet forward, stop port, close port, detach port.
>> -		 */
>> +		ret = rte_eth_dev_get_port_by_name(device_name, &port_id);
>> +		if (ret) {
>> +			printf("can not get port by device %s!\n",
>> device_name);
>> +			return;
>> +		}
>> +		rmv_event_callback((void *)(intptr_t)port_id);
>>   		break;
>>   	case RTE_DEV_EVENT_ADD:
>>   		RTE_LOG(ERR, EAL, "The device: %s has been added!\n", @@ -
>> 2736,7 +2752,6 @@ main(int argc, char** argv)
>>   			return -1;
>>   		}
>>   		eth_dev_event_callback_register();
>> -
>>   	}
>>
>>   	if (start_port(RTE_PORT_ALL) != 0)
>> --
>> 2.7.4

Patch
diff mbox series

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 24c1998..286f242 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -1951,9 +1951,10 @@  eth_dev_event_callback_unregister(void)
 void
 attach_port(char *identifier)
 {
-	portid_t pi = 0;
 	unsigned int socket_id;
 
+	portid_t pi = rte_eth_dev_count_avail();
+
 	printf("Attaching a new port...\n");
 
 	if (identifier == NULL) {
@@ -2125,16 +2126,25 @@  check_all_ports_link_status(uint32_t port_mask)
 static void
 rmv_event_callback(void *arg)
 {
+	struct rte_eth_dev *dev;
+
 	int need_to_start = 0;
 	int org_no_link_check = no_link_check;
 	portid_t port_id = (intptr_t)arg;
 
 	RTE_ETH_VALID_PORTID_OR_RET(port_id);
+	dev = &rte_eth_devices[port_id];
+
+	if (dev->state == RTE_ETH_DEV_UNUSED)
+		return;
+
+	printf("removing device %s\n", dev->device->name);
 
 	if (!test_done && port_is_forwarding(port_id)) {
 		need_to_start = 1;
 		stop_packet_forwarding();
 	}
+
 	no_link_check = 1;
 	stop_port(port_id);
 	no_link_check = org_no_link_check;
@@ -2196,6 +2206,9 @@  static void
 eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
 			     __rte_unused void *arg)
 {
+	uint16_t port_id;
+	int ret;
+
 	if (type >= RTE_DEV_EVENT_MAX) {
 		fprintf(stderr, "%s called upon invalid event %d\n",
 			__func__, type);
@@ -2206,9 +2219,12 @@  eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
 	case RTE_DEV_EVENT_REMOVE:
 		RTE_LOG(ERR, EAL, "The device: %s has been removed!\n",
 			device_name);
-		/* TODO: After finish failure handle, begin to stop
-		 * packet forward, stop port, close port, detach port.
-		 */
+		ret = rte_eth_dev_get_port_by_name(device_name, &port_id);
+		if (ret) {
+			printf("can not get port by device %s!\n", device_name);
+			return;
+		}
+		rmv_event_callback((void *)(intptr_t)port_id);
 		break;
 	case RTE_DEV_EVENT_ADD:
 		RTE_LOG(ERR, EAL, "The device: %s has been added!\n",
@@ -2736,7 +2752,6 @@  main(int argc, char** argv)
 			return -1;
 		}
 		eth_dev_event_callback_register();
-
 	}
 
 	if (start_port(RTE_PORT_ALL) != 0)