diff mbox series

[v12,7/7] testpmd: use hot-unplug failure handle mechanism

Message ID 1538483726-96411-8-git-send-email-jia.guo@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v12,1/7] bus: add hot-unplug handler |

Checks

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

Commit Message

Guo, Jia Oct. 2, 2018, 12:35 p.m. UTC 
  This patch use testpmd for example, to show how an app smoothly handle
failure when device be hot-unplug. Except app should enabled the device
event monitor and register the hotplug event’s callback, it also need
enable hotplug handle mechanism before running. Once app detect the
removal event, the hot-unplug callback would be called. It will first stop
the packet forwarding, then stop the port, close the port, and finally
detach the port to clean the device and release the resources.

Signed-off-by: Jeff Guo <jia.guo@intel.com>
---
v12->v11:
no change.
---
 app/test-pmd/testpmd.c | 39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

Comments

Iremonger, Bernard Oct. 2, 2018, 3:21 p.m. UTC | #1 
Hi Jeff,

<snip>

> Subject: [PATCH v12 7/7] testpmd: use hot-unplug failure handle mechanism

./devtools/check-git-log.sh -1
Wrong headline label:
        testpmd: use hot-unplug failure handle mechanism
 
> This patch use testpmd for example, to show how an app smoothly handle
> failure when device be hot-unplug. Except app should enabled the device event
> monitor and register the hotplug event’s callback, it also need enable hotplug
> handle mechanism before running. Once app detect the removal event, the hot-
> unplug callback would be called. It will first stop the packet forwarding, then
> stop the port, close the port, and finally detach the port to clean the device and
> release the resources.
> 
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v12->v11:
> no change.
> ---
>  app/test-pmd/testpmd.c | 39 +++++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 001f0e5..bfef483 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2093,14 +2093,22 @@ pmd_test_exit(void)
> 
>  	if (hot_plug) {
>  		ret = rte_dev_event_monitor_stop();
> -		if (ret)
> +		if (ret) {
>  			RTE_LOG(ERR, EAL,
>  				"fail to stop device event monitor.");
> +			return;
> +		}
> 
>  		ret = eth_dev_event_callback_unregister();
>  		if (ret)

Should there be an RTE_LOG() call here?

> +			return;
> +
> +		ret = rte_dev_hotplug_handle_disable();
> +		if (ret) {
>  			RTE_LOG(ERR, EAL,
> -				"fail to unregister all event callbacks.");
> +				"fail to disable hotplug handling.");
> +			return;
> +		}
>  	}
> 
>  	printf("\nBye...\n");
> @@ -2244,6 +2252,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);
> @@ -2254,9 +2265,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);

It would be better to use an RTE_LOG() call here instead of printf().

> +			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", @@ -
> 2779,14 +2793,23 @@ main(int argc, char** argv)
>  	init_config();
> 
>  	if (hot_plug) {
> -		/* enable hot plug monitoring */
> +		ret = rte_dev_hotplug_handle_enable();
> +		if (ret) {
> +			RTE_LOG(ERR, EAL,
> +				"fail to enable hotplug handling.");
> +			return -1;
> +		}
> +
>  		ret = rte_dev_event_monitor_start();
>  		if (ret) {
> -			rte_errno = EINVAL;
> +			RTE_LOG(ERR, EAL,
> +				"fail to start device event monitoring.");
>  			return -1;
>  		}
> -		eth_dev_event_callback_register();
> 
> +		ret = eth_dev_event_callback_register();
> +		if (ret)

Should there be an RTE_LOG() call here?

> +			return -1;
>  	}
> 
>  	if (start_port(RTE_PORT_ALL) != 0)
> --
> 2.7.4

Regards,

Bernard.
Guo, Jia Oct. 4, 2018, 2:56 a.m. UTC | #2 
hi, bernard

thanks for your review, comment as below.


On 10/2/2018 11:21 PM, Iremonger, Bernard wrote:
> Hi Jeff,
>
> <snip>
>
>> Subject: [PATCH v12 7/7] testpmd: use hot-unplug failure handle mechanism
> ./devtools/check-git-log.sh -1
> Wrong headline label:
>          testpmd: use hot-unplug failure handle mechanism
>   


ok, let me check it.


>> This patch use testpmd for example, to show how an app smoothly handle
>> failure when device be hot-unplug. Except app should enabled the device event
>> monitor and register the hotplug event’s callback, it also need enable hotplug
>> handle mechanism before running. Once app detect the removal event, the hot-
>> unplug callback would be called. It will first stop the packet forwarding, then
>> stop the port, close the port, and finally detach the port to clean the device and
>> release the resources.
>>
>> Signed-off-by: Jeff Guo <jia.guo@intel.com>
>> ---
>> v12->v11:
>> no change.
>> ---
>>   app/test-pmd/testpmd.c | 39 +++++++++++++++++++++++++++++++--------
>>   1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>> 001f0e5..bfef483 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -2093,14 +2093,22 @@ pmd_test_exit(void)
>>
>>   	if (hot_plug) {
>>   		ret = rte_dev_event_monitor_stop();
>> -		if (ret)
>> +		if (ret) {
>>   			RTE_LOG(ERR, EAL,
>>   				"fail to stop device event monitor.");
>> +			return;
>> +		}
>>
>>   		ret = eth_dev_event_callback_unregister();
>>   		if (ret)
> Should there be an RTE_LOG() call here?


ok, in order to make it more clean, no need to add help here.


>> +			return;
>> +
>> +		ret = rte_dev_hotplug_handle_disable();
>> +		if (ret) {
>>   			RTE_LOG(ERR, EAL,
>> -				"fail to unregister all event callbacks.");
>> +				"fail to disable hotplug handling.");
>> +			return;
>> +		}
>>   	}
>>
>>   	printf("\nBye...\n");
>> @@ -2244,6 +2252,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);
>> @@ -2254,9 +2265,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);
> It would be better to use an RTE_LOG() call here instead of printf().


ok.


>> +			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", @@ -
>> 2779,14 +2793,23 @@ main(int argc, char** argv)
>>   	init_config();
>>
>>   	if (hot_plug) {
>> -		/* enable hot plug monitoring */
>> +		ret = rte_dev_hotplug_handle_enable();
>> +		if (ret) {
>> +			RTE_LOG(ERR, EAL,
>> +				"fail to enable hotplug handling.");
>> +			return -1;
>> +		}
>> +
>>   		ret = rte_dev_event_monitor_start();
>>   		if (ret) {
>> -			rte_errno = EINVAL;
>> +			RTE_LOG(ERR, EAL,
>> +				"fail to start device event monitoring.");
>>   			return -1;
>>   		}
>> -		eth_dev_event_callback_register();
>>
>> +		ret = eth_dev_event_callback_register();
>> +		if (ret)
> Should there be an RTE_LOG() call here?


please see above answer.


>> +			return -1;
>>   	}
>>
>>   	if (start_port(RTE_PORT_ALL) != 0)
>> --
>> 2.7.4
> Regards,
>
> Bernard.
diff mbox series

Patch

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 001f0e5..bfef483 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2093,14 +2093,22 @@  pmd_test_exit(void)
 
 	if (hot_plug) {
 		ret = rte_dev_event_monitor_stop();
-		if (ret)
+		if (ret) {
 			RTE_LOG(ERR, EAL,
 				"fail to stop device event monitor.");
+			return;
+		}
 
 		ret = eth_dev_event_callback_unregister();
 		if (ret)
+			return;
+
+		ret = rte_dev_hotplug_handle_disable();
+		if (ret) {
 			RTE_LOG(ERR, EAL,
-				"fail to unregister all event callbacks.");
+				"fail to disable hotplug handling.");
+			return;
+		}
 	}
 
 	printf("\nBye...\n");
@@ -2244,6 +2252,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);
@@ -2254,9 +2265,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",
@@ -2779,14 +2793,23 @@  main(int argc, char** argv)
 	init_config();
 
 	if (hot_plug) {
-		/* enable hot plug monitoring */
+		ret = rte_dev_hotplug_handle_enable();
+		if (ret) {
+			RTE_LOG(ERR, EAL,
+				"fail to enable hotplug handling.");
+			return -1;
+		}
+
 		ret = rte_dev_event_monitor_start();
 		if (ret) {
-			rte_errno = EINVAL;
+			RTE_LOG(ERR, EAL,
+				"fail to start device event monitoring.");
 			return -1;
 		}
-		eth_dev_event_callback_register();
 
+		ret = eth_dev_event_callback_register();
+		if (ret)
+			return -1;
 	}
 
 	if (start_port(RTE_PORT_ALL) != 0)