From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id E1ECCA499 for ; Thu, 25 Jan 2018 15:57:50 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Jan 2018 06:57:49 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,412,1511856000"; d="scan'208";a="12966603" Received: from tianfeiz-mobl2.ccr.corp.intel.com (HELO [10.255.28.200]) ([10.255.28.200]) by fmsmga007.fm.intel.com with ESMTP; 25 Jan 2018 06:57:47 -0800 To: "Wu, Jingjing" , "stephen@networkplumber.org" , "Richardson, Bruce" , "Yigit, Ferruh" , "gaetan.rivet@6wind.com" References: <1516013331-18939-3-git-send-email-jia.guo@intel.com> <1516248723-16985-1-git-send-email-jia.guo@intel.com> <9BB6961774997848B5B42BEC655768F810F17FAB@SHSMSX103.ccr.corp.intel.com> Cc: "Ananyev, Konstantin" , "jblunck@infradead.org" , "shreyansh.jain@nxp.com" , "dev@dpdk.org" , "thomas@monjalon.net" , "Zhang, Helin" , "motih@mellanox.com" From: "Guo, Jia" Message-ID: <13279f2f-949d-2443-4fc7-424272edccd2@intel.com> Date: Thu, 25 Jan 2018 22:57:47 +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: <9BB6961774997848B5B42BEC655768F810F17FAB@SHSMSX103.ccr.corp.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH V12 1/3] eal: add uevent monitor api and callback func 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: Thu, 25 Jan 2018 14:57:51 -0000 thanks for your review. please check v13. On 1/24/2018 10:52 PM, Wu, Jingjing wrote: > >> -----Original Message----- >> From: Guo, Jia >> Sent: Thursday, January 18, 2018 12:12 PM >> To: stephen@networkplumber.org; Richardson, Bruce ; >> Yigit, Ferruh ; gaetan.rivet@6wind.com >> Cc: Ananyev, Konstantin ; jblunck@infradead.org; >> shreyansh.jain@nxp.com; Wu, Jingjing ; dev@dpdk.org; Guo, Jia >> ; thomas@monjalon.net; Zhang, Helin ; >> motih@mellanox.com >> Subject: [PATCH V12 1/3] eal: add uevent monitor api and callback func >> >> This patch aim to add a general uevent mechanism in eal device layer, >> to enable all linux kernel object uevent monitoring, user could use these >> APIs to monitor and read out the device status info that sent from the >> kernel side, then corresponding to handle it, such as when detect hotplug >> uevent type, user could detach or attach the device, and more it benefit >> to use to do smoothly fail safe work. >> >> About uevent monitoring: >> a: add one epolling to poll the netlink socket, to monitor the uevent of >> the device. >> b: add enum of rte_eal_dev_event_type and struct of rte_eal_uevent. >> c: add below APIs in rte eal device layer. >> rte_dev_callback_register >> rte_dev_callback_unregister >> _rte_dev_callback_process >> rte_dev_event_monitor_start >> rte_dev_event_monitor_stop >> >> Signed-off-by: Jeff Guo >> --- >> v12->v11: >> identify null param in callback for monitor all devices uevent >> --- >> lib/librte_eal/bsdapp/eal/eal_dev.c | 38 ++++++ >> lib/librte_eal/common/eal_common_dev.c | 128 ++++++++++++++++++ >> lib/librte_eal/common/include/rte_dev.h | 119 +++++++++++++++++ >> lib/librte_eal/linuxapp/eal/Makefile | 1 + >> lib/librte_eal/linuxapp/eal/eal_dev.c | 223 ++++++++++++++++++++++++++++++++ >> 5 files changed, 509 insertions(+) >> create mode 100644 lib/librte_eal/bsdapp/eal/eal_dev.c >> create mode 100644 lib/librte_eal/linuxapp/eal/eal_dev.c >> > [......] > >> +int >> +rte_dev_callback_register(char *device_name, rte_dev_event_cb_fn cb_fn, >> + void *cb_arg) >> +{ >> + struct rte_dev_event_callback *event_cb = NULL; >> + >> + rte_spinlock_lock(&rte_dev_event_lock); >> + >> + if (TAILQ_EMPTY(&(dev_event_cbs))) >> + TAILQ_INIT(&(dev_event_cbs)); >> + >> + TAILQ_FOREACH(event_cb, &(dev_event_cbs), next) { >> + if (event_cb->cb_fn == cb_fn && >> + event_cb->cb_arg == cb_arg && >> + !strcmp(event_cb->dev_name, device_name)) > device_name = NULL means means for all devices, right? Can strcmp accept NULL arguments? got it. >> + break; >> + } >> + >> + /* create a new callback. */ >> + if (event_cb == NULL) { >> + /* allocate a new user callback entity */ >> + event_cb = malloc(sizeof(struct rte_dev_event_callback)); >> + if (event_cb != NULL) { >> + event_cb->cb_fn = cb_fn; >> + event_cb->cb_arg = cb_arg; >> + event_cb->dev_name = device_name; >> + } > Is that OK to call TAILQ_INSERT_TAIL below if event_cb == NULL? yes, that might be wrong. >> + TAILQ_INSERT_TAIL(&(dev_event_cbs), event_cb, next); >> + } >> + >> + rte_spinlock_unlock(&rte_dev_event_lock); >> + return (event_cb == NULL) ? -1 : 0; >> +} >> + >> +int >> +rte_dev_callback_unregister(char *device_name, rte_dev_event_cb_fn cb_fn, >> + void *cb_arg) >> +{ >> + int ret; >> + struct rte_dev_event_callback *event_cb, *next; >> + >> + if (!cb_fn || device_name == NULL) >> + return -EINVAL; >> + >> + rte_spinlock_lock(&rte_dev_event_lock); >> + >> + ret = 0; >> + >> + for (event_cb = TAILQ_FIRST(&(dev_event_cbs)); event_cb != NULL; >> + event_cb = next) { >> + >> + next = TAILQ_NEXT(event_cb, next); >> + >> + if (event_cb->cb_fn != cb_fn || >> + (event_cb->cb_arg != (void *)-1 && >> + event_cb->cb_arg != cb_arg) || >> + strcmp(event_cb->dev_name, device_name)) > The same comments as above. ok. >> + continue; >> + >> + /* >> + * if this callback is not executing right now, >> + * then remove it. >> + */ >> + if (event_cb->active == 0) { >> + TAILQ_REMOVE(&(dev_event_cbs), event_cb, next); >> + rte_free(event_cb); >> + } else { >> + ret = -EAGAIN; >> + } >> + } >> + >> + rte_spinlock_unlock(&rte_dev_event_lock); >> + return ret; >> +} >> + > [......] > >> +int >> +rte_dev_event_monitor_start(void) >> +{ >> + int ret; >> + struct rte_service_spec service; >> + uint32_t id; >> + const uint32_t sid = 0; >> + >> + if (!service_no_init) >> + return 0; >> + >> + uint32_t slcore_1 = rte_get_next_lcore(/* start core */ -1, >> + /* skip master */ 1, >> + /* wrap */ 0); >> + >> + ret = rte_service_lcore_add(slcore_1); >> + if (ret) { >> + RTE_LOG(ERR, EAL, "dev event monitor lcore add fail"); >> + return ret; >> + } >> + >> + memset(&service, 0, sizeof(service)); >> + snprintf(service.name, sizeof(service.name), DEV_EV_MNT_SERVICE_NAME); >> + >> + service.socket_id = rte_socket_id(); >> + service.callback = dev_uev_monitoring; >> + service.callback_userdata = NULL; >> + service.capabilities = 0; >> + ret = rte_service_component_register(&service, &id); >> + if (ret) { >> + RTE_LOG(ERR, EAL, "Failed to register service %s " >> + "err = %" PRId32, >> + service.name, ret); >> + return ret; >> + } >> + ret = rte_service_runstate_set(sid, 1); >> + if (ret) { >> + RTE_LOG(ERR, EAL, "Failed to set the runstate of " >> + "the service"); > Any rollback need to be done when fails? yes, should be handle fails. >> + return ret; >> + } >> + ret = rte_service_component_runstate_set(id, 1); >> + if (ret) { >> + RTE_LOG(ERR, EAL, "Failed to set the backend runstate" >> + " of a component"); >> + return ret; >> + } >> + ret = rte_service_map_lcore_set(sid, slcore_1, 1); >> + if (ret) { >> + RTE_LOG(ERR, EAL, "Failed to enable lcore 1 on " >> + "dev event monitor service"); >> + return ret; >> + } >> + rte_service_lcore_start(slcore_1); >> + service_no_init = false; >> + return 0; >> +} >> + >> +int >> +rte_dev_event_monitor_stop(void) >> +{ >> + service_exit = true; >> + service_no_init = true; >> + return 0; > Are start and stop peer functions to call? If we call rte_dev_event_monitor_start to start monitor and then call rte_dev_event_monitor_stop to stop it, and then how to start again? sure. should peer control. >> +} >> -- >> 2.7.4