Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/94869/?format=api
{ "id": 94869, "url": "https://patches.dpdk.org/api/patches/94869/?format=api", "web_url": "https://patches.dpdk.org/project/dpdk/patch/0421e68b57d4df9598fbad35919477af7021beac.1624629506.git.anatoly.burakov@intel.com/", "project": { "id": 1, "url": "https://patches.dpdk.org/api/projects/1/?format=api", "name": "DPDK", "link_name": "dpdk", "list_id": "dev.dpdk.org", "list_email": "dev@dpdk.org", "web_url": "http://core.dpdk.org", "scm_url": "git://dpdk.org/dpdk", "webscm_url": "http://git.dpdk.org/dpdk", "list_archive_url": "https://inbox.dpdk.org/dev", "list_archive_url_format": "https://inbox.dpdk.org/dev/{}", "commit_url_format": "" }, "msgid": "<0421e68b57d4df9598fbad35919477af7021beac.1624629506.git.anatoly.burakov@intel.com>", "list_archive_url": "https://inbox.dpdk.org/dev/0421e68b57d4df9598fbad35919477af7021beac.1624629506.git.anatoly.burakov@intel.com", "date": "2021-06-25T14:00:07", "name": "[v2,4/7] power: remove thread safety from PMD power API's", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": true, "hash": "31d132892340efb5092f2ae062c2d59c37ca5fc6", "submitter": { "id": 4, "url": "https://patches.dpdk.org/api/people/4/?format=api", "name": "Anatoly Burakov", "email": "anatoly.burakov@intel.com" }, "delegate": { "id": 1, "url": "https://patches.dpdk.org/api/users/1/?format=api", "username": "tmonjalo", "first_name": "Thomas", "last_name": "Monjalon", "email": "thomas@monjalon.net" }, "mbox": "https://patches.dpdk.org/project/dpdk/patch/0421e68b57d4df9598fbad35919477af7021beac.1624629506.git.anatoly.burakov@intel.com/mbox/", "series": [ { "id": 17490, "url": "https://patches.dpdk.org/api/series/17490/?format=api", "web_url": "https://patches.dpdk.org/project/dpdk/list/?series=17490", "date": "2021-06-25T14:00:03", "name": "Enhancements for PMD power management", "version": 2, "mbox": "https://patches.dpdk.org/series/17490/mbox/" } ], "comments": "https://patches.dpdk.org/api/patches/94869/comments/", "check": "warning", "checks": "https://patches.dpdk.org/api/patches/94869/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<dev-bounces@dpdk.org>", "X-Original-To": "patchwork@inbox.dpdk.org", "Delivered-To": "patchwork@inbox.dpdk.org", "Received": [ "from mails.dpdk.org (mails.dpdk.org [217.70.189.124])\n\tby inbox.dpdk.org (Postfix) with ESMTP id A2169A0547;\n\tFri, 25 Jun 2021 16:01:06 +0200 (CEST)", "from [217.70.189.124] (localhost [127.0.0.1])\n\tby mails.dpdk.org (Postfix) with ESMTP id 670F6410EE;\n\tFri, 25 Jun 2021 16:00:52 +0200 (CEST)", "from mga11.intel.com (mga11.intel.com [192.55.52.93])\n by mails.dpdk.org (Postfix) with ESMTP id 8880340E0F\n for <dev@dpdk.org>; Fri, 25 Jun 2021 16:00:48 +0200 (CEST)", "from fmsmga003.fm.intel.com ([10.253.24.29])\n by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n 25 Jun 2021 07:00:23 -0700", "from silpixa00399498.ir.intel.com (HELO\n silpixa00399498.ger.corp.intel.com) ([10.237.223.53])\n by FMSMGA003.fm.intel.com with ESMTP; 25 Jun 2021 07:00:21 -0700" ], "IronPort-SDR": [ "\n gO6Q8ZYfUIDvsgwMx76h5154vhuDO1fttc3usBEjhH7NepoI8yJ0asIQQznSLG2bTNuzWBQOsI\n DiOuCYKsKACA==", "\n bkTflFAhnTRWSDI81DzpUDPtICaa16qDqXaO35XHTCIdO2tVDQfejBKUuaEJc633tANJzO4JKe\n ay0xh9t0H6mg==" ], "X-IronPort-AV": [ "E=McAfee;i=\"6200,9189,10026\"; a=\"204664520\"", "E=Sophos;i=\"5.83,298,1616482800\"; d=\"scan'208\";a=\"204664520\"", "E=Sophos;i=\"5.83,298,1616482800\"; d=\"scan'208\";a=\"481858714\"" ], "X-ExtLoop1": "1", "From": "Anatoly Burakov <anatoly.burakov@intel.com>", "To": "dev@dpdk.org,\n\tDavid Hunt <david.hunt@intel.com>", "Cc": "ciara.loftus@intel.com", "Date": "Fri, 25 Jun 2021 14:00:07 +0000", "Message-Id": "\n <0421e68b57d4df9598fbad35919477af7021beac.1624629506.git.anatoly.burakov@intel.com>", "X-Mailer": "git-send-email 2.25.1", "In-Reply-To": "<cover.1624629506.git.anatoly.burakov@intel.com>", "References": "<cover.1622548381.git.anatoly.burakov@intel.com>\n <cover.1624629506.git.anatoly.burakov@intel.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[dpdk-dev] [PATCH v2 4/7] power: remove thread safety from PMD\n power API's", "X-BeenThere": "dev@dpdk.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "DPDK patches and discussions <dev.dpdk.org>", "List-Unsubscribe": "<https://mails.dpdk.org/options/dev>,\n <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 <mailto:dev-request@dpdk.org?subject=subscribe>", "Errors-To": "dev-bounces@dpdk.org", "Sender": "\"dev\" <dev-bounces@dpdk.org>" }, "content": "Currently, we expect that only one callback can be active at any given\nmoment, for a particular queue configuration, which is relatively easy\nto implement in a thread-safe way. However, we're about to add support\nfor multiple queues per lcore, which will greatly increase the\npossibility of various race conditions.\n\nWe could have used something like an RCU for this use case, but absent\nof a pressing need for thread safety we'll go the easy way and just\nmandate that the API's are to be called when all affected ports are\nstopped, and document this limitation. This greatly simplifies the\n`rte_power_monitor`-related code.\n\nSigned-off-by: Anatoly Burakov <anatoly.burakov@intel.com>\n---\n\nNotes:\n v2:\n - Add check for stopped queue\n - Clarified doc message\n - Added release notes\n\n doc/guides/rel_notes/release_21_08.rst | 5 +\n lib/power/meson.build | 3 +\n lib/power/rte_power_pmd_mgmt.c | 133 ++++++++++---------------\n lib/power/rte_power_pmd_mgmt.h | 6 ++\n 4 files changed, 67 insertions(+), 80 deletions(-)", "diff": "diff --git a/doc/guides/rel_notes/release_21_08.rst b/doc/guides/rel_notes/release_21_08.rst\nindex 9d1cfac395..f015c509fc 100644\n--- a/doc/guides/rel_notes/release_21_08.rst\n+++ b/doc/guides/rel_notes/release_21_08.rst\n@@ -88,6 +88,11 @@ API Changes\n \n * eal: the ``rte_power_intrinsics`` API changed to use a callback mechanism.\n \n+* rte_power: The experimental PMD power management API is no longer considered\n+ to be thread safe; all Rx queues affected by the API will now need to be\n+ stopped before making any changes to the power management scheme.\n+\n+\n ABI Changes\n -----------\n \ndiff --git a/lib/power/meson.build b/lib/power/meson.build\nindex c1097d32f1..4f6a242364 100644\n--- a/lib/power/meson.build\n+++ b/lib/power/meson.build\n@@ -21,4 +21,7 @@ headers = files(\n 'rte_power_pmd_mgmt.h',\n 'rte_power_guest_channel.h',\n )\n+if cc.has_argument('-Wno-cast-qual')\n+ cflags += '-Wno-cast-qual'\n+endif\n deps += ['timer', 'ethdev']\ndiff --git a/lib/power/rte_power_pmd_mgmt.c b/lib/power/rte_power_pmd_mgmt.c\nindex db03cbf420..9b95cf1794 100644\n--- a/lib/power/rte_power_pmd_mgmt.c\n+++ b/lib/power/rte_power_pmd_mgmt.c\n@@ -40,8 +40,6 @@ struct pmd_queue_cfg {\n \t/**< Callback mode for this queue */\n \tconst struct rte_eth_rxtx_callback *cur_cb;\n \t/**< Callback instance */\n-\tvolatile bool umwait_in_progress;\n-\t/**< are we currently sleeping? */\n \tuint64_t empty_poll_stats;\n \t/**< Number of empty polls */\n } __rte_cache_aligned;\n@@ -92,30 +90,11 @@ clb_umwait(uint16_t port_id, uint16_t qidx, struct rte_mbuf **pkts __rte_unused,\n \t\t\tstruct rte_power_monitor_cond pmc;\n \t\t\tuint16_t ret;\n \n-\t\t\t/*\n-\t\t\t * we might get a cancellation request while being\n-\t\t\t * inside the callback, in which case the wakeup\n-\t\t\t * wouldn't work because it would've arrived too early.\n-\t\t\t *\n-\t\t\t * to get around this, we notify the other thread that\n-\t\t\t * we're sleeping, so that it can spin until we're done.\n-\t\t\t * unsolicited wakeups are perfectly safe.\n-\t\t\t */\n-\t\t\tq_conf->umwait_in_progress = true;\n-\n-\t\t\trte_atomic_thread_fence(__ATOMIC_SEQ_CST);\n-\n-\t\t\t/* check if we need to cancel sleep */\n-\t\t\tif (q_conf->pwr_mgmt_state == PMD_MGMT_ENABLED) {\n-\t\t\t\t/* use monitoring condition to sleep */\n-\t\t\t\tret = rte_eth_get_monitor_addr(port_id, qidx,\n-\t\t\t\t\t\t&pmc);\n-\t\t\t\tif (ret == 0)\n-\t\t\t\t\trte_power_monitor(&pmc, UINT64_MAX);\n-\t\t\t}\n-\t\t\tq_conf->umwait_in_progress = false;\n-\n-\t\t\trte_atomic_thread_fence(__ATOMIC_SEQ_CST);\n+\t\t\t/* use monitoring condition to sleep */\n+\t\t\tret = rte_eth_get_monitor_addr(port_id, qidx,\n+\t\t\t\t\t&pmc);\n+\t\t\tif (ret == 0)\n+\t\t\t\trte_power_monitor(&pmc, UINT64_MAX);\n \t\t}\n \t} else\n \t\tq_conf->empty_poll_stats = 0;\n@@ -177,12 +156,24 @@ clb_scale_freq(uint16_t port_id, uint16_t qidx,\n \treturn nb_rx;\n }\n \n+static int\n+queue_stopped(const uint16_t port_id, const uint16_t queue_id)\n+{\n+\tstruct rte_eth_rxq_info qinfo;\n+\n+\tif (rte_eth_rx_queue_info_get(port_id, queue_id, &qinfo) < 0)\n+\t\treturn -1;\n+\n+\treturn qinfo.queue_state == RTE_ETH_QUEUE_STATE_STOPPED;\n+}\n+\n int\n rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,\n \t\tuint16_t queue_id, enum rte_power_pmd_mgmt_type mode)\n {\n \tstruct pmd_queue_cfg *queue_cfg;\n \tstruct rte_eth_dev_info info;\n+\trte_rx_callback_fn clb;\n \tint ret;\n \n \tRTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);\n@@ -203,6 +194,14 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,\n \t\tgoto end;\n \t}\n \n+\t/* check if the queue is stopped */\n+\tret = queue_stopped(port_id, queue_id);\n+\tif (ret != 1) {\n+\t\t/* error means invalid queue, 0 means queue wasn't stopped */\n+\t\tret = ret < 0 ? -EINVAL : -EBUSY;\n+\t\tgoto end;\n+\t}\n+\n \tqueue_cfg = &port_cfg[port_id][queue_id];\n \n \tif (queue_cfg->pwr_mgmt_state != PMD_MGMT_DISABLED) {\n@@ -232,17 +231,7 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,\n \t\t\tret = -ENOTSUP;\n \t\t\tgoto end;\n \t\t}\n-\t\t/* initialize data before enabling the callback */\n-\t\tqueue_cfg->empty_poll_stats = 0;\n-\t\tqueue_cfg->cb_mode = mode;\n-\t\tqueue_cfg->umwait_in_progress = false;\n-\t\tqueue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;\n-\n-\t\t/* ensure we update our state before callback starts */\n-\t\trte_atomic_thread_fence(__ATOMIC_SEQ_CST);\n-\n-\t\tqueue_cfg->cur_cb = rte_eth_add_rx_callback(port_id, queue_id,\n-\t\t\t\tclb_umwait, NULL);\n+\t\tclb = clb_umwait;\n \t\tbreak;\n \t}\n \tcase RTE_POWER_MGMT_TYPE_SCALE:\n@@ -269,16 +258,7 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,\n \t\t\tret = -ENOTSUP;\n \t\t\tgoto end;\n \t\t}\n-\t\t/* initialize data before enabling the callback */\n-\t\tqueue_cfg->empty_poll_stats = 0;\n-\t\tqueue_cfg->cb_mode = mode;\n-\t\tqueue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;\n-\n-\t\t/* this is not necessary here, but do it anyway */\n-\t\trte_atomic_thread_fence(__ATOMIC_SEQ_CST);\n-\n-\t\tqueue_cfg->cur_cb = rte_eth_add_rx_callback(port_id,\n-\t\t\t\tqueue_id, clb_scale_freq, NULL);\n+\t\tclb = clb_scale_freq;\n \t\tbreak;\n \t}\n \tcase RTE_POWER_MGMT_TYPE_PAUSE:\n@@ -286,18 +266,21 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t port_id,\n \t\tif (global_data.tsc_per_us == 0)\n \t\t\tcalc_tsc();\n \n-\t\t/* initialize data before enabling the callback */\n-\t\tqueue_cfg->empty_poll_stats = 0;\n-\t\tqueue_cfg->cb_mode = mode;\n-\t\tqueue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;\n-\n-\t\t/* this is not necessary here, but do it anyway */\n-\t\trte_atomic_thread_fence(__ATOMIC_SEQ_CST);\n-\n-\t\tqueue_cfg->cur_cb = rte_eth_add_rx_callback(port_id, queue_id,\n-\t\t\t\tclb_pause, NULL);\n+\t\tclb = clb_pause;\n \t\tbreak;\n+\tdefault:\n+\t\tRTE_LOG(DEBUG, POWER, \"Invalid power management type\\n\");\n+\t\tret = -EINVAL;\n+\t\tgoto end;\n \t}\n+\n+\t/* initialize data before enabling the callback */\n+\tqueue_cfg->empty_poll_stats = 0;\n+\tqueue_cfg->cb_mode = mode;\n+\tqueue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED;\n+\tqueue_cfg->cur_cb = rte_eth_add_rx_callback(port_id, queue_id,\n+\t\t\tclb, NULL);\n+\n \tret = 0;\n end:\n \treturn ret;\n@@ -308,12 +291,20 @@ rte_power_ethdev_pmgmt_queue_disable(unsigned int lcore_id,\n \t\tuint16_t port_id, uint16_t queue_id)\n {\n \tstruct pmd_queue_cfg *queue_cfg;\n+\tint ret;\n \n \tRTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);\n \n \tif (lcore_id >= RTE_MAX_LCORE || queue_id >= RTE_MAX_QUEUES_PER_PORT)\n \t\treturn -EINVAL;\n \n+\t/* check if the queue is stopped */\n+\tret = queue_stopped(port_id, queue_id);\n+\tif (ret != 1) {\n+\t\t/* error means invalid queue, 0 means queue wasn't stopped */\n+\t\treturn ret < 0 ? -EINVAL : -EBUSY;\n+\t}\n+\n \t/* no need to check queue id as wrong queue id would not be enabled */\n \tqueue_cfg = &port_cfg[port_id][queue_id];\n \n@@ -323,27 +314,8 @@ rte_power_ethdev_pmgmt_queue_disable(unsigned int lcore_id,\n \t/* stop any callbacks from progressing */\n \tqueue_cfg->pwr_mgmt_state = PMD_MGMT_DISABLED;\n \n-\t/* ensure we update our state before continuing */\n-\trte_atomic_thread_fence(__ATOMIC_SEQ_CST);\n-\n \tswitch (queue_cfg->cb_mode) {\n-\tcase RTE_POWER_MGMT_TYPE_MONITOR:\n-\t{\n-\t\tbool exit = false;\n-\t\tdo {\n-\t\t\t/*\n-\t\t\t * we may request cancellation while the other thread\n-\t\t\t * has just entered the callback but hasn't started\n-\t\t\t * sleeping yet, so keep waking it up until we know it's\n-\t\t\t * done sleeping.\n-\t\t\t */\n-\t\t\tif (queue_cfg->umwait_in_progress)\n-\t\t\t\trte_power_monitor_wakeup(lcore_id);\n-\t\t\telse\n-\t\t\t\texit = true;\n-\t\t} while (!exit);\n-\t}\n-\t/* fall-through */\n+\tcase RTE_POWER_MGMT_TYPE_MONITOR: /* fall-through */\n \tcase RTE_POWER_MGMT_TYPE_PAUSE:\n \t\trte_eth_remove_rx_callback(port_id, queue_id,\n \t\t\t\tqueue_cfg->cur_cb);\n@@ -356,10 +328,11 @@ rte_power_ethdev_pmgmt_queue_disable(unsigned int lcore_id,\n \t\tbreak;\n \t}\n \t/*\n-\t * we don't free the RX callback here because it is unsafe to do so\n-\t * unless we know for a fact that all data plane threads have stopped.\n+\t * the API doc mandates that the user stops all processing on affected\n+\t * ports before calling any of these API's, so we can assume that the\n+\t * callbacks can be freed. we're intentionally casting away const-ness.\n \t */\n-\tqueue_cfg->cur_cb = NULL;\n+\trte_free((void *)queue_cfg->cur_cb);\n \n \treturn 0;\n }\ndiff --git a/lib/power/rte_power_pmd_mgmt.h b/lib/power/rte_power_pmd_mgmt.h\nindex 7a0ac24625..444e7b8a66 100644\n--- a/lib/power/rte_power_pmd_mgmt.h\n+++ b/lib/power/rte_power_pmd_mgmt.h\n@@ -43,6 +43,9 @@ enum rte_power_pmd_mgmt_type {\n *\n * @note This function is not thread-safe.\n *\n+ * @warning This function must be called when all affected Ethernet queues are\n+ * stopped and no Rx/Tx is in progress!\n+ *\n * @param lcore_id\n * The lcore the Rx queue will be polled from.\n * @param port_id\n@@ -69,6 +72,9 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id,\n *\n * @note This function is not thread-safe.\n *\n+ * @warning This function must be called when all affected Ethernet queues are\n+ * stopped and no Rx/Tx is in progress!\n+ *\n * @param lcore_id\n * The lcore the Rx queue is polled from.\n * @param port_id\n", "prefixes": [ "v2", "4/7" ] }