Patch Comment List
List patch comments
GET /api/patches/40749/comments/?format=api&order=date
[ { "id": 82266, "web_url": "http://patches.dpdk.org/comment/82266/", "msgid": "<bab2b78c-3037-661d-2e18-84e2749f8d94@intel.com>", "list_archive_url": "https://inbox.dpdk.org/dev/bab2b78c-3037-661d-2e18-84e2749f8d94@intel.com", "date": "2018-06-15T15:42:33", "subject": "Re: [dpdk-dev] [PATCH 05/22] ethdev: introduce device lock", "submitter": { "id": 4, "url": "http://patches.dpdk.org/api/people/4/?format=api", "name": "Burakov, Anatoly", "email": "anatoly.burakov@intel.com" }, "content": "On 07-Jun-18 1:38 PM, Qi Zhang wrote:\n> Introduce API rte_eth_dev_lock and rte_eth_dev_unlock to let\n> application lock or unlock on specific ethdev, a locked device\n> can't be detached, this help applicaiton to prevent unexpected\n> device detaching, especially in multi-process envrionment.\n> \n> Aslo the new API let application to register a callback function\n> which will be invoked before a device is going to be detached,\n> the return value of the function will decide if device will continue\n> be detached or not, this support application to do condition check\n> at runtime.\n> \n> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>\n> ---\n\n<snip>\n\n> +\n> +int\n> +rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,\n> +\t\t void *user_args)\n> +{\n> +\tRTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);\n> +\n> +\tif (callback == NULL)\n> +\t\treturn register_lock_callback(port_id, dev_is_busy, NULL);\n> +\telse\n> +\t\treturn register_lock_callback(port_id, callback, user_args);\n\nAs much as i don't like seeing negative errno values as return, the rest \nof ethdev library uses those, so this is OK :)\n\n> +}\n> +\n> +int\n> +rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,\n> +\t\t void *user_args)\n> +{\n> +\tRTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);\n> +\n\n<snip>\n\n> + * Also, any callback function return !0 value will prevent device be\n> + * detached(ref. rte_eth_dev_lock and rte_eth_dev_unlock).\n> + *\n> + * @param port_id\n> + * The port identifier of the Ethernet device.\n> + * @param user_args\n> + * This is parameter \"user_args\" be saved when callback function is\n> + * registered(rte_dev_eth_lock).\n> + *\n> + * @return\n> + * 0 device is allowed be detached.\n> + * !0 device is not allowed be detached.\n\n!0 can be negative or positive. Are we expecting positive return values \nfrom this API?\n\n> + */\n> +typedef int (*rte_eth_dev_lock_callback_t)(uint16_t port_id, void *user_args);\n> +\n> +/**\n> + * Lock an Ethernet Device directly or register a callback function\n> + * for condition check at runtime, this help application to prevent\n> + * a device be detached unexpectly.\n> + * NOTE: Lock a device mutliple times with same parmeter will increase\n> + * a ref_count, and coresponding unlock decrease the ref_count, the\n> + * device will be unlocked when ref_count reach 0.\n\nNitpick: \"note\" sections should be done with @note marker.\n\nAlso, i would mention that this is a per-process lock that does not \naffect other processes (assuming i understood the code correctly, of \ncourse...).\n\n> + *\n> + * @param port_id\n> + * The port identifier of the Ethernet device.\n> + * @param callback\n> + * !NULL the callback function will be added into a pre-detach list,\n> + * it will be invoked when a device is going to be detached. The\n> + * return value will decide if continue detach the device or not.\n> + * NULL lock the device directly, basically this just regiter a empty\n> + * callback function(dev_is_busy) that return -EBUSY, so we can\n> + * handle the pre-detach check in unified way.\n> + * @param user_args\n> + * parameter will be parsed to callback function, only valid when\n> + * callback != NULL.\n> + * @return\n> + * 0 on success, negative on error.\n> + */\n> +int rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,\n> +\t\t void *user_args);\n\nNitpicks: DPDK style guide discourages using spaces as indentation \n(other parts of this patch, and other patches have this issue as well).\n\n> +\n> +/**\n> + * Reverse operation of rte_eth_dev_lock.\n> + *\n> + * @param port_id\n> + * The port identifier of the Ethernet device.\n> + * @param callback\n> + * NULL decrease the ref_count of default callback function.\n> + * !NULL decrease the ref_count of specific callback with matched\n> + * user_args.\n> + * @param user_args\n> + * parameter to match, only valid when callback != NULL.\n> + * @return\n> + * 0 on success, negative on error.\n> + */\n> +int rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,\n> +\t\t void *user_args);\n> +\n> #ifdef __cplusplus\n> }\n> #endif\n> diff --git a/lib/librte_ethdev/rte_ethdev_lock.c b/lib/librte_ethdev/rte_ethdev_lock.c\n\nrte_ethdev_lock.* seem to be internal-only files. Perhaps you should \nname them without the rte_ prefix to indicate that they're not exported?\n\n> new file mode 100644\n> index 000000000..688d1d70a\n> --- /dev/null\n> +++ b/lib/librte_ethdev/rte_ethdev_lock.c\n> @@ -0,0 +1,102 @@\n> +/* SPDX-License-Identifier: BSD-3-Clause\n> + * Copyright(c) 2018 Intel Corporation\n> + */\n> +#include \"rte_ethdev_lock.h\"\n> +\n> +struct lock_entry {\n> +\tTAILQ_ENTRY(lock_entry) next;\n> +\trte_eth_dev_lock_callback_t callback;\n> +\tuint16_t port_id;\n\n<snip>\n\n> +register_lock_callback(uint16_t port_id,\n> +\t\t rte_eth_dev_lock_callback_t callback,\n> +\t\t void *user_args)\n> +{\n> +\tstruct lock_entry *le;\n> +\n> +\trte_spinlock_lock(&lock_entry_lock);\n> +\n> +\tTAILQ_FOREACH(le, &lock_entry_list, next) {\n> +\t\tif (le->port_id == port_id &&\n> +\t\t le->callback == callback &&\n> +\t\t le->user_args == user_args)\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\tif (!le) {\n> +\t\tle = calloc(1, sizeof(struct lock_entry));\n> +\t\tif (!le) {\n\nNitpick: generally, DPDK style guide prefers \"if (value)\" or \"if \n(!value)\" to only be reserved for boolean values, and use explicit \ncomparison (e.g. \"if (value == NULL)\" or \"if (value == 0)\") for all \nother cases.", "headers": { "Return-Path": "<dev-bounces@dpdk.org>", "X-Original-To": "patchwork@dpdk.org", "Delivered-To": "patchwork@dpdk.org", "Received": [ "from [92.243.14.124] (localhost [127.0.0.1])\n\tby dpdk.org (Postfix) with ESMTP id AB2731BEB6;\n\tFri, 15 Jun 2018 17:42:39 +0200 (CEST)", "from mga12.intel.com (mga12.intel.com [192.55.52.136])\n\tby dpdk.org (Postfix) with ESMTP id D58AF1BEB5\n\tfor <dev@dpdk.org>; Fri, 15 Jun 2018 17:42:36 +0200 (CEST)", "from fmsmga003.fm.intel.com ([10.253.24.29])\n\tby fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t15 Jun 2018 08:42:35 -0700", "from aburakov-mobl.ger.corp.intel.com (HELO [10.237.220.66])\n\t([10.237.220.66])\n\tby FMSMGA003.fm.intel.com with ESMTP; 15 Jun 2018 08:42:34 -0700" ], "X-Amp-Result": "SKIPPED(no attachment in message)", "X-Amp-File-Uploaded": "False", "X-ExtLoop1": "1", "X-IronPort-AV": "E=Sophos;i=\"5.51,227,1526367600\"; d=\"scan'208\";a=\"57630072\"", "To": "Qi Zhang <qi.z.zhang@intel.com>, thomas@monjalon.net", "Cc": "konstantin.ananyev@intel.com, dev@dpdk.org, bruce.richardson@intel.com, \n\tferruh.yigit@intel.com, benjamin.h.shelton@intel.com,\n\tnarender.vangati@intel.com", "References": "<20180607123849.14439-1-qi.z.zhang@intel.com>\n\t<20180607123849.14439-6-qi.z.zhang@intel.com>", "From": "\"Burakov, Anatoly\" <anatoly.burakov@intel.com>", "Message-ID": "<bab2b78c-3037-661d-2e18-84e2749f8d94@intel.com>", "Date": "Fri, 15 Jun 2018 16:42:33 +0100", "User-Agent": "Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101\n\tThunderbird/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 <dev.dpdk.org>", "List-Unsubscribe": "<https://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://dpdk.org/ml/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<https://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "Errors-To": "dev-bounces@dpdk.org", "Sender": "\"dev\" <dev-bounces@dpdk.org>" }, "addressed": null }, { "id": 82269, "web_url": "http://patches.dpdk.org/comment/82269/", "msgid": "<20180615090903.67198498@xeon-e3>", "list_archive_url": "https://inbox.dpdk.org/dev/20180615090903.67198498@xeon-e3", "date": "2018-06-15T16:09:03", "subject": "Re: [dpdk-dev] [PATCH 05/22] ethdev: introduce device lock", "submitter": { "id": 27, "url": "http://patches.dpdk.org/api/people/27/?format=api", "name": "Stephen Hemminger", "email": "stephen@networkplumber.org" }, "content": "On Thu, 7 Jun 2018 20:38:32 +0800\nQi Zhang <qi.z.zhang@intel.com> wrote:\n\n> +/**\n> + * Lock an Ethernet Device directly or register a callback function\n> + * for condition check at runtime, this help application to prevent\n> + * a device be detached unexpectly.\n> + * NOTE: Lock a device mutliple times with same parmeter will increase\n> + * a ref_count, and coresponding unlock decrease the ref_count, the\n> + * device will be unlocked when ref_count reach 0.\n> + *\n> + * @param port_id\n> + * The port identifier of the Ethernet device.\n> + * @param callback\n> + * !NULL the callback function will be added into a pre-detach list,\n> + * it will be invoked when a device is going to be detached. The\n> + * return value will decide if continue detach the device or not.\n> + * NULL lock the device directly, basically this just regiter a empty\n> + * callback function(dev_is_busy) that return -EBUSY, so we can\n> + * handle the pre-detach check in unified way.\n> + * @param user_args\n> + * parameter will be parsed to callback function, only valid when\n> + * callback != NULL.\n> + * @return\n> + * 0 on success, negative on error.\n> + */\n> +int rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,\n> +\t\t void *user_args);\n\nI prefer API's that do one thing with one function.\nWhy not\n\trte_eth_dev_lock(uint16_t port_id);\n\trte_eth_dev_ondetach(uint16_t port_id, rte_eth_dev_lock_callback_t callback,\n \t\t void *user_args);", "headers": { "Return-Path": "<dev-bounces@dpdk.org>", "X-Original-To": "patchwork@dpdk.org", "Delivered-To": "patchwork@dpdk.org", "Received": [ "from [92.243.14.124] (localhost [127.0.0.1])\n\tby dpdk.org (Postfix) with ESMTP id 2E7F01BFE8;\n\tFri, 15 Jun 2018 18:09:09 +0200 (CEST)", "from mail-pf0-f195.google.com (mail-pf0-f195.google.com\n\t[209.85.192.195]) by dpdk.org (Postfix) with ESMTP id DC9271BFE7\n\tfor <dev@dpdk.org>; Fri, 15 Jun 2018 18:09:06 +0200 (CEST)", "by mail-pf0-f195.google.com with SMTP id a22-v6so5084264pfo.12\n\tfor <dev@dpdk.org>; Fri, 15 Jun 2018 09:09:06 -0700 (PDT)", "from xeon-e3 (204-195-35-107.wavecable.com. [204.195.35.107])\n\tby smtp.gmail.com with ESMTPSA id\n\tt5-v6sm12153429pfh.32.2018.06.15.09.09.05\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 15 Jun 2018 09:09:05 -0700 (PDT)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=networkplumber-org.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:in-reply-to:references\n\t:mime-version:content-transfer-encoding;\n\tbh=GkAaBEBza+sWfVa2d+hNyrLMMZGxLOWbUvWE/c2KaC0=;\n\tb=fMFR17ZleuMy7hLwp45fLGWxac6TV31io8MVkK1/56CdsO7oDANlbvKThrS+6JVak9\n\t6Xqlzv1zaI6pkORf5FF3DI5Ot9H9F7uzHuM6AE7AnSFpXAgVbFh0nGWJYt2Y/FlS2Muz\n\t/AkWimtZQh5MHFGu6duzHHE4NXsZYiiNJUUrDex0cfeYUYITWQwhFyxvCjc0AZBFdZI4\n\t5bsQJ0FOQKdBnMGgDjXAo3MF0vkIodrIEU3YuEBbTbHGsz1hmdNizmJ6jtubb07qsfP7\n\tJ99LgqrW+7M0no5BA1qz6i4biCPodMAfXa696Ixa129DA3ne2hethPq3LYlLZMHbWTL2\n\tpYXw==", "X-Google-DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to\n\t:references:mime-version:content-transfer-encoding;\n\tbh=GkAaBEBza+sWfVa2d+hNyrLMMZGxLOWbUvWE/c2KaC0=;\n\tb=YqMCXF8zTiZ1V1ARTnbwzkJTl/YkcwGsj8vkTKTnWTSbJENkV66IvxU2tWrIVae++a\n\tgAZqxLorSECENFJOxpOiYcZaQnG2YuGU42Gf+/mMDaMIqnbtZ1DqG+d3siuuRLne1HkU\n\tpyg1LzW4Qr2SH4mJ+qJZ19IpBcun9njUopWbYKsdUgdyQ9SlY5R/YsLe+RvLEUlwmS7T\n\tXzTfJkN/ZBjmjjPS9iXFL2cTjGFRGFmAVqmjVGB2dsEuA6zhGKKJWDppJVru3hrSxCt6\n\t2780hJi8Zjl41JjUc6+Dr8hMQGXgDL8fJ64hFsuZH0RSXRC++oaRprI1rJ9Ha9DjxphN\n\tI2Tg==", "X-Gm-Message-State": "APt69E0cOByyx3EmOCGjJC61mP2PFHXKq+5Aq6H++EmeGlH+KwJMyHst\n\tKq0rln7RGR5ysC//EsqG97anCQ==", "X-Google-Smtp-Source": "ADUXVKIaf/BdU0zmNsQZpdhJqv2Cw3wmtwAbKngpa7KRSKojGAvL/aXgBqtcQCtgkq7upnG8uEx5mg==", "X-Received": "by 2002:aa7:810c:: with SMTP id\n\tb12-v6mr2589540pfi.79.1529078946001; \n\tFri, 15 Jun 2018 09:09:06 -0700 (PDT)", "Date": "Fri, 15 Jun 2018 09:09:03 -0700", "From": "Stephen Hemminger <stephen@networkplumber.org>", "To": "Qi Zhang <qi.z.zhang@intel.com>", "Cc": "thomas@monjalon.net, anatoly.burakov@intel.com,\n\tkonstantin.ananyev@intel.com, dev@dpdk.org, bruce.richardson@intel.com,\n\tferruh.yigit@intel.com, benjamin.h.shelton@intel.com,\n\tnarender.vangati@intel.com", "Message-ID": "<20180615090903.67198498@xeon-e3>", "In-Reply-To": "<20180607123849.14439-6-qi.z.zhang@intel.com>", "References": "<20180607123849.14439-1-qi.z.zhang@intel.com>\n\t<20180607123849.14439-6-qi.z.zhang@intel.com>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=US-ASCII", "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 <dev.dpdk.org>", "List-Unsubscribe": "<https://dpdk.org/ml/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://dpdk.org/ml/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<https://dpdk.org/ml/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "Errors-To": "dev-bounces@dpdk.org", "Sender": "\"dev\" <dev-bounces@dpdk.org>" }, "addressed": null }, { "id": 82400, "web_url": "http://patches.dpdk.org/comment/82400/", "msgid": "<039ED4275CED7440929022BC67E706115323A32F@SHSMSX103.ccr.corp.intel.com>", "list_archive_url": "https://inbox.dpdk.org/dev/039ED4275CED7440929022BC67E706115323A32F@SHSMSX103.ccr.corp.intel.com", "date": "2018-06-19T14:16:30", "subject": "Re: [dpdk-dev] [PATCH 05/22] ethdev: introduce device lock", "submitter": { "id": 504, "url": "http://patches.dpdk.org/api/people/504/?format=api", "name": "Qi Zhang", "email": "qi.z.zhang@intel.com" }, "content": "Hi Stephen:\n\n\n> -----Original Message-----\n> From: Stephen Hemminger [mailto:stephen@networkplumber.org]\n> Sent: Saturday, June 16, 2018 12:09 AM\n> To: Zhang, Qi Z <qi.z.zhang@intel.com>\n> Cc: thomas@monjalon.net; Burakov, Anatoly <anatoly.burakov@intel.com>;\n> Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;\n> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh\n> <ferruh.yigit@intel.com>; Shelton, Benjamin H\n> <benjamin.h.shelton@intel.com>; Vangati, Narender\n> <narender.vangati@intel.com>\n> Subject: Re: [dpdk-dev] [PATCH 05/22] ethdev: introduce device lock\n> \n> On Thu, 7 Jun 2018 20:38:32 +0800\n> Qi Zhang <qi.z.zhang@intel.com> wrote:\n> \n> > +/**\n> > + * Lock an Ethernet Device directly or register a callback function\n> > + * for condition check at runtime, this help application to prevent\n> > + * a device be detached unexpectly.\n> > + * NOTE: Lock a device mutliple times with same parmeter will increase\n> > + * a ref_count, and coresponding unlock decrease the ref_count, the\n> > + * device will be unlocked when ref_count reach 0.\n> > + *\n> > + * @param port_id\n> > + * The port identifier of the Ethernet device.\n> > + * @param callback\n> > + * !NULL the callback function will be added into a pre-detach list,\n> > + * it will be invoked when a device is going to be detached. The\n> > + * return value will decide if continue detach the device or not.\n> > + * NULL lock the device directly, basically this just regiter a empty\n> > + * callback function(dev_is_busy) that return -EBUSY, so we can\n> > + * handle the pre-detach check in unified way.\n> > + * @param user_args\n> > + * parameter will be parsed to callback function, only valid when\n> > + * callback != NULL.\n> > + * @return\n> > + * 0 on success, negative on error.\n> > + */\n> > +int rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t\n> callback,\n> > +\t\t void *user_args);\n> \n> I prefer API's that do one thing with one function.\n\nAgree\n\n> Why not\n> \trte_eth_dev_lock(uint16_t port_id);\n> \trte_eth_dev_ondetach(uint16_t port_id, rte_eth_dev_lock_callback_t\n> callback,\n> \t\t void *user_args);\n\nRte_eth_dev_ondetach looks like a callback function, \nbut this is the function to register some condition check.\nHow about rte_eth_dev_lock and rte_eth_dev_lock_with_cond?\n\nThanks\nQi", "headers": { "Return-Path": "<dev-bounces@dpdk.org>", "X-Original-To": "patchwork@dpdk.org", "Delivered-To": "patchwork@dpdk.org", "Received": [ "from [92.243.14.124] (localhost [127.0.0.1])\n\tby dpdk.org (Postfix) with ESMTP id 5A0244CE4;\n\tTue, 19 Jun 2018 16:16:36 +0200 (CEST)", "from mga12.intel.com (mga12.intel.com [192.55.52.136])\n\tby dpdk.org (Postfix) with ESMTP id 5DB1C4CC0\n\tfor <dev@dpdk.org>; Tue, 19 Jun 2018 16:16:35 +0200 (CEST)", "from fmsmga004.fm.intel.com ([10.253.24.48])\n\tby fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t19 Jun 2018 07:16:34 -0700", "from fmsmsx105.amr.corp.intel.com ([10.18.124.203])\n\tby fmsmga004.fm.intel.com with ESMTP; 19 Jun 2018 07:16:34 -0700", "from shsmsx102.ccr.corp.intel.com (10.239.4.154) by\n\tFMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP\n\tServer (TLS) id 14.3.319.2; Tue, 19 Jun 2018 07:16:33 -0700", "from shsmsx103.ccr.corp.intel.com ([169.254.4.51]) by\n\tshsmsx102.ccr.corp.intel.com ([169.254.2.223]) with mapi id\n\t14.03.0319.002; Tue, 19 Jun 2018 22:16:31 +0800" ], "X-Amp-Result": "SKIPPED(no attachment in message)", "X-Amp-File-Uploaded": "False", "X-ExtLoop1": "1", "X-IronPort-AV": "E=Sophos;i=\"5.51,243,1526367600\"; d=\"scan'208\";a=\"64493619\"", "From": "\"Zhang, Qi Z\" <qi.z.zhang@intel.com>", "To": "Stephen Hemminger <stephen@networkplumber.org>", "CC": "\"thomas@monjalon.net\" <thomas@monjalon.net>, \"Burakov, Anatoly\"\n\t<anatoly.burakov@intel.com>, \"Ananyev, Konstantin\"\n\t<konstantin.ananyev@intel.com>, \"dev@dpdk.org\" <dev@dpdk.org>,\n\t\"Richardson, Bruce\" <bruce.richardson@intel.com>, \"Yigit, Ferruh\"\n\t<ferruh.yigit@intel.com>, \"Shelton, Benjamin H\"\n\t<benjamin.h.shelton@intel.com>, \"Vangati, Narender\"\n\t<narender.vangati@intel.com>", "Thread-Topic": "[dpdk-dev] [PATCH 05/22] ethdev: introduce device lock", "Thread-Index": "AQHUBMM3/7FOR3hvjUSZ2LucRDEGyqRno5SQ", "Date": "Tue, 19 Jun 2018 14:16:30 +0000", "Message-ID": "<039ED4275CED7440929022BC67E706115323A32F@SHSMSX103.ccr.corp.intel.com>", "References": "<20180607123849.14439-1-qi.z.zhang@intel.com>\n\t<20180607123849.14439-6-qi.z.zhang@intel.com>\n\t<20180615090903.67198498@xeon-e3>", "In-Reply-To": "<20180615090903.67198498@xeon-e3>", "Accept-Language": "en-US", "Content-Language": "en-US", "X-MS-Has-Attach": "", "X-MS-TNEF-Correlator": "", "x-titus-metadata-40": "eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOTRkY2I2ZDYtZGI1NC00ZTQ1LWI2YWUtZTNmMmNhZjdiZDFlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiME9JQjNUY09FQnpXbmdadjc4T09CNHlGODlSZU9mMHJIXC9OYUJ2ZHBHbXdXWjBsVWNjcU9qSUpBVUljK0ZcL253In0=", "x-ctpclassification": "CTP_NT", "dlp-product": "dlpe-windows", "dlp-version": "11.0.200.100", "dlp-reaction": "no-action", "x-originating-ip": "[10.239.127.40]", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "quoted-printable", "MIME-Version": "1.0", "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 <dev.dpdk.org>", "List-Unsubscribe": "<https://mails.dpdk.org/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://mails.dpdk.org/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<https://mails.dpdk.org/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "Errors-To": "dev-bounces@dpdk.org", "Sender": "\"dev\" <dev-bounces@dpdk.org>" }, "addressed": null }, { "id": 82419, "web_url": "http://patches.dpdk.org/comment/82419/", "msgid": "<039ED4275CED7440929022BC67E706115323AA7F@SHSMSX103.ccr.corp.intel.com>", "list_archive_url": "https://inbox.dpdk.org/dev/039ED4275CED7440929022BC67E706115323AA7F@SHSMSX103.ccr.corp.intel.com", "date": "2018-06-20T04:00:01", "subject": "Re: [dpdk-dev] [PATCH 05/22] ethdev: introduce device lock", "submitter": { "id": 504, "url": "http://patches.dpdk.org/api/people/504/?format=api", "name": "Qi Zhang", "email": "qi.z.zhang@intel.com" }, "content": "Hi Anatoly:\n\tSorry to miss this email and reply late.\n\n> -----Original Message-----\n> From: Burakov, Anatoly\n> Sent: Friday, June 15, 2018 11:43 PM\n> To: Zhang, Qi Z <qi.z.zhang@intel.com>; thomas@monjalon.net\n> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org;\n> Richardson, Bruce <bruce.richardson@intel.com>; Yigit, Ferruh\n> <ferruh.yigit@intel.com>; Shelton, Benjamin H\n> <benjamin.h.shelton@intel.com>; Vangati, Narender\n> <narender.vangati@intel.com>\n> Subject: Re: [PATCH 05/22] ethdev: introduce device lock\n> \n> On 07-Jun-18 1:38 PM, Qi Zhang wrote:\n> > Introduce API rte_eth_dev_lock and rte_eth_dev_unlock to let\n> > application lock or unlock on specific ethdev, a locked device can't\n> > be detached, this help applicaiton to prevent unexpected device\n> > detaching, especially in multi-process envrionment.\n> >\n> > Aslo the new API let application to register a callback function which\n> > will be invoked before a device is going to be detached, the return\n> > value of the function will decide if device will continue be detached\n> > or not, this support application to do condition check at runtime.\n> >\n> > Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>\n> > ---\n> \n> <snip>\n> \n> > +\n> > +int\n> > +rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,\n> > +\t\t void *user_args)\n> > +{\n> > +\tRTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);\n> > +\n> > +\tif (callback == NULL)\n> > +\t\treturn register_lock_callback(port_id, dev_is_busy, NULL);\n> > +\telse\n> > +\t\treturn register_lock_callback(port_id, callback, user_args);\n> \n> As much as i don't like seeing negative errno values as return, the rest of\n> ethdev library uses those, so this is OK :)\n> \n> > +}\n> > +\n> > +int\n> > +rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t callback,\n> > +\t\t void *user_args)\n> > +{\n> > +\tRTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);\n> > +\n> \n> <snip>\n> \n> > + * Also, any callback function return !0 value will prevent device be\n> > + * detached(ref. rte_eth_dev_lock and rte_eth_dev_unlock).\n> > + *\n> > + * @param port_id\n> > + * The port identifier of the Ethernet device.\n> > + * @param user_args\n> > + * This is parameter \"user_args\" be saved when callback function is\n> > + * registered(rte_dev_eth_lock).\n> > + *\n> > + * @return\n> > + * 0 device is allowed be detached.\n> > + * !0 device is not allowed be detached.\n> \n> !0 can be negative or positive. Are we expecting positive return values from\n> this API?\n\nI have no strong opinion, but if you think below or other option is better, I can change\n\n>=0 device is allowed be detached.\n<0 device is not allowed be detached.\n\n> \n> > + */\n> > +typedef int (*rte_eth_dev_lock_callback_t)(uint16_t port_id, void\n> > +*user_args);\n> > +\n> > +/**\n> > + * Lock an Ethernet Device directly or register a callback function\n> > + * for condition check at runtime, this help application to prevent\n> > + * a device be detached unexpectedly \n> > + * NOTE: Lock a device multiple times with same parmeter will\n> > +increase\n> > + * a ref_count, and corresponding unlock decrease the ref_count, the\n> > + * device will be unlocked when ref_count reach 0.\n> \n> Nitpick: \"note\" sections should be done with @note marker.\n> \n> Also, i would mention that this is a per-process lock that does not affect other\n> processes (assuming i understood the code correctly, of course...).\n\nOK, I will add more comment to explain this.\n\n> \n> > + *\n> > + * @param port_id\n> > + * The port identifier of the Ethernet device.\n> > + * @param callback\n> > + * !NULL the callback function will be added into a pre-detach list,\n> > + * it will be invoked when a device is going to be detached. The\n> > + * return value will decide if continue detach the device or not.\n> > + * NULL lock the device directly, basically this just regiter a empty\n> > + * callback function(dev_is_busy) that return -EBUSY, so we can\n> > + * handle the pre-detach check in unified way.\n> > + * @param user_args\n> > + * parameter will be parsed to callback function, only valid when\n> > + * callback != NULL.\n> > + * @return\n> > + * 0 on success, negative on error.\n> > + */\n> > +int rte_eth_dev_lock(uint16_t port_id, rte_eth_dev_lock_callback_t\n> callback,\n> > +\t\t void *user_args);\n> \n> Nitpicks: DPDK style guide discourages using spaces as indentation (other parts\n> of this patch, and other patches have this issue as well).\n\nOK, will fix all.\n> \n> > +\n> > +/**\n> > + * Reverse operation of rte_eth_dev_lock.\n> > + *\n> > + * @param port_id\n> > + * The port identifier of the Ethernet device.\n> > + * @param callback\n> > + * NULL decrease the ref_count of default callback function.\n> > + * !NULL decrease the ref_count of specific callback with matched\n> > + * user_args.\n> > + * @param user_args\n> > + * parameter to match, only valid when callback != NULL.\n> > + * @return\n> > + * 0 on success, negative on error.\n> > + */\n> > +int rte_eth_dev_unlock(uint16_t port_id, rte_eth_dev_lock_callback_t\n> callback,\n> > +\t\t void *user_args);\n> > +\n> > #ifdef __cplusplus\n> > }\n> > #endif\n> > diff --git a/lib/librte_ethdev/rte_ethdev_lock.c\n> > b/lib/librte_ethdev/rte_ethdev_lock.c\n> \n> rte_ethdev_lock.* seem to be internal-only files. Perhaps you should name\n> them without the rte_ prefix to indicate that they're not exported?\n> \n> > new file mode 100644\n> > index 000000000..688d1d70a\n> > --- /dev/null\n> > +++ b/lib/librte_ethdev/rte_ethdev_lock.c\n> > @@ -0,0 +1,102 @@\n> > +/* SPDX-License-Identifier: BSD-3-Clause\n> > + * Copyright(c) 2018 Intel Corporation */ #include\n> > +\"rte_ethdev_lock.h\"\n> > +\n> > +struct lock_entry {\n> > +\tTAILQ_ENTRY(lock_entry) next;\n> > +\trte_eth_dev_lock_callback_t callback;\n> > +\tuint16_t port_id;\n> \n> <snip>\n> \n> > +register_lock_callback(uint16_t port_id,\n> > +\t\t rte_eth_dev_lock_callback_t callback,\n> > +\t\t void *user_args)\n> > +{\n> > +\tstruct lock_entry *le;\n> > +\n> > +\trte_spinlock_lock(&lock_entry_lock);\n> > +\n> > +\tTAILQ_FOREACH(le, &lock_entry_list, next) {\n> > +\t\tif (le->port_id == port_id &&\n> > +\t\t le->callback == callback &&\n> > +\t\t le->user_args == user_args)\n> > +\t\t\tbreak;\n> > +\t}\n> > +\n> > +\tif (!le) {\n> > +\t\tle = calloc(1, sizeof(struct lock_entry));\n> > +\t\tif (!le) {\n> \n> Nitpick: generally, DPDK style guide prefers \"if (value)\" or \"if (!value)\" to only\n> be reserved for boolean values, and use explicit comparison (e.g. \"if (value ==\n> NULL)\" or \"if (value == 0)\") for all other cases.\n\nOK, will fix\n> \n> --\n> Thanks,\n> Anatoly", "headers": { "Return-Path": "<dev-bounces@dpdk.org>", "X-Original-To": "patchwork@dpdk.org", "Delivered-To": "patchwork@dpdk.org", "Received": [ "from [92.243.14.124] (localhost [127.0.0.1])\n\tby dpdk.org (Postfix) with ESMTP id 2827D1B431;\n\tWed, 20 Jun 2018 06:00:09 +0200 (CEST)", "from mga07.intel.com (mga07.intel.com [134.134.136.100])\n\tby dpdk.org (Postfix) with ESMTP id 57D011B42F\n\tfor <dev@dpdk.org>; Wed, 20 Jun 2018 06:00:06 +0200 (CEST)", "from orsmga003.jf.intel.com ([10.7.209.27])\n\tby orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;\n\t19 Jun 2018 21:00:04 -0700", "from fmsmsx105.amr.corp.intel.com ([10.18.124.203])\n\tby orsmga003.jf.intel.com with ESMTP; 19 Jun 2018 21:00:04 -0700", "from shsmsx104.ccr.corp.intel.com (10.239.4.70) by\n\tFMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP\n\tServer (TLS) id 14.3.319.2; Tue, 19 Jun 2018 21:00:03 -0700", "from shsmsx103.ccr.corp.intel.com ([169.254.4.51]) by\n\tSHSMSX104.ccr.corp.intel.com ([169.254.5.87]) with mapi id\n\t14.03.0319.002; Wed, 20 Jun 2018 12:00:01 +0800" ], "X-Amp-Result": "SKIPPED(no attachment in message)", "X-Amp-File-Uploaded": "False", "X-ExtLoop1": "1", "X-IronPort-AV": "E=Sophos;i=\"5.51,245,1526367600\"; d=\"scan'208\";a=\"60663670\"", "From": "\"Zhang, Qi Z\" <qi.z.zhang@intel.com>", "To": "\"Burakov, Anatoly\" <anatoly.burakov@intel.com>, \"thomas@monjalon.net\"\n\t<thomas@monjalon.net>", "CC": "\"Ananyev, Konstantin\" <konstantin.ananyev@intel.com>, \"dev@dpdk.org\"\n\t<dev@dpdk.org>, \"Richardson, Bruce\" <bruce.richardson@intel.com>,\n\t\"Yigit, Ferruh\" <ferruh.yigit@intel.com>, \"Shelton, Benjamin H\"\n\t<benjamin.h.shelton@intel.com>, \"Vangati, Narender\"\n\t<narender.vangati@intel.com>", "Thread-Topic": "[PATCH 05/22] ethdev: introduce device lock", "Thread-Index": "AQHUBL+GBVCVeOqaBU6hufOQOUk106RoiNIg", "Date": "Wed, 20 Jun 2018 04:00:01 +0000", "Message-ID": "<039ED4275CED7440929022BC67E706115323AA7F@SHSMSX103.ccr.corp.intel.com>", "References": "<20180607123849.14439-1-qi.z.zhang@intel.com>\n\t<20180607123849.14439-6-qi.z.zhang@intel.com>\n\t<bab2b78c-3037-661d-2e18-84e2749f8d94@intel.com>", "In-Reply-To": "<bab2b78c-3037-661d-2e18-84e2749f8d94@intel.com>", "Accept-Language": "en-US", "Content-Language": "en-US", "X-MS-Has-Attach": "", "X-MS-TNEF-Correlator": "", "x-titus-metadata-40": "eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYjEyODAzZTgtMDljMS00ODdhLWI1MWYtNTQ3MGVlNGQ1MTY5IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoibGNiOFIrYnVVRnA4dENhQ3ZUWWEwYnBoVEZscVJaRG1ibjZYM3pNdWtwRmlGMk5US0xjcmNYb3RCZTF2VnoyUCJ9", "x-ctpclassification": "CTP_NT", "dlp-product": "dlpe-windows", "dlp-version": "11.0.200.100", "dlp-reaction": "no-action", "x-originating-ip": "[10.239.127.40]", "Content-Type": "text/plain; charset=\"utf-8\"", "Content-Transfer-Encoding": "base64", "MIME-Version": "1.0", "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 <dev.dpdk.org>", "List-Unsubscribe": "<https://mails.dpdk.org/options/dev>,\n\t<mailto:dev-request@dpdk.org?subject=unsubscribe>", "List-Archive": "<http://mails.dpdk.org/archives/dev/>", "List-Post": "<mailto:dev@dpdk.org>", "List-Help": "<mailto:dev-request@dpdk.org?subject=help>", "List-Subscribe": "<https://mails.dpdk.org/listinfo/dev>,\n\t<mailto:dev-request@dpdk.org?subject=subscribe>", "Errors-To": "dev-bounces@dpdk.org", "Sender": "\"dev\" <dev-bounces@dpdk.org>" }, "addressed": null } ]