From patchwork Mon Jul 11 10:57:46 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Van Haaren, Harry" X-Patchwork-Id: 113896 X-Patchwork-Delegate: thomas@monjalon.net Return-Path: 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]) by inbox.dpdk.org (Postfix) with ESMTP id 84604A0032; Mon, 11 Jul 2022 12:57:56 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2404540695; Mon, 11 Jul 2022 12:57:56 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mails.dpdk.org (Postfix) with ESMTP id 953F140223 for ; Mon, 11 Jul 2022 12:57:54 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1657537074; x=1689073074; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=ZApIfADsRhivycTOkLerqw9rsmDtlNURwNLVcLQRiaA=; b=bCOTyhC2Zk0K8J79zBUpz+Uagc2jx13yuqpRr4mTNjCTJsqBE2EtydLH tOWhe3v8gG98IPd33WrxLRwotrXFnYL92VLBF6bOI3ICO9+7S2rKrRudb bZ0JpcwfBk0C+SurnL9NhNZGeXPi7IR8eThBCHqRR0uAWi+q6VIcjNotL Uzrzt1EozgQsmcV9cwPW4cMaXuSYywNy1i6+1ah3D+vnOHcOg/sm0orF2 9zhFLbLg+mKaTazN5Umjl7VzPB1tB1pQYimqNXjPSgImdjGE7gO0prqaT Pa/BwOXqoVTpTMMtDgE3hENB1p1nKib/nDpLt87tYKWBXnuZKM/1cbfSL A==; X-IronPort-AV: E=McAfee;i="6400,9594,10404"; a="264409400" X-IronPort-AV: E=Sophos;i="5.92,262,1650956400"; d="scan'208";a="264409400" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jul 2022 03:57:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.92,262,1650956400"; d="scan'208";a="662503002" Received: from silpixa00401454.ir.intel.com ([10.55.128.122]) by fmsmga004.fm.intel.com with ESMTP; 11 Jul 2022 03:57:51 -0700 From: Harry van Haaren To: dev@dpdk.org Cc: Harry van Haaren , =?utf-8?q?Mattias_R=C3=B6?= =?utf-8?q?nnblom?= , Honnappa Nagarahalli , =?utf-8?q?Morten_Br?= =?utf-8?q?=C3=B8rup?= Subject: [PATCH v2 1/2] test/service: add perf measurements for with stats mode Date: Mon, 11 Jul 2022 10:57:46 +0000 Message-Id: <20220711105747.3295201-1-harry.van.haaren@intel.com> X-Mailer: git-send-email 2.32.0 MIME-Version: 1.0 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org This commit improves the performance reporting of the service cores polling loop to show both with and without statistics collection modes. Collecting cycle statistics is costly, due to calls to rte_rdtsc() per service iteration. Reported-by: Mattias Rönnblom Suggested-by: Honnappa Nagarahalli Suggested-by: Morten Brørup Signed-off-by: Harry van Haaren --- This is split out as a seperate patch from the fix to allow measuring the before/after of the service stats atomic fixup. --- app/test/test_service_cores.c | 36 ++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/app/test/test_service_cores.c b/app/test/test_service_cores.c index ced6ed0081..7415b6b686 100644 --- a/app/test/test_service_cores.c +++ b/app/test/test_service_cores.c @@ -777,6 +777,22 @@ service_run_on_app_core_func(void *arg) return rte_service_run_iter_on_app_lcore(*delay_service_id, 1); } +static float +service_app_lcore_perf_measure(uint32_t id) +{ + /* Performance test: call in a loop, and measure tsc() */ + const uint32_t perf_iters = (1 << 12); + uint64_t start = rte_rdtsc(); + uint32_t i; + for (i = 0; i < perf_iters; i++) { + int err = service_run_on_app_core_func(&id); + TEST_ASSERT_EQUAL(0, err, "perf test: returned run failure"); + } + uint64_t end = rte_rdtsc(); + + return (end - start)/(float)perf_iters; +} + static int service_app_lcore_poll_impl(const int mt_safe) { @@ -828,17 +844,15 @@ service_app_lcore_poll_impl(const int mt_safe) "MT Unsafe: App core1 didn't return -EBUSY"); } - /* Performance test: call in a loop, and measure tsc() */ - const uint32_t perf_iters = (1 << 12); - uint64_t start = rte_rdtsc(); - uint32_t i; - for (i = 0; i < perf_iters; i++) { - int err = service_run_on_app_core_func(&id); - TEST_ASSERT_EQUAL(0, err, "perf test: returned run failure"); - } - uint64_t end = rte_rdtsc(); - printf("perf test for %s: %0.1f cycles per call\n", mt_safe ? - "MT Safe" : "MT Unsafe", (end - start)/(float)perf_iters); + /* Measure performance of no-stats and with-stats. */ + float cyc_no_stats = service_app_lcore_perf_measure(id); + + TEST_ASSERT_EQUAL(0, rte_service_set_stats_enable(id, 1), + "failed to enable stats for service."); + float cyc_with_stats = service_app_lcore_perf_measure(id); + + printf("perf test for %s, no stats: %0.1f, with stats %0.1f cycles/call\n", + mt_safe ? "MT Safe" : "MT Unsafe", cyc_no_stats, cyc_with_stats); unregister_all(); return TEST_SUCCESS; From patchwork Mon Jul 11 10:57:47 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Van Haaren, Harry" X-Patchwork-Id: 113897 X-Patchwork-Delegate: thomas@monjalon.net Return-Path: 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]) by inbox.dpdk.org (Postfix) with ESMTP id 10985A0032; Mon, 11 Jul 2022 12:58:00 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1295041156; Mon, 11 Jul 2022 12:57:57 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mails.dpdk.org (Postfix) with ESMTP id D72ED40223 for ; Mon, 11 Jul 2022 12:57:55 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1657537076; x=1689073076; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=vrNSfATQ8g5a3Z6JCnQiPS33ZjBS87tzjkCFljxwlW0=; b=Y4g3Ln7NuSMLJWAHMUdz3T7wc0eAN1DaGffaGl6gl5cpEseHgwKMnd5l VJmEIzGkPCDJIJ6ZTUslCd6Fn878wjmnKlp8sZF3C/C+TIx8ZPLJrQMU6 8w0zCNHQE7HbhEPeL1Q/ZamJRFyFgZYpSWcy0ykl74QZhcthJHax7UKyJ q676yiS8saDkoI98pBE8sVOl1YcszAmS7adqs/7hblXn9Ef+oRFi6eI4P EaGDEbORFxz40inbqmPxYBoZM//iyGLM12WqmIoNRar8UvrPdRa2InE7u key09Cvjdy53c4Tt7S2G8F21/QZNfgwv+eOWEJDCpYPcMRi5oCvUOkSkE A==; X-IronPort-AV: E=McAfee;i="6400,9594,10404"; a="264409404" X-IronPort-AV: E=Sophos;i="5.92,262,1650956400"; d="scan'208";a="264409404" Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Jul 2022 03:57:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.92,262,1650956400"; d="scan'208";a="662503011" Received: from silpixa00401454.ir.intel.com ([10.55.128.122]) by fmsmga004.fm.intel.com with ESMTP; 11 Jul 2022 03:57:53 -0700 From: Harry van Haaren To: dev@dpdk.org Cc: Harry van Haaren , =?utf-8?q?Mattias_R=C3=B6?= =?utf-8?q?nnblom?= , Honnappa Nagarahalli , =?utf-8?q?Morten_Br?= =?utf-8?q?=C3=B8rup?= , Bruce Richardson Subject: [PATCH v2 2/2] service: fix potential stats race-condition on MT services Date: Mon, 11 Jul 2022 10:57:47 +0000 Message-Id: <20220711105747.3295201-2-harry.van.haaren@intel.com> X-Mailer: git-send-email 2.32.0 In-Reply-To: <20220711105747.3295201-1-harry.van.haaren@intel.com> References: <20220711105747.3295201-1-harry.van.haaren@intel.com> MIME-Version: 1.0 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org This commit fixes a potential racey-add that could occur if multiple service-lcores were executing the same MT-safe service at the same time, with service statistics collection enabled. Because multiple threads can run and execute the service, the stats values can have multiple writer threads, resulting in the requirement of using atomic addition for correctness. Note that when a MT unsafe service is executed, a spinlock is held, so the stats increments are protected. This fact is used to avoid executing atomic add instructions when not required. Regular reads and increments are used, and only the store is specified as atomic, reducing perf impact on e.g. x86 arch. This patch causes a 1.25x increase in cycle-cost for polling a MT safe service when statistics are enabled. No change was seen for MT unsafe services, or when statistics are disabled. Reported-by: Mattias Rönnblom Suggested-by: Honnappa Nagarahalli Suggested-by: Morten Brørup Suggested-by: Bruce Richardson Signed-off-by: Harry van Haaren --- v2 (Thanks Honnappa, Morten, Bruce & Mattias for discussion): - Improved handling of stat stores to ensure they're atomic by using __atomic_store_n() with regular loads/increments. - Added BUILD_BUG_ON alignment checks for the uint64_t stats variables, tested with __rte_packed to ensure build breaks if not aligned naturally. --- lib/eal/common/rte_service.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c index d2b7275ac0..90d12032f0 100644 --- a/lib/eal/common/rte_service.c +++ b/lib/eal/common/rte_service.c @@ -54,6 +54,9 @@ struct rte_service_spec_impl { uint64_t cycles_spent; } __rte_cache_aligned; +/* Mask used to ensure uint64_t 8 byte vars are naturally aligned. */ +#define RTE_SERVICE_STAT_ALIGN_MASK (8 - 1) + /* the internal values of a service core */ struct core_state { /* map of services IDs are run on this core */ @@ -359,13 +362,29 @@ service_runner_do_callback(struct rte_service_spec_impl *s, { void *userdata = s->spec.callback_userdata; + /* Ensure the atomically stored variables are naturally aligned, + * as required for regular loads to be atomic. + */ + RTE_BUILD_BUG_ON((offsetof(struct rte_service_spec_impl, calls) + & RTE_SERVICE_STAT_ALIGN_MASK) != 0); + RTE_BUILD_BUG_ON((offsetof(struct rte_service_spec_impl, cycles_spent) + & RTE_SERVICE_STAT_ALIGN_MASK) != 0); + if (service_stats_enabled(s)) { uint64_t start = rte_rdtsc(); s->spec.callback(userdata); uint64_t end = rte_rdtsc(); - s->cycles_spent += end - start; + uint64_t cycles = end - start; cs->calls_per_service[service_idx]++; - s->calls++; + if (service_mt_safe(s)) { + __atomic_fetch_add(&s->cycles_spent, cycles, __ATOMIC_RELAXED); + __atomic_fetch_add(&s->calls, 1, __ATOMIC_RELAXED); + } else { + uint64_t cycles_new = s->cycles_spent + cycles; + uint64_t calls_new = s->calls++; + __atomic_store_n(&s->cycles_spent, cycles_new, __ATOMIC_RELAXED); + __atomic_store_n(&s->calls, calls_new, __ATOMIC_RELAXED); + } } else s->spec.callback(userdata); }