Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/113901/?format=api
{ "id": 113901, "url": "http://patches.dpdk.org/api/patches/113901/?format=api", "web_url": "http://patches.dpdk.org/project/dpdk/patch/20220711131825.3373195-2-harry.van.haaren@intel.com/", "project": { "id": 1, "url": "http://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": "<20220711131825.3373195-2-harry.van.haaren@intel.com>", "list_archive_url": "https://inbox.dpdk.org/dev/20220711131825.3373195-2-harry.van.haaren@intel.com", "date": "2022-07-11T13:18:25", "name": "[v3,2/2] service: fix potential stats race-condition on MT services", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": true, "hash": "0db4f012df478ab00f8d9ad6c352c6effcc48a92", "submitter": { "id": 317, "url": "http://patches.dpdk.org/api/people/317/?format=api", "name": "Van Haaren, Harry", "email": "harry.van.haaren@intel.com" }, "delegate": { "id": 24651, "url": "http://patches.dpdk.org/api/users/24651/?format=api", "username": "dmarchand", "first_name": "David", "last_name": "Marchand", "email": "david.marchand@redhat.com" }, "mbox": "http://patches.dpdk.org/project/dpdk/patch/20220711131825.3373195-2-harry.van.haaren@intel.com/mbox/", "series": [ { "id": 23959, "url": "http://patches.dpdk.org/api/series/23959/?format=api", "web_url": "http://patches.dpdk.org/project/dpdk/list/?series=23959", "date": "2022-07-11T13:18:24", "name": "[v3,1/2] test/service: add perf measurements for with stats mode", "version": 3, "mbox": "http://patches.dpdk.org/series/23959/mbox/" } ], "comments": "http://patches.dpdk.org/api/patches/113901/comments/", "check": "warning", "checks": "http://patches.dpdk.org/api/patches/113901/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 527F9A0032;\n\tMon, 11 Jul 2022 15:18:41 +0200 (CEST)", "from [217.70.189.124] (localhost [127.0.0.1])\n\tby mails.dpdk.org (Postfix) with ESMTP id A699B4280D;\n\tMon, 11 Jul 2022 15:18:40 +0200 (CEST)", "from mga18.intel.com (mga18.intel.com [134.134.136.126])\n by mails.dpdk.org (Postfix) with ESMTP id 6891541614\n for <dev@dpdk.org>; Mon, 11 Jul 2022 15:18:37 +0200 (CEST)", "from fmsmga005.fm.intel.com ([10.253.24.32])\n by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n 11 Jul 2022 06:18:36 -0700", "from silpixa00401454.ir.intel.com ([10.55.128.122])\n by fmsmga005.fm.intel.com with ESMTP; 11 Jul 2022 06:18:35 -0700" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple;\n d=intel.com; i=@intel.com; q=dns/txt; s=Intel;\n t=1657545517; x=1689081517;\n h=from:to:cc:subject:date:message-id:in-reply-to:\n references:mime-version:content-transfer-encoding;\n bh=S1oRdNn8qoOHe+VI4mW6ATcl9w2lcuWeHPT1Vm9JdgU=;\n b=BNOEkKeFksZAnvbnxSpnoGZVCmQW6i0QEx4xPLkcixjjnEhiox/Gbh4m\n BYNeMYt+SZwqfy58YhzxbX8xWoZcKnCRP6qQMCkahK4fgJchfYHtl6hLT\n /ayF67d7YssOtfD21jnek3xsf73KuDern+PX5R3Z6fQhIdGVqggdE280u\n tie45TzmN0Pv/HcJrjIrtOnTQtvq3fteCzEXGHevm0TE/Fx+rCCzMbXwy\n Q3hchFwXBqMjwNiYTvmc98OEK3egoF30CpkRW4S8lKh4mPzYkpW3aG6CU\n 3HpUPdWFk8ppxLDtlaTPZkfACHl4c9pV2MPLpC05CEY0jj5qDg7sqyfMF Q==;", "X-IronPort-AV": [ "E=McAfee;i=\"6400,9594,10404\"; a=\"267697822\"", "E=Sophos;i=\"5.92,262,1650956400\"; d=\"scan'208\";a=\"267697822\"", "E=Sophos;i=\"5.92,262,1650956400\"; d=\"scan'208\";a=\"921778882\"" ], "X-ExtLoop1": "1", "From": "Harry van Haaren <harry.van.haaren@intel.com>", "To": "dev@dpdk.org", "Cc": "Harry van Haaren <harry.van.haaren@intel.com>, =?utf-8?q?Mattias_R=C3=B6?=\n\t=?utf-8?q?nnblom?= <mattias.ronnblom@ericsson.com>,\n Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>, =?utf-8?q?Morten_Br?=\n\t=?utf-8?q?=C3=B8rup?= <mb@smartsharesystems.com>,\n Bruce Richardson <bruce.richardson@intel.com>", "Subject": "[PATCH v3 2/2] service: fix potential stats race-condition on MT\n services", "Date": "Mon, 11 Jul 2022 13:18:25 +0000", "Message-Id": "<20220711131825.3373195-2-harry.van.haaren@intel.com>", "X-Mailer": "git-send-email 2.32.0", "In-Reply-To": "<20220711131825.3373195-1-harry.van.haaren@intel.com>", "References": "<20220711105747.3295201-1-harry.van.haaren@intel.com>\n <20220711131825.3373195-1-harry.van.haaren@intel.com>", "MIME-Version": "1.0", "Content-Type": "text/plain; charset=UTF-8", "Content-Transfer-Encoding": "8bit", "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" }, "content": "This commit fixes a potential racey-add that could occur if\nmultiple service-lcores were executing the same MT-safe service\nat the same time, with service statistics collection enabled.\n\nBecause multiple threads can run and execute the service, the\nstats values can have multiple writer threads, resulting in the\nrequirement of using atomic addition for correctness.\n\nNote that when a MT unsafe service is executed, a spinlock is\nheld, so the stats increments are protected. This fact is used\nto avoid executing atomic add instructions when not required.\nRegular reads and increments are used, and only the store is\nspecified as atomic, reducing perf impact on e.g. x86 arch.\n\nThis patch causes a 1.25x increase in cycle-cost for polling a\nMT safe service when statistics are enabled. No change was seen\nfor MT unsafe services, or when statistics are disabled.\n\nReported-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>\nSuggested-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>\nSuggested-by: Morten Brørup <mb@smartsharesystems.com>\nSuggested-by: Bruce Richardson <bruce.richardson@intel.com>\nSigned-off-by: Harry van Haaren <harry.van.haaren@intel.com>\n\n---\n\nv3:\n- Fix 32-bit build, by forcing natural alignment of uint64_t in\n the struct that contains it, using __rte_aligned(8) macro.\n- Note: I'm seeing a checkpatch \"avoid externs in .c files\" warning,\n but it doesn't make sense to me, so perhaps its a false-positive..?\n\nv2 (Thanks Honnappa, Morten, Bruce & Mattias for discussion):\n- Improved handling of stat stores to ensure they're atomic by\n using __atomic_store_n() with regular loads/increments.\n- Added BUILD_BUG_ON alignment checks for the uint64_t stats\n variables, tested with __rte_packed to ensure build breaks.\n---\n lib/eal/common/rte_service.c | 31 +++++++++++++++++++++++++++----\n 1 file changed, 27 insertions(+), 4 deletions(-)", "diff": "diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c\nindex d2b7275ac0..94cb056196 100644\n--- a/lib/eal/common/rte_service.c\n+++ b/lib/eal/common/rte_service.c\n@@ -50,10 +50,17 @@ struct rte_service_spec_impl {\n \t * on currently.\n \t */\n \tuint32_t num_mapped_cores;\n-\tuint64_t calls;\n-\tuint64_t cycles_spent;\n+\n+\t/* 32-bit builds won't naturally align a uint64_t, so force alignment,\n+\t * allowing regular reads to be atomic.\n+\t */\n+\tuint64_t calls __rte_aligned(8);\n+\tuint64_t cycles_spent __rte_aligned(8);\n } __rte_cache_aligned;\n \n+/* Mask used to ensure uint64_t 8 byte vars are naturally aligned. */\n+#define RTE_SERVICE_STAT_ALIGN_MASK (8 - 1)\n+\n /* the internal values of a service core */\n struct core_state {\n \t/* map of services IDs are run on this core */\n@@ -359,13 +366,29 @@ service_runner_do_callback(struct rte_service_spec_impl *s,\n {\n \tvoid *userdata = s->spec.callback_userdata;\n \n+\t/* Ensure the atomically stored variables are naturally aligned,\n+\t * as required for regular loads to be atomic.\n+\t */\n+\tRTE_BUILD_BUG_ON((offsetof(struct rte_service_spec_impl, calls)\n+\t\t& RTE_SERVICE_STAT_ALIGN_MASK) != 0);\n+\tRTE_BUILD_BUG_ON((offsetof(struct rte_service_spec_impl, cycles_spent)\n+\t\t& RTE_SERVICE_STAT_ALIGN_MASK) != 0);\n+\n \tif (service_stats_enabled(s)) {\n \t\tuint64_t start = rte_rdtsc();\n \t\ts->spec.callback(userdata);\n \t\tuint64_t end = rte_rdtsc();\n-\t\ts->cycles_spent += end - start;\n+\t\tuint64_t cycles = end - start;\n \t\tcs->calls_per_service[service_idx]++;\n-\t\ts->calls++;\n+\t\tif (service_mt_safe(s)) {\n+\t\t\t__atomic_fetch_add(&s->cycles_spent, cycles, __ATOMIC_RELAXED);\n+\t\t\t__atomic_fetch_add(&s->calls, 1, __ATOMIC_RELAXED);\n+\t\t} else {\n+\t\t\tuint64_t cycles_new = s->cycles_spent + cycles;\n+\t\t\tuint64_t calls_new = s->calls++;\n+\t\t\t__atomic_store_n(&s->cycles_spent, cycles_new, __ATOMIC_RELAXED);\n+\t\t\t__atomic_store_n(&s->calls, calls_new, __ATOMIC_RELAXED);\n+\t\t}\n \t} else\n \t\ts->spec.callback(userdata);\n }\n", "prefixes": [ "v3", "2/2" ] }