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

Guo, Jia jia.guo at intel.com
Mon Apr 2 13:31:46 CEST 2018


hi,jingjing


On 4/2/2018 1:49 PM, Wu, Jingjing wrote:
>> +static int
>> +eth_dev_event_callback_register(portid_t port_id)
>> +{
>> +	int diag;
>> +	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) {
>> +			free(device_name);
> If device_name is NULL, no memory allocated, why free?
you are right.
>> +			return -1;
>> +		}
>> +	}
>> +	/* register the dev_event callback */
>> +	diag = rte_dev_event_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;
>> +	}
>> +
>> +	free(device_name);
>> +	return 0;
>> +}
>> +
>> +
>> +static int
>> +eth_dev_event_callback_unregister(portid_t port_id)
>> +{
>> +	int diag;
>> +	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) {
>> +			free(device_name);
> Same as above.
>
>> +			return -1;
>> +		}
>> +	}
>> +
>> +	/* unregister the dev_event callback */
>> +	diag = rte_dev_event_callback_unregister(device_name,
>> +		eth_dev_event_callback, (void *)(intptr_t)port_id);
>> +	if (diag) {
>> +		printf("Failed to setup dev_event callback\n");
>> +		return -1;
>> +	}
>> +
>> +	free(device_name);
>> +	return 0;
>> +}
>> +
>>   void
>>   attach_port(char *identifier)
>>   {
>> @@ -1869,6 +1941,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)
>> @@ -1880,6 +1954,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 +1986,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");
>> @@ -1916,6 +2002,7 @@ void
>>   pmd_test_exit(void)
>>   {
>>   	portid_t pt_id;
>> +	int ret;
>>
>>   	if (test_done == 0)
>>   		stop_packet_forwarding();
>> @@ -1929,6 +2016,19 @@ pmd_test_exit(void)
>>   			close_port(pt_id);
>>   		}
>>   	}
>> +
> Need to check if hot_plug is enabled?
sure.
>> +	ret = rte_dev_event_monitor_stop();
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL, "fail to stop device event monitor.");
>> +		return;
>> +	}
>> +
>> +	ret = eth_dev_event_callback_unregister(HOT_PLUG_FOR_ALL_DEVICE);
>> +	if (ret) {
>> +		RTE_LOG(ERR, EAL, "fail to unregister all event callbacks.");
>> +		return;
>> +	}
>
> <...>
>
>> +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))
> Remove "!" in the check
the hot plug list is for hot plug in and hot plug out device, that is 
management by app, when remove a device will add into the hotplug list 
for the future adding.
>> +		return;
>> +
>> +	RTE_LOG(ERR, EAL, "add device: %s\n", dev_name);
> It is not ERR, please make the log aligned with remove device.
yes.
>> +	attach_port(dev_name);
>> +}
>> +
> <...>
>> +
>> +/* 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_ADD] = "add",
>> +		[RTE_DEV_EVENT_REMOVE] = "remove",
>> +	};
>> +	char *dev_name = malloc(strlen(device_name) + 1);
>> +
>> +	strcpy(dev_name, device_name);
> Why not use strdup as above?
ok.
>> +	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))
>> +			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");
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +	return 0;
> Always 0, even alarm set fails?
>
should check the alarm fails.
> Thanks
> Jingjing



More information about the dev mailing list