From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id C832B7CD6 for ; Mon, 2 Apr 2018 13:31:51 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Apr 2018 04:31:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,395,1517904000"; d="scan'208";a="47268376" Received: from jguo15x-mobl3.ccr.corp.intel.com (HELO [10.67.68.69]) ([10.67.68.69]) by orsmga002.jf.intel.com with ESMTP; 02 Apr 2018 04:31:46 -0700 To: "Wu, Jingjing" , "stephen@networkplumber.org" , "Richardson, Bruce" , "Yigit, Ferruh" , "Ananyev, Konstantin" , "gaetan.rivet@6wind.com" , "thomas@monjalon.net" , "motih@mellanox.com" , "Van Haaren, Harry" , "Tan, Jianfeng" References: <1522063256-3997-5-git-send-email-jia.guo@intel.com> <1522339205-27698-1-git-send-email-jia.guo@intel.com> <1522339205-27698-5-git-send-email-jia.guo@intel.com> <9BB6961774997848B5B42BEC655768F810FCF3B8@SHSMSX103.ccr.corp.intel.com> Cc: "jblunck@infradead.org" , "shreyansh.jain@nxp.com" , "dev@dpdk.org" , "Zhang, Helin" From: "Guo, Jia" Message-ID: <1ec357c1-cac9-3736-3aec-e68ec2906402@intel.com> Date: Mon, 2 Apr 2018 19:31:46 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <9BB6961774997848B5B42BEC655768F810FCF3B8@SHSMSX103.ccr.corp.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH V17 4/4] app/testpmd: enable device hotplug monitoring X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 02 Apr 2018 11:31:52 -0000 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