Re: [dpdk-dev] [PATCH V19 4/4] app/testpmd: use auto handle for hotplug

[Matan Azrad <matan@mellanox.com>]

Hi All

From: Jeff Guo, Friday, April 6, 2018 1:57 PM
> Use testpmd for example, to show how an application smoothly handle
> failure when device be hot removal, and show how to auto bind kernal driver
> to preparing attach device when device being hot insertion.

I saw the kmod of old devices were saved by name and the app hope that new device name will be as previous saved name.
How can the application know which driver should be bind for any new device?

Moreover, I think that the application should not deal with driver binding\remap and all this should be managed by the relevant PMDs - 
Looks like the bind mechanism should be managed directly by the PMDs and not annoying application to deal with it.

Am I missing something?

> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v19->v18:
> clean code
> ---
>  app/test-pmd/testpmd.c | 199
> +++++++++++++++++++++++++++++++++++++++++++------
>  app/test-pmd/testpmd.h |   9 +++
>  2 files changed, 184 insertions(+), 24 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> d2c122a..d7fa913 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -285,6 +285,8 @@ uint8_t lsc_interrupt = 1; /* enabled by default */
>   */
>  uint8_t rmv_interrupt = 1; /* enabled by default */
> 
> +#define HOT_PLUG_FOR_ALL_DEVICE -1
> +#define ALL_CALLBACK -1
>  uint8_t hot_plug = 0; /**< hotplug disabled by default. */
> 
>  /*
> @@ -387,6 +389,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);
> @@ -397,9 +401,13 @@ static int eth_event_callback(portid_t port_id,  static
> void eth_dev_event_callback(char *device_name,
>  				enum rte_dev_event_type type,
>  				void *param);
> -static int eth_dev_event_callback_register(void);
> -static int eth_dev_event_callback_unregister(void);
> +static int eth_dev_event_callback_register(portid_t port_id); static
> +int eth_dev_event_callback_unregister(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.
> @@ -1120,11 +1128,17 @@ run_pkt_fwd_on_lcore(struct fwd_lcore *fc,
> packet_fwd_t pkt_fwd)
>  	uint64_t tics_datum;
>  	uint64_t tics_current;
>  	uint8_t idx_port, cnt_ports;
> +	int ret;
> 
>  	cnt_ports = rte_eth_dev_count();
>  	tics_datum = rte_rdtsc();
>  	tics_per_1sec = rte_get_timer_hz();
>  #endif
> +	if (hot_plug) {
> +		ret = rte_dev_handle_hot_unplug();
> +		if (ret)
> +			printf("The device is being hot unplug!\n");
> +	}
>  	fsm = &fwd_streams[fc->stream_idx];
>  	nb_fs = fc->stream_nb;
>  	do {
> @@ -1863,15 +1877,24 @@ reset_port(portid_t pid)  }
> 
>  static int
> -eth_dev_event_callback_register(void)
> +eth_dev_event_callback_register(portid_t port_id)
>  {
>  	int ret;
> +	char *device_name;
> 
> +	/* if port id equal -1, unregister event callbacks for all device. */

unregister => register
And I think that HOT_PLUG_FOR_ALL_DEVICE is descriptive enough.


> +	if (port_id == (portid_t)HOT_PLUG_FOR_ALL_DEVICE) {
> +		device_name = NULL;
> +	} else {
> +		device_name = strdup(rte_eth_devices[port_id].device-
> >name);
> +		if (!device_name)
> +			return -1;
> +	}
>  	/* register the device event callback */
> -	ret = rte_dev_event_callback_register(NULL,
> -		eth_dev_event_callback, NULL);
> +	ret = rte_dev_event_callback_register(device_name,
> +		eth_dev_event_callback, (void *)(intptr_t)port_id);
>  	if (ret) {
> -		printf("Failed to register device event callback\n");
> +		printf("Failed to register device event callback.\n");
>  		return -1;
>  	}

You missed device_name freeing.

> @@ -1880,15 +1903,25 @@ eth_dev_event_callback_register(void)
> 
> 
>  static int
> -eth_dev_event_callback_unregister(void)
> +eth_dev_event_callback_unregister(portid_t port_id)
>  {
>  	int ret;
> +	char *device_name;
> +
> +	/* if port id equal -1, unregister all device event callbacks */
> +	if (port_id == (portid_t)HOT_PLUG_FOR_ALL_DEVICE) {
> +		device_name = NULL;
> +	} else {
> +		device_name = strdup(rte_eth_devices[port_id].device-
> >name);
> +		if (!device_name)
> +			return -1;
> +	}
> 
>  	/* unregister the device event callback */
> -	ret = rte_dev_event_callback_unregister(NULL,
> -		eth_dev_event_callback, NULL);
> -	if (ret < 0) {
> -		printf("Failed to unregister device event callback\n");
> +	ret = rte_dev_event_callback_unregister(device_name,
> +		eth_dev_event_callback, (void *)(intptr_t)port_id);

Also here.

> +	if (ret) {
> +		printf("Failed to unregister device event callback.\n");
>  		return -1;
>  	}
> 
> @@ -1911,6 +1944,8 @@ attach_port(char *identifier)
>  	if (rte_eth_dev_attach(identifier, &pi))
>  		return;
> 
> +	eth_dev_event_callback_register(pi);
> +
>  	socket_id = (unsigned)rte_eth_dev_socket_id(pi);
>  	/* if socket_id is invalid, set to 0 */
>  	if (check_socket_id(socket_id) < 0)
> @@ -1922,6 +1957,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");
>  }
> @@ -1933,6 +1974,12 @@ detach_port(portid_t port_id)
> 
>  	printf("Detaching a port...\n");
> 
> +	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);
> +	}
> +
>  	if (!port_is_closed(port_id)) {
>  		printf("Please close port first\n");
>  		return;
> @@ -1979,7 +2026,7 @@ pmd_test_exit(void)
>  			RTE_LOG(ERR, EAL,
>  				"fail to stop device event monitor.");
> 
> -		ret = eth_dev_event_callback_unregister();
> +		ret = eth_dev_event_callback_unregister(ALL_CALLBACK);
>  		if (ret)
>  			RTE_LOG(ERR, EAL,
>  				"fail to unregister all event callbacks."); @@ -
> 2068,6 +2115,40 @@ rmv_event_callback(void *arg)
>  			dev->device->name);
>  }
> 
> +static void
> +rmv_dev_event_callback(void *arg)
> +{
> +	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);
> +	detach_port(port_id);

Did you check the synchronization with the RMV event callback of ethdev? 

> +}
> +
> +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, @@ -2114,31 +2195,96 @@ 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 enum rte_kernel_driver
> +get_hotplug_driver(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))
> +			return hp_request->dev_kdrv;
> +	}
> +	return -1;
> +}
> +
> +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 void
> eth_dev_event_callback(char *device_name, enum rte_dev_event_type
> type,
> -			     __rte_unused void *arg)
> +				void *arg)
>  {
> +	static const char * const event_desc[] = {
> +		[RTE_DEV_EVENT_ADD] = "add",
> +		[RTE_DEV_EVENT_REMOVE] = "remove",
> +	};
> +	char *dev_name = malloc(strlen(device_name) + 1);
> +
> +	strcpy(dev_name, device_name);
> +
>  	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);
>  	}
>

You need to use port_id_is_invalid() to check the port validity.
 
>  	switch (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.
> -		 */
> +		if (rte_eal_alarm_set(100000,
> +			rmv_dev_event_callback, arg))
> +			fprintf(stderr, "Could not set up deferred "
> +				"device removal\n");
>  		break;
>  	case RTE_DEV_EVENT_ADD:
> -		RTE_LOG(ERR, EAL, "The device: %s has been added!\n",
> -			device_name);
> -		/* TODO: After finish kernel driver binding,
> -		 * begin to attach port.
> +		/**
> +		 * bind the driver to the device
> +		 * before process of hot plug adding device
>  		 */
> +		rte_dev_bind_kernel_driver(dev_name,
> +					   get_hotplug_driver(dev_name));
> +		if (rte_eal_alarm_set(100000,
> +			add_dev_event_callback, dev_name))
> +			fprintf(stderr, "Could not set up deferred "
> +				"device add\n");
>  		break;
>  	default:
>  		break;
> @@ -2638,8 +2784,13 @@ main(int argc, char** argv)
>  			rte_errno = EINVAL;
>  			return -1;
>  		}
> -		eth_dev_event_callback_register();
> -
> +		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);
> +		}
>  	}
> 
>  	if (start_port(RTE_PORT_ALL) != 0)
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> 8fde68d..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,
> --
> 2.7.4