From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1CAE0A00C2 for ; Thu, 3 Nov 2022 10:31:51 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 185824069B; Thu, 3 Nov 2022 10:31:51 +0100 (CET) Received: from mail-wr1-f44.google.com (mail-wr1-f44.google.com [209.85.221.44]) by mails.dpdk.org (Postfix) with ESMTP id C817240693 for ; Thu, 3 Nov 2022 10:31:49 +0100 (CET) Received: by mail-wr1-f44.google.com with SMTP id bk15so1676558wrb.13 for ; Thu, 03 Nov 2022 02:31:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=B1hdDcz3uVp5W61IhBsQJFD2TOQzG4g9lh3E+Kmy8x8=; b=B7oE3rzAkgUzbqNmEXYmOAv22G+Nwo/iqhqGp+iaQ/ec3LSpynoZwPzmiWtD5kXZnm l1qQhuOQQMoXWFPZ14AW4QLLZFSnpNG2NPF821ZTp83yQChhfmRp6oSKHuiK7Hzdkj6L /IZYDp0wEX63QTLGro15TW3NSIYQt72P38hPyiZKX4u7CX5k6su8KVlTMD7Z3mqQW8g8 Dg4ITpJWq+kPKDJzc8UtpKt4fU7VwhIuvcoemJ+65Vvtg+mCN/mV6rzUidiYecGkSk2+ 5JdorpaB1BjrYUVxlQP8wVOQCPkxBsRlL5DOqRt/jJ8q612L2ojIQy/dfmN86V3OridB BIrA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=B1hdDcz3uVp5W61IhBsQJFD2TOQzG4g9lh3E+Kmy8x8=; b=scNGm58S/l5DXKsVxcqUSYKqh+5HJRsDhTMeA8GFAX2GQ8ss6T9dSyRjsJ9RpJhN+1 p+Zwtthrs7qnaJZelGsz4pOfd0SJsXcT9H/PJVS9hs9FhnfBFSZr/7+zNV+vhLEvDmaJ Id05uc8U2dqM8usJgE97stXO3SAYq0e9zxs7wSyy7bb+KX22va5GDLwC8f3T6oCOuJyR R3xkU5aqMiOlhTjXbfOjGiuA297+siV2abh3mFtg/8mfr8Iohd5+yCs7yaZmwxIN74vP FIfbwXhjPvCl9pcp4cpzRHDumaWRVztkLnbj3InKq6QjPdkqJc11YGaYeTBUXeRdFcUr /ojw== X-Gm-Message-State: ACrzQf2xDGSae1TIWj3cUPWIExZ7FBngoKj05U6megrBWE5+gdNlv0hm OOztwWH3OSCj7HHgk5sbsXAFntsZOBaV+BQF X-Google-Smtp-Source: AMsMyM7Pi60iEx1FUhXR4hlSbuF61jRH0fJXv4vtky5kB5oKrWGKGAFHfYY6WOiU1jY93rIcHcbM+A== X-Received: by 2002:adf:e30f:0:b0:236:d8ef:9ede with SMTP id b15-20020adfe30f000000b00236d8ef9edemr11741382wrj.170.1667467909492; Thu, 03 Nov 2022 02:31:49 -0700 (PDT) Received: from localhost ([2a01:4b00:d307:1000:f1d3:eb5e:11f4:a7d9]) by smtp.gmail.com with ESMTPSA id h8-20020a05600c314800b003a1980d55c4sm4800415wmo.47.2022.11.03.02.31.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 03 Nov 2022 02:31:48 -0700 (PDT) From: luca.boccassi@gmail.com To: Harry van Haaren Cc: =?UTF-8?q?Mattias=20R=C3=B6nnblom?= , Honnappa Nagarahalli , =?UTF-8?q?Morten=20Br=C3=B8rup?= , Bruce Richardson , dpdk stable Subject: patch 'service: fix stats race condition for MT safe service' has been queued to stable release 20.11.7 Date: Thu, 3 Nov 2022 09:27:24 +0000 Message-Id: <20221103092758.1099402-66-luca.boccassi@gmail.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20221103092758.1099402-1-luca.boccassi@gmail.com> References: <20221103092758.1099402-1-luca.boccassi@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Hi, FYI, your patch has been queued to stable release 20.11.7 Note it hasn't been pushed to http://dpdk.org/browse/dpdk-stable yet. It will be pushed if I get no objections before 11/05/22. So please shout if anyone has objections. Also note that after the patch there's a diff of the upstream commit vs the patch applied to the branch. This will indicate if there was any rebasing needed to apply to the stable branch. If there were code changes for rebasing (ie: not only metadata diffs), please double check that the rebase was correctly done. Queued patches are on a temporary branch at: https://github.com/kevintraynor/dpdk-stable This queued commit can be viewed at: https://github.com/kevintraynor/dpdk-stable/commit/027b9bcee35794f6cf27ad8399fbcbaf37d9b2c0 Thanks. Luca Boccassi --- >From 027b9bcee35794f6cf27ad8399fbcbaf37d9b2c0 Mon Sep 17 00:00:00 2001 From: Harry van Haaren Date: Mon, 11 Jul 2022 13:18:25 +0000 Subject: [PATCH] service: fix stats race condition for MT safe service MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [ upstream commit 99e4e840479ab44ffb35177e22c9999fec64b848 ] 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. Fixes: 21698354c832 ("service: introduce service cores concept") Reported-by: Mattias Rönnblom Suggested-by: Honnappa Nagarahalli Suggested-by: Morten Brørup Suggested-by: Bruce Richardson Signed-off-by: Harry van Haaren --- lib/librte_eal/common/rte_service.c | 31 +++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c index e76c2baffc..d5e3574ec7 100644 --- a/lib/librte_eal/common/rte_service.c +++ b/lib/librte_eal/common/rte_service.c @@ -56,10 +56,17 @@ struct rte_service_spec_impl { * on currently. */ uint32_t num_mapped_cores; - uint64_t calls; - uint64_t cycles_spent; + + /* 32-bit builds won't naturally align a uint64_t, so force alignment, + * allowing regular reads to be atomic. + */ + uint64_t calls __rte_aligned(8); + uint64_t cycles_spent __rte_aligned(8); } __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 */ @@ -365,13 +372,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); } -- 2.34.1 --- Diff of the applied patch vs upstream commit (please double-check if non-empty: --- --- - 2022-11-03 09:27:29.365045661 +0000 +++ 0066-service-fix-stats-race-condition-for-MT-safe-service.patch 2022-11-03 09:27:25.485424608 +0000 @@ -1 +1 @@ -From 99e4e840479ab44ffb35177e22c9999fec64b848 Mon Sep 17 00:00:00 2001 +From 027b9bcee35794f6cf27ad8399fbcbaf37d9b2c0 Mon Sep 17 00:00:00 2001 @@ -8,0 +9,2 @@ +[ upstream commit 99e4e840479ab44ffb35177e22c9999fec64b848 ] + @@ -35 +37 @@ - lib/eal/common/rte_service.c | 31 +++++++++++++++++++++++++++---- + lib/librte_eal/common/rte_service.c | 31 +++++++++++++++++++++++++---- @@ -38,5 +40,5 @@ -diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c -index d2b7275ac0..94cb056196 100644 ---- a/lib/eal/common/rte_service.c -+++ b/lib/eal/common/rte_service.c -@@ -50,10 +50,17 @@ struct rte_service_spec_impl { +diff --git a/lib/librte_eal/common/rte_service.c b/lib/librte_eal/common/rte_service.c +index e76c2baffc..d5e3574ec7 100644 +--- a/lib/librte_eal/common/rte_service.c ++++ b/lib/librte_eal/common/rte_service.c +@@ -56,10 +56,17 @@ struct rte_service_spec_impl { @@ -62 +64 @@ -@@ -359,13 +366,29 @@ service_runner_do_callback(struct rte_service_spec_impl *s, +@@ -365,13 +372,29 @@ service_runner_do_callback(struct rte_service_spec_impl *s,