From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id C90EA199B0 for ; Tue, 9 Jan 2018 12:58:22 +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 fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jan 2018 03:58:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,335,1511856000"; d="scan'208,217";a="8287511" Received: from jguo15x-mobl3.ccr.corp.intel.com (HELO [10.67.68.66]) ([10.67.68.66]) by fmsmga007.fm.intel.com with ESMTP; 09 Jan 2018 03:58:19 -0800 To: Thomas Monjalon References: <1509567405-27439-3-git-send-email-jia.guo@intel.com> <2000997.W0TFPrnL2l@xps> <6b97ebc5-1ec5-a724-a620-96b23b126d01@intel.com> <1930193.dgY3bQfKM7@xps> Cc: dev@dpdk.org, stephen@networkplumber.org, bruce.richardson@intel.com, ferruh.yigit@intel.com, gaetan.rivet@6wind.com, konstantin.ananyev@intel.com, shreyansh.jain@nxp.com, jingjing.wu@intel.com, helin.zhang@intel.com, motih@mellanox.com, harry.van.haaren@intel.com From: "Guo, Jia" Message-ID: Date: Tue, 9 Jan 2018 19:58:18 +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: <1930193.dgY3bQfKM7@xps> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH v7 1/2] eal: add uevent monitor for hot plug 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: Tue, 09 Jan 2018 11:58:23 -0000 On 1/9/2018 7:38 PM, Thomas Monjalon wrote: > 09/01/2018 09:25, Guo, Jia: >> On 1/9/2018 8:39 AM, Thomas Monjalon wrote: >>>> +int >>>> +_rte_dev_callback_process(struct rte_device *device, >>>> + enum rte_eal_dev_event_type event, >>>> + void *cb_arg, void *ret_param) >>> cb_arg must be an opaque parameter which is registered with the >>> callback and passed later. No need as parameter of this function. >>> >>> ret_param is not needed at all. The kernel event will be just >>> translated as rte_eal_dev_event_type (rte_dev_event after rename). >> suggest hold one to let new param, such as device info, add by >> ret_param, so cb_arg have set when register and no use anymore, delete it. > Sorry I don't understand. Please rephrase. please see v8 part of it. i need ret_param to pass the device name by the call back to the user. >>>> --- a/lib/librte_eal/common/include/rte_bus.h >>>> +++ b/lib/librte_eal/common/include/rte_bus.h >>>> /** >>>> + * Device iterator to find a device on a bus. >>>> + * >>>> + * This function returns an rte_device if one of those held by the bus >>>> + * matches the data passed as parameter. >>>> + * >>>> + * If the comparison function returns zero this function should stop iterating >>>> + * over any more devices. To continue a search the device of a previous search >>>> + * can be passed via the start parameter. >>>> + * >>>> + * @param cmp >>>> + * the device name comparison function. >>>> + * >>>> + * @param data >>>> + * Data to compare each device against. >>>> + * >>>> + * @param start >>>> + * starting point for the iteration >>>> + * >>>> + * @return >>>> + * The first device matching the data, NULL if none exists. >>>> + */ >>>> +typedef struct rte_device * >>>> +(*rte_bus_find_device_by_name_t)(const struct rte_device *start, >>>> + rte_dev_cmp_name_t cmp, >>>> + const void *data); >>> Why is it needed? There is already rte_bus_find_device_t. >> because the rte_bus_find_device_t just find a device structure in the >> device list, but here need to find a device structure by device name >> which come from uevent info. > I don't understand how it is different? > Looking at the code, it is a copy/paste except it is dedicated > to name comparison. > You can remove rte_bus_find_device_by_name_t and provide a > comparison function which looks at name. i mean that if the device have been remove and then insertion, the device have not construct when just got the device name from the uevent massage, so this case could i use the original find device function? >>>> +int >>>> +rte_eal_dev_monitor_enable(void); >>> I suggest to drop this function which is just calling rte_dev_monitor_start. >> more discuss, i suggest keep on it , let rte_dev_monitor_start >> separately stay on the platform code and let user commonly call >> rte_eal_dev_monitor_enable. > Then you may need a disable function. > It will end up to be like start/stop. > I think it is just redundant. > > If kept, please rename it to rte_dev_event_monitor_enable. >>>> + /* create the host thread to wait/handle the uevent from kernel */ >>>> + ret = pthread_create(&uev_monitor_thread, NULL, >>>> + dev_uev_monitoring, NULL); >>>> + return ret; >>>> +} >>> I think you should use rte_service for thread management. >> thanks for your info, such a good mechanism to use that i even not know >> that before. i will study and use it. > OK, good. >