From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id D58AF1BEB5 for ; Fri, 15 Jun 2018 17:42:36 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Jun 2018 08:42:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,227,1526367600"; d="scan'208";a="57630072" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.66]) ([10.237.220.66]) by FMSMGA003.fm.intel.com with ESMTP; 15 Jun 2018 08:42:34 -0700 To: Qi Zhang , thomas@monjalon.net Cc: konstantin.ananyev@intel.com, dev@dpdk.org, bruce.richardson@intel.com, ferruh.yigit@intel.com, benjamin.h.shelton@intel.com, narender.vangati@intel.com References: <20180607123849.14439-1-qi.z.zhang@intel.com> <20180607123849.14439-6-qi.z.zhang@intel.com> From: "Burakov, Anatoly" Message-ID: Date: Fri, 15 Jun 2018 16:42:33 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180607123849.14439-6-qi.z.zhang@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 05/22] ethdev: introduce device lock 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: Fri, 15 Jun 2018 15:42:38 -0000 On 07-Jun-18 1:38 PM, Qi Zhang wrote: > Introduce API rte_eth_dev_lock and rte_eth_dev_unlock to let > application lock or unlock on specific ethdev, a locked device > can't be detached, this help applicaiton to prevent unexpected > device detaching, especially in multi-process envrionment. > > Aslo the new API let application to register a callback function > which will be invoked before a device is going to be detached, > the return value of the function will decide if device will continue > be detached or not, this support application to do condition check > at runtime. > > Signed-off-by: Qi Zhang > --- > + > +int > +rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t callback, > + void *user_args) > +{ > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > + > + if (callback == NULL) > + return register_lock_callback(port_id, dev_is_busy, NULL); > + else > + return register_lock_callback(port_id, callback, user_args); As much as i don't like seeing negative errno values as return, the rest of ethdev library uses those, so this is OK :) > +} > + > +int > +rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t callback, > + void *user_args) > +{ > + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV); > + > + * Also, any callback function return !0 value will prevent device be > + * detached(ref. rte_eth_dev_lock and rte_eth_dev_unlock). > + * > + * @param port_id > + * The port identifier of the Ethernet device. > + * @param user_args > + * This is parameter "user_args" be saved when callback function is > + * registered(rte_dev_eth_lock). > + * > + * @return > + * 0 device is allowed be detached. > + * !0 device is not allowed be detached. !0 can be negative or positive. Are we expecting positive return values from this API? > + */ > +typedef int (*rte_eth_dev_lock_callback_t)(uint16_t port_id, void *user_args); > + > +/** > + * Lock an Ethernet Device directly or register a callback function > + * for condition check at runtime, this help application to prevent > + * a device be detached unexpectly. > + * NOTE: Lock a device mutliple times with same parmeter will increase > + * a ref_count, and coresponding unlock decrease the ref_count, the > + * device will be unlocked when ref_count reach 0. Nitpick: "note" sections should be done with @note marker. Also, i would mention that this is a per-process lock that does not affect other processes (assuming i understood the code correctly, of course...). > + * > + * @param port_id > + * The port identifier of the Ethernet device. > + * @param callback > + * !NULL the callback function will be added into a pre-detach list, > + * it will be invoked when a device is going to be detached. The > + * return value will decide if continue detach the device or not. > + * NULL lock the device directly, basically this just regiter a empty > + * callback function(dev_is_busy) that return -EBUSY, so we can > + * handle the pre-detach check in unified way. > + * @param user_args > + * parameter will be parsed to callback function, only valid when > + * callback != NULL. > + * @return > + * 0 on success, negative on error. > + */ > +int rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t callback, > + void *user_args); Nitpicks: DPDK style guide discourages using spaces as indentation (other parts of this patch, and other patches have this issue as well). > + > +/** > + * Reverse operation of rte_eth_dev_lock. > + * > + * @param port_id > + * The port identifier of the Ethernet device. > + * @param callback > + * NULL decrease the ref_count of default callback function. > + * !NULL decrease the ref_count of specific callback with matched > + * user_args. > + * @param user_args > + * parameter to match, only valid when callback != NULL. > + * @return > + * 0 on success, negative on error. > + */ > +int rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t callback, > + void *user_args); > + > #ifdef __cplusplus > } > #endif > diff --git a/lib/librte_ethdev/rte_ethdev_lock.c b/lib/librte_ethdev/rte_ethdev_lock.c rte_ethdev_lock.* seem to be internal-only files. Perhaps you should name them without the rte_ prefix to indicate that they're not exported? > new file mode 100644 > index 000000000..688d1d70a > --- /dev/null > +++ b/lib/librte_ethdev/rte_ethdev_lock.c > @@ -0,0 +1,102 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright(c) 2018 Intel Corporation > + */ > +#include "rte_ethdev_lock.h" > + > +struct lock_entry { > + TAILQ_ENTRY(lock_entry) next; > + rte_eth_dev_lock_callback_t callback; > + uint16_t port_id; > +register_lock_callback(uint16_t port_id, > + rte_eth_dev_lock_callback_t callback, > + void *user_args) > +{ > + struct lock_entry *le; > + > + rte_spinlock_lock(&lock_entry_lock); > + > + TAILQ_FOREACH(le, &lock_entry_list, next) { > + if (le->port_id == port_id && > + le->callback == callback && > + le->user_args == user_args) > + break; > + } > + > + if (!le) { > + le = calloc(1, sizeof(struct lock_entry)); > + if (!le) { Nitpick: generally, DPDK style guide prefers "if (value)" or "if (!value)" to only be reserved for boolean values, and use explicit comparison (e.g. "if (value == NULL)" or "if (value == 0)") for all other cases. -- Thanks, Anatoly