[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