From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 831311B00B for ; Tue, 9 Jan 2018 09:25:46 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jan 2018 00:25:44 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,334,1511856000"; d="scan'208,217";a="9790809" Received: from jguo15x-mobl3.ccr.corp.intel.com (HELO [10.67.68.66]) ([10.67.68.66]) by orsmga006.jf.intel.com with ESMTP; 09 Jan 2018 00:25:41 -0800 To: Thomas Monjalon References: <1509567405-27439-3-git-send-email-jia.guo@intel.com> <1514943736-5931-1-git-send-email-jia.guo@intel.com> <1514943736-5931-2-git-send-email-jia.guo@intel.com> <2000997.W0TFPrnL2l@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: <6b97ebc5-1ec5-a724-a620-96b23b126d01@intel.com> Date: Tue, 9 Jan 2018 16:25:41 +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: <2000997.W0TFPrnL2l@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 08:25:47 -0000 On 1/9/2018 8:39 AM, Thomas Monjalon wrote: > Hi Jeff, > > I am surprised that there is not a lot of review of these very > important patches. Maybe it is not easy to review. > Let's try to progress in the following days. > > This patch is really too big with a lot of concepts spread > in separate files, so it is difficult to properly review. > Please, try to split in several patches, bringing only one concept > per patch. > > At first, you can introduce the new events and the callback API. > The second patch (and the most important one) would be to bring > the uevent parsing for Linux (and void implementation for BSD). > Then you can add and explain some patches around PCI mapping. > > At last there is the kernel binding effort - this one will probably > be ignored for 18.02, because it is another huge topic. > Without bothering with kernel binding, we can at least remove a device, > get a notification, and eventually re-add it. It is a good first step. > Anyway your testpmd patch tests exactly this scenario (totally new > devices are not seen). i will separate it for you all to benefit for review. for kernel binding, i just let it automatically compare with the first time manually binding, and it is the part of he hot plug flow. so i suggest to review more about that if it is not side effect and workable, beg for keep on. > More comments below: > > 03/01/2018 02:42, Jeff Guo: >> --- /dev/null >> +++ b/lib/librte_eal/bsdapp/eal/eal_dev.c >> @@ -0,0 +1,64 @@ >> +/*- >> + * Copyright(c) 2010-2017 Intel Corporation. >> + * All rights reserved. >> + * >> + * Redistribution and use in source and binary forms, with or without >> + * modification, are permitted provided that the following conditions >> + * are met: > Please check how Intel Copyright and BSD license is newly formatted > with SPDX tag. > got it. >> --- /dev/null >> +++ b/lib/librte_eal/bsdapp/eal/include/exec-env/rte_dev.h >> +enum rte_eal_dev_event_type { >> + RTE_EAL_DEV_EVENT_UNKNOWN, /**< unknown event type */ >> + RTE_EAL_DEV_EVENT_ADD, /**< device adding event */ >> + RTE_EAL_DEV_EVENT_REMOVE, >> + /**< device removing event */ >> + RTE_EAL_DEV_EVENT_CHANGE, >> + /**< device status change event */ >> + RTE_EAL_DEV_EVENT_MOVE, /**< device sys path move event */ >> + RTE_EAL_DEV_EVENT_ONLINE, /**< device online event */ >> + RTE_EAL_DEV_EVENT_OFFLINE, /**< device offline event */ >> + RTE_EAL_DEV_EVENT_MAX /**< max value of this enum */ >> +}; > The comments are not useful. > Please better explain what is change, move, online, etc. > > The shorter prefix RTE_DEV is preferred over RTE_EAL_DEV. > > This file is full of definitions which must be common, not specific > to BSD or Linux. Please move it. will move it to the better place. >> +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. >> --- 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. >> +/** >> + * Implementation specific remap function which is responsible for remmaping >> + * devices on that bus from original share memory resource to a private memory >> + * resource for the sake of device has been removal. >> + * >> + * @param dev >> + * Device pointer that was returned by a previous call to find_device. >> + * >> + * @return >> + * 0 on success. >> + * !0 on error. >> + */ >> +typedef int (*rte_bus_remap_device_t)(struct rte_device *dev); > You need to better explain why this remap op is needed, > and when it is called exactly? sure. >> @@ -206,9 +265,13 @@ struct rte_bus { >> rte_bus_scan_t scan; /**< Scan for devices attached to bus */ >> rte_bus_probe_t probe; /**< Probe devices on bus */ >> rte_bus_find_device_t find_device; /**< Find a device on the bus */ >> + rte_bus_find_device_by_name_t find_device_by_name; >> + /**< Find a device on the bus */ >> rte_bus_plug_t plug; /**< Probe single device for drivers */ >> rte_bus_unplug_t unplug; /**< Remove single device from driver */ >> rte_bus_parse_t parse; /**< Parse a device name */ >> + rte_bus_remap_device_t remap_device; /**< remap a device */ >> + rte_bus_bind_driver_t bind_driver; /**< bind a driver for bus device */ >> struct rte_bus_conf conf; /**< Bus configuration */ >> rte_bus_get_iommu_class_t get_iommu_class; /**< Get iommu class */ >> }; > Every new op must be introduced in a separate patch > (if not completely removed). make sense. >> --- a/lib/librte_eal/common/include/rte_dev.h >> +++ b/lib/librte_eal/common/include/rte_dev.h >> +enum device_state { >> + DEVICE_UNDEFINED, >> + DEVICE_FAULT, >> + DEVICE_PARSED, >> + DEVICE_PROBED, >> +}; > These constants must prefixed with RTE_ > and documented with doxygen please. got it. >> +/** >> + * It enable the device event monitoring for a specific event. > This comment must be reworded. ok. >> + * >> + * @param none > useless thanks. >> + * @return >> + * - On success, zero. >> + * - On failure, a negative value. >> + */ >> +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. >> --- a/lib/librte_eal/linuxapp/eal/eal_alarm.c >> +++ b/lib/librte_eal/linuxapp/eal/eal_alarm.c >> @@ -259,6 +260,10 @@ rte_eal_alarm_cancel(rte_eal_alarm_callback cb_fn, void *cb_arg) >> } >> ap_prev = ap; >> } >> + >> + ret |= rte_intr_callback_unregister(&intr_handle, >> + eal_alarm_callback, NULL); >> + >> rte_spinlock_unlock(&alarm_list_lk); >> } while (executing != 0); > Looks to be unrelated. > If it is a fix, please do a separate patch. ok. >> --- /dev/null >> +++ b/lib/librte_eal/linuxapp/eal/eal_dev.c >> +int >> +rte_dev_monitor_start(void) > What about adding "event" in the name of this function? > rte_dev_event_monitor_start dev monitor sounds shock, agree. >> +{ >> + int ret; >> + >> + if (!no_request_thread) >> + return 0; >> + no_request_thread = false; >> + >> + /* 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. >> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c >> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c >> @@ -354,6 +354,12 @@ igbuio_pci_release(struct uio_info *info, struct inode *inode) >> struct rte_uio_pci_dev *udev = info->priv; >> struct pci_dev *dev = udev->pdev; >> >> + /* check if device have been remove before release */ >> + if ((&dev->dev.kobj)->state_remove_uevent_sent == 1) { >> + pr_info("The device have been removed\n"); >> + return -1; >> + } > This looks to be unrelated. Separate patch please. > got it. > End of first pass review. There are some basic requirements that other > maintainers (especially at Intel) could have reviewed earlier. > Let's try to improve it quickly for 18.02, thanks. > If we are short in time, we should at least focus on adding the > events/callback API and the Linux events implementation.