[dpdk-dev] [PATCH V16 4/4] app/testpmd: enable device hotplug monitoring

[Tan, Jianfeng]

On 3/26/2018 7:20 PM, Jeff Guo wrote:
> Use testpmd for example, to show an application how to use device event
> mechanism to monitor the hotplug event, involve both hot removal event

involve -> including

> and the hot insertion event.
>
> The process is that, testpmd first enable hotplug monitoring and register
> the user's callback, when device being hotplug insertion or hotplug
> removal, the eal monitor the event and call user's callbacks, the
> application according their hot plug policy to detach or attach the device
> from the bus.

You are not exactly describing what's done in this example. From what I 
see, you only implement hot-unplug. For hot-plug, we will need to 
register callback with dev_name of NULL.

And without the other patch series, this only works without starting 
datapath.


>
> Signed-off-by: Jeff Guo <jia.guo at intel.com>
> ---
> 1.modify log and patch description.
> ---
>   app/test-pmd/parameters.c |   5 +-
>   app/test-pmd/testpmd.c    | 195 +++++++++++++++++++++++++++++++++++++++++++++-
>   app/test-pmd/testpmd.h    |  11 +++
>   3 files changed, 209 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index 97d22b8..825d602 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -186,6 +186,7 @@ usage(char* progname)
>   	printf("  --flow-isolate-all: "
>   	       "requests flow API isolated mode on all ports at initialization time.\n");
>   	printf("  --tx-offloads=0xXXXXXXXX: hexadecimal bitmask of TX queue offloads\n");
> +	printf("  --hot-plug: enalbe hot plug for device.\n");
>   }
>   
>   #ifdef RTE_LIBRTE_CMDLINE
> @@ -621,6 +622,7 @@ launch_args_parse(int argc, char** argv)
>   		{ "print-event",		1, 0, 0 },
>   		{ "mask-event",			1, 0, 0 },
>   		{ "tx-offloads",		1, 0, 0 },
> +		{ "hot-plug",			0, 0, 0 },
>   		{ 0, 0, 0, 0 },
>   	};
>   
> @@ -1102,7 +1104,8 @@ launch_args_parse(int argc, char** argv)
>   					rte_exit(EXIT_FAILURE,
>   						 "invalid mask-event argument\n");
>   				}
> -
> +			if (!strcmp(lgopts[opt_idx].name, "hot-plug"))
> +				hot_plug = 1;
>   			break;
>   		case 'h':
>   			usage(argv[0]);
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 4c0e258..bb1ac8f 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -12,6 +12,7 @@
>   #include <sys/mman.h>
>   #include <sys/types.h>
>   #include <errno.h>
> +#include <stdbool.h>
>   
>   #include <sys/queue.h>
>   #include <sys/stat.h>
> @@ -284,6 +285,9 @@ uint8_t lsc_interrupt = 1; /* enabled by default */
>    */
>   uint8_t rmv_interrupt = 1; /* enabled by default */
>   
> +
> +uint8_t hot_plug = 0; /**< hotplug disabled by default. */
> +
>   /*
>    * Display or mask ether events
>    * Default to all events except VF_MBOX
> @@ -384,6 +388,8 @@ uint8_t bitrate_enabled;
>   struct gro_status gro_ports[RTE_MAX_ETHPORTS];
>   uint8_t gro_flush_cycles = GRO_DEFAULT_FLUSH_CYCLES;
>   
> +static struct hotplug_request_list hp_list;
> +
>   /* Forward function declarations */
>   static void map_port_queue_stats_mapping_registers(portid_t pi,
>   						   struct rte_port *port);
> @@ -391,6 +397,14 @@ static void check_all_ports_link_status(uint32_t port_mask);
>   static int eth_event_callback(portid_t port_id,
>   			      enum rte_eth_event_type type,
>   			      void *param, void *ret_param);
> +static int eth_dev_event_callback(char *device_name,
> +				enum rte_dev_event_type type,
> +				void *param);
> +static int eth_dev_event_callback_register(portid_t port_id);
> +static bool in_hotplug_list(const char *dev_name);
> +
> +static int hotplug_list_add(struct rte_device *device,
> +				enum rte_kernel_driver device_kdrv);
>   
>   /*
>    * Check if all the ports are started.
> @@ -1853,6 +1867,27 @@ reset_port(portid_t pid)
>   	printf("Done\n");
>   }
>   
> +static int
> +eth_dev_event_callback_register(portid_t port_id)
> +{
> +	int diag;
> +	char device_name[128];
> +
> +	snprintf(device_name, sizeof(device_name),
> +		"%s", rte_eth_devices[port_id].device->name);
> +
> +	/* register the dev_event callback */
> +
> +	diag = rte_dev_callback_register(device_name,
> +		eth_dev_event_callback, (void *)(intptr_t)port_id);
> +	if (diag) {
> +		printf("Failed to setup dev_event callback\n");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>   void
>   attach_port(char *identifier)
>   {
> @@ -1869,6 +1904,8 @@ attach_port(char *identifier)
>   	if (rte_eth_dev_attach(identifier, &pi))
>   		return;
>   
> +	eth_dev_event_callback_register(pi);

What's the difference with below one?

> +
>   	socket_id = (unsigned)rte_eth_dev_socket_id(pi);
>   	/* if socket_id is invalid, set to 0 */
>   	if (check_socket_id(socket_id) < 0)
> @@ -1880,6 +1917,12 @@ attach_port(char *identifier)
>   
>   	ports[pi].port_status = RTE_PORT_STOPPED;
>   
> +	if (hot_plug) {
> +		hotplug_list_add(rte_eth_devices[pi].device,
> +				 rte_eth_devices[pi].data->kdrv);
> +		eth_dev_event_callback_register(pi);
> +	}
> +
>   	printf("Port %d is attached. Now total ports is %d\n", pi, nb_ports);
>   	printf("Done\n");
>   }
> @@ -1906,6 +1949,12 @@ detach_port(portid_t port_id)
>   
>   	nb_ports = rte_eth_dev_count();
>   
> +	if (hot_plug) {
> +		hotplug_list_add(rte_eth_devices[port_id].device,
> +				 rte_eth_devices[port_id].data->kdrv);
> +		eth_dev_event_callback_register(port_id);
> +	}
> +
>   	printf("Port '%s' is detached. Now total ports is %d\n",
>   			name, nb_ports);
>   	printf("Done\n");
> @@ -1929,6 +1978,9 @@ pmd_test_exit(void)
>   			close_port(pt_id);
>   		}
>   	}
> +
> +	rte_dev_event_monitor_stop();
> +
>   	printf("\nBye...\n");
>   }
>   
> @@ -2013,6 +2065,49 @@ rmv_event_callback(void *arg)
>   			dev->device->name);
>   }
>   
> +static void
> +rmv_dev_event_callback(void *arg)
> +{
> +	char name[RTE_ETH_NAME_MAX_LEN];
> +	uint8_t port_id = (intptr_t)arg;
> +
> +	rte_eal_alarm_cancel(rmv_dev_event_callback, arg);
> +
> +	RTE_ETH_VALID_PORTID_OR_RET(port_id);
> +	printf("removing port id:%u\n", port_id);
> +
> +	if (!in_hotplug_list(rte_eth_devices[port_id].device->name))
> +		return;
> +
> +	stop_packet_forwarding();
> +
> +	stop_port(port_id);
> +	close_port(port_id);
> +	if (rte_eth_dev_detach(port_id, name)) {
> +		RTE_LOG(ERR, USER1, "Failed to detach port '%s'\n", name);
> +		return;
> +	}
> +
> +	nb_ports = rte_eth_dev_count();
> +
> +	printf("Port '%s' is detached. Now total ports is %d\n",
> +			name, nb_ports);
> +}
> +
> +static void
> +add_dev_event_callback(void *arg)
> +{
> +	char *dev_name = (char *)arg;
> +
> +	rte_eal_alarm_cancel(add_dev_event_callback, arg);
> +
> +	if (!in_hotplug_list(dev_name))
> +		return;
> +
> +	RTE_LOG(ERR, EAL, "add device: %s\n", dev_name);
> +	attach_port(dev_name);
> +}
> +
>   /* This function is used by the interrupt thread */
>   static int
>   eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
> @@ -2059,6 +2154,86 @@ eth_event_callback(portid_t port_id, enum rte_eth_event_type type, void *param,
>   	return 0;
>   }
>   
> +static bool
> +in_hotplug_list(const char *dev_name)
> +{
> +	struct hotplug_request *hp_request = NULL;
> +
> +	TAILQ_FOREACH(hp_request, &hp_list, next) {
> +		if (!strcmp(hp_request->dev_name, dev_name))
> +			break;
> +	}
> +
> +	if (hp_request)
> +		return true;
> +
> +	return false;
> +}
> +
> +static int
> +hotplug_list_add(struct rte_device *device, enum rte_kernel_driver device_kdrv)
> +{
> +	struct hotplug_request *hp_request;
> +
> +	hp_request = rte_zmalloc("hoplug request",
> +			sizeof(*hp_request), 0);
> +	if (hp_request == NULL) {
> +		fprintf(stderr, "%s can not alloc memory\n",
> +			__func__);
> +		return -ENOMEM;
> +	}
> +
> +	hp_request->dev_name = device->name;
> +	hp_request->dev_kdrv = device_kdrv;
> +
> +	TAILQ_INSERT_TAIL(&hp_list, hp_request, next);
> +
> +	return 0;
> +}
> +
> +/* This function is used by the interrupt thread */
> +static int
> +eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
> +			     void *arg)
> +{
> +	static const char * const event_desc[] = {
> +		[RTE_DEV_EVENT_UNKNOWN] = "Unknown",
> +		[RTE_DEV_EVENT_ADD] = "add",
> +		[RTE_DEV_EVENT_REMOVE] = "remove",
> +	};
> +	char *dev_name = malloc(strlen(device_name) + 1);
> +
> +	strcpy(dev_name, device_name);

strdup is easier?

> +
> +	if (type >= RTE_DEV_EVENT_MAX) {
> +		fprintf(stderr, "%s called upon invalid event %d\n",
> +			__func__, type);
> +		fflush(stderr);
> +	} else if (event_print_mask & (UINT32_C(1) << type)) {
> +		printf("%s event\n",
> +			event_desc[type]);
> +		fflush(stdout);
> +	}
> +
> +	switch (type) {
> +	case RTE_DEV_EVENT_REMOVE:
> +		if (rte_eal_alarm_set(100000,
> +			rmv_dev_event_callback, arg))

Why not rmv_dev_event_callback directly?

Besides, the name of rmv_dev_event_callback is really confusing. It's 
not a dev event callback.

> +			fprintf(stderr,
> +				"Could not set up deferred device removal\n");
> +		break;
> +	case RTE_DEV_EVENT_ADD:
> +		if (rte_eal_alarm_set(100000,
> +			add_dev_event_callback, dev_name))
> +			fprintf(stderr,
> +				"Could not set up deferred device add\n");

Ditto.

> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
>   static int
>   set_tx_queue_stats_mapping_registers(portid_t port_id, struct rte_port *port)
>   {
> @@ -2474,8 +2649,9 @@ signal_handler(int signum)
>   int
>   main(int argc, char** argv)
>   {
> -	int  diag;
> +	int diag;
>   	portid_t port_id;
> +	int ret;
>   
>   	signal(SIGINT, signal_handler);
>   	signal(SIGTERM, signal_handler);
> @@ -2543,6 +2719,23 @@ main(int argc, char** argv)
>   		       nb_rxq, nb_txq);
>   
>   	init_config();
> +
> +	if (hot_plug) {
> +		/* enable hot plug monitoring */
> +		ret = rte_dev_event_monitor_start();
> +		if (ret) {
> +			rte_errno = EINVAL;
> +			return -1;
> +		}
> +		if (TAILQ_EMPTY(&hp_list))
> +			TAILQ_INIT(&hp_list);
> +		RTE_ETH_FOREACH_DEV(port_id) {
> +			hotplug_list_add(rte_eth_devices[port_id].device,
> +					 rte_eth_devices[port_id].data->kdrv);
> +			eth_dev_event_callback_register(port_id);
> +		}

Why not monitoring all devices with dev_name of NULL? It makes things 
much easier. And we can also monitor the hot-plug event.

> +	}
> +
>   	if (start_port(RTE_PORT_ALL) != 0)
>   		rte_exit(EXIT_FAILURE, "Start ports failed\n");
>   
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 153abea..c619e11 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -63,6 +63,15 @@ typedef uint16_t streamid_t;
>   #define TM_MODE			0
>   #endif
>   
> +struct hotplug_request {
> +	TAILQ_ENTRY(hotplug_request) next; /**< Callbacks list */
> +	const char *dev_name;            /* request device name */
> +	enum rte_kernel_driver dev_kdrv;            /* kernel driver binded */
> +};
> +
> +/** @internal Structure to keep track of registered callbacks */
> +TAILQ_HEAD(hotplug_request_list, hotplug_request);
> +
>   enum {
>   	PORT_TOPOLOGY_PAIRED,
>   	PORT_TOPOLOGY_CHAINED,
> @@ -319,6 +328,8 @@ extern volatile int test_done; /* stop packet forwarding when set to 1. */
>   extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
>   extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter */
>   extern uint32_t event_print_mask;
> +extern uint8_t hot_plug; /**< enable by "--hot-plug" parameter */
> +
>   /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
>   
>   #ifdef RTE_LIBRTE_IXGBE_BYPASS