From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 28F9F2C57 for ; Thu, 11 Jan 2018 15:24:09 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Jan 2018 06:24:07 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,344,1511856000"; d="scan'208";a="20797729" Received: from jguo15x-mobl3.ccr.corp.intel.com (HELO [10.249.162.35]) ([10.249.162.35]) by fmsmga001.fm.intel.com with ESMTP; 11 Jan 2018 06:24:05 -0800 To: Thomas Monjalon References: <1515555037-9419-4-git-send-email-jia.guo@intel.com> <1515575544-2141-1-git-send-email-jia.guo@intel.com> <1515575544-2141-2-git-send-email-jia.guo@intel.com> <1763212.jpufmvHW6Q@xps> Cc: dev@dpdk.org, stephen@networkplumber.org, bruce.richardson@intel.com, ferruh.yigit@intel.com, gaetan.rivet@6wind.com, konstantin.ananyev@intel.com, jblunck@infradead.org, shreyansh.jain@nxp.com, jingjing.wu@intel.com, helin.zhang@intel.com, motih@mellanox.com From: "Guo, Jia" Message-ID: <707f361d-9ba0-c156-8819-7344bc4c2dc0@intel.com> Date: Thu, 11 Jan 2018 22:24:04 +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: <1763212.jpufmvHW6Q@xps> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH V9 1/5] 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, 11 Jan 2018 14:24:10 -0000 On 1/11/2018 9:43 AM, Thomas Monjalon wrote: > Hi, > > Thanks for splitting the patches. > I will review the first one today. Please see below. > > 10/01/2018 10:12, Jeff Guo: >> --- /dev/null >> +++ b/lib/librte_eal/bsdapp/eal/eal_dev.c >> +int >> +rte_dev_monitor_start(void) >> +{ >> + return -1; >> +} >> + >> +int >> +rte_dev_monitor_stop(void) >> +{ >> + return -1; >> +} > You should add a log to show it is not supported. ok. >> --- /dev/null >> +++ b/lib/librte_eal/bsdapp/eal/include/exec-env/rte_dev.h >> +#ifndef _RTE_DEV_H_ >> +#error "don't include this file directly, please include generic " >> +#endif > Why creating different rte_dev.h for BSD and Linux? > This is an API, it should be the same. if no need at this time, combine it to a file. >> +/** >> + * Start the device uevent monitoring. >> + * >> + * @param none >> + * @return >> + * - On success, zero. >> + * - On failure, a negative value. >> + */ >> +int >> +rte_dev_monitor_start(void); >> + >> +/** >> + * Stop the device uevent monitoring . >> + * >> + * @param none >> + * @return >> + * - On success, zero. >> + * - On failure, a negative value. >> + */ >> + >> +int >> +rte_dev_monitor_stop(void); >> --- a/lib/librte_eal/common/eal_common_dev.c >> +++ b/lib/librte_eal/common/eal_common_dev.c >> @@ -42,9 +42,32 @@ >> #include >> #include >> #include >> +#include >> +#include >> >> #include "eal_private.h" >> >> +/* spinlock for device callbacks */ >> +static rte_spinlock_t rte_dev_cb_lock = RTE_SPINLOCK_INITIALIZER; > Please rename to rte_dev_event_lock. > Let's use rte_dev_event_ prefix consistently. make consistently, agree. >> + * The user application callback description. >> + * >> + * It contains callback address to be registered by user application, >> + * the pointer to the parameters for callback, and the event type. >> + */ >> +struct rte_eal_dev_callback { > Rename to rte_dev_event? >> + TAILQ_ENTRY(rte_eal_dev_callback) next; /**< Callbacks list */ >> + rte_eal_dev_cb_fn cb_fn; /**< Callback address */ > Rename to rte_dev_event_callback? >> + void *cb_arg; /**< Parameter for callback */ > Comment should be about opaque context. >> + void *ret_param; /**< Return parameter */ >> + enum rte_dev_event_type event; /**< device event type */ >> + uint32_t active; /**< Callback is executing */ > Why active is needed? avoid the lock when unregistered callback. >> +}; >> + >> +/* A genaral callback for all new devices be added onto the bus */ >> +static struct rte_eal_dev_callback *dev_add_cb; > It should not be a different callback for new devices. > You must allow registering the callback for all and new devices. > Please look how it's done for ethdev: > https://dpdk.org/patch/32900/ the aim to use this special callback is because when new device add onto the bus, no device instance to store the callback. i saw ethdev solution, that is base on port but that would not make sense in rte device layer. so i try to abandon add callback in rte device, replace of add device name into callback , please see my v10 patch. >> +int >> +rte_dev_callback_register(struct rte_device *device, >> + enum rte_dev_event_type event, >> + rte_eal_dev_cb_fn cb_fn, void *cb_arg) >> +{ > Why passing an event type at registration? > I think the event processing dispatch must be done in the callback, > not at registration. make sense, just register all type for device ,and let eal to pass the event. >> + /* allocate a new interrupt callback entity */ >> + user_cb = rte_zmalloc("eal device event", >> + sizeof(*user_cb), 0); > No need to use rte_malloc here. > Please check this callback API patch: > https://dpdk.org/patch/33144/ could be better to concentration the code. but if you could tell me why not use rte_zmalloc. >> --- a/lib/librte_eal/common/include/rte_dev.h >> +++ b/lib/librte_eal/common/include/rte_dev.h >> +enum uev_monitor_netlink_group { >> + UEV_MONITOR_KERNEL, >> + UEV_MONITOR_UDEV, >> +}; > Please keep a namespace prefix like RTE_DEV_EVENT_ (same for enum name). > Some comments are missing for these constants. >> +/** >> + * The device event type. >> + */ >> +enum rte_dev_event_type { >> + RTE_DEV_EVENT_UNKNOWN, /**< unknown event type */ >> + RTE_DEV_EVENT_ADD, /**< device being added */ >> + RTE_DEV_EVENT_REMOVE, >> + /**< device being removed */ >> + RTE_DEV_EVENT_CHANGE, >> + /**< device status being changed, >> + * etc charger percent >> + */ > What means status changed? > What means charger percent? status changed means that object path change or other more, charger percent just a example for some kobject status. so i don't think we should explicit identify all , i will delete it until we want to use it. >> + RTE_DEV_EVENT_MOVE, /**< device sysfs path being moved */ > sysfs is Linux specific > >> + RTE_DEV_EVENT_ONLINE, /**< device being enable */ > You mean a device can be added but not enabled? > So enabling is switching it on by a register? or something else? > >> + RTE_DEV_EVENT_OFFLINE, /**< device being disable */ >> + RTE_DEV_EVENT_MAX /**< max value of this enum */ >> +}; >> + >> +struct rte_eal_uevent { >> + enum rte_dev_event_type type; /**< device event type */ >> + int subsystem; /**< subsystem id */ >> + char *devname; /**< device name */ >> + enum uev_monitor_netlink_group group; /**< device netlink group */ >> +}; > I don't understand why this struct is exposed in the public API. > Please rename from rte_eal_ to rte_dev_. will modify the uevent to event. >> @@ -166,6 +204,8 @@ struct rte_device { >> const struct rte_driver *driver;/**< Associated driver */ >> int numa_node; /**< NUMA node connection */ >> struct rte_devargs *devargs; /**< Device user arguments */ >> + /** User application callbacks for device event */ >> + struct rte_eal_dev_cb_list uev_cbs; > Do not use uev word in API, it refers to uevent which is implementation > specific. You can name it event_callbacks. > > I am afraid this change is breaking the ABI. > For the first time, 18.02 will be ABI stable. will not modify rte device struct in v10. >> +/** >> + * It registers the callback for the specific event. Multiple >> + * callbacks cal be registered at the same time. >> + * @param event >> + * The device event type. >> + * @param cb_fn >> + * callback address. >> + * @param cb_arg >> + * address of parameter for callback. >> + * >> + * @return >> + * - On success, zero. >> + * - On failure, a negative value. >> + */ >> +int rte_dev_callback_register(struct rte_device *device, >> + enum rte_dev_event_type event, >> + rte_eal_dev_cb_fn cb_fn, void *cb_arg); >> + >> +/** >> + * It unregisters the callback according to the specified event. >> + * >> + * @param event >> + * The event type which corresponding to the callback. >> + * @param cb_fn >> + * callback address. >> + * address of parameter for callback, (void *)-1 means to remove all >> + * registered which has the same callback address. >> + * >> + * @return >> + * - On success, return the number of callback entities removed. >> + * - On failure, a negative value. >> + */ >> +int rte_dev_callback_unregister(struct rte_device *device, >> + enum rte_dev_event_type event, >> + rte_eal_dev_cb_fn cb_fn, void *cb_arg); > Such new functions should be added as experimental. > > There will be probably more to review in this patch. > Let's progress on these comments please. thanks for your review!