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 AFAFD48AF1 for ; Wed, 12 Nov 2025 20:12:33 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A89D340151; Wed, 12 Nov 2025 20:12:33 +0100 (CET) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id 2A6C840151 for ; Wed, 12 Nov 2025 20:12:32 +0100 (CET) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id CBFC11515; Wed, 12 Nov 2025 11:12:23 -0800 (PST) Received: from [10.122.38.161] (unknown [10.122.38.161]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 45A2E3F66E; Wed, 12 Nov 2025 11:12:31 -0800 (PST) Message-ID: <4f026a78-346e-4504-926a-24411dce7e53@arm.com> Date: Wed, 12 Nov 2025 13:12:30 -0600 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: patch 'ring: establish safe partial order in default mode' has been queued to stable release 22.11.11 To: luca.boccassi@gmail.com Cc: Ola Liljedahl , Honnappa Nagarahalli , Dhruv Tripathi , Konstantin Ananyev , dpdk stable References: <20251027162001.3710450-79-luca.boccassi@gmail.com> <20251112165308.1618107-1-luca.boccassi@gmail.com> <20251112165308.1618107-48-luca.boccassi@gmail.com> Content-Language: en-US From: Wathsala Vithanage In-Reply-To: <20251112165308.1618107-48-luca.boccassi@gmail.com> Content-Type: text/plain; charset=UTF-8; format=flowed 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 Luca, On 11/12/25 10:53, luca.boccassi@gmail.com wrote: > Hi, > > FYI, your patch has been queued to stable release 22.11.11 > > 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/14/25. So please > shout if anyone has objections. > Looked like it needed some work, so I just posted a patch. Let me know if you need any help with RTS and HTS patches as well. Thanks --wathsala > 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/bluca/dpdk-stable > > This queued commit can be viewed at: > https://github.com/bluca/dpdk-stable/commit/8e64e64659fe628f6b7ce903b67a6c8d271da524 > > Thanks. > > Luca Boccassi > > --- > From 8e64e64659fe628f6b7ce903b67a6c8d271da524 Mon Sep 17 00:00:00 2001 > From: Wathsala Vithanage > Date: Tue, 11 Nov 2025 18:37:17 +0000 > Subject: [PATCH] ring: establish safe partial order in default mode > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > [ upstream commit a4ad0eba9def1d1d071da8afe5e96eb2a2e0d71f ] > > The function __rte_ring_headtail_move_head() assumes that the barrier > (fence) between the load of the head and the load-acquire of the > opposing tail guarantees the following: if a first thread reads tail > and then writes head and a second thread reads the new value of head > and then reads tail, then it should observe the same (or a later) > value of tail. > > This assumption is incorrect under the C11 memory model. If the barrier > (fence) is intended to establish a total ordering of ring operations, > it fails to do so. Instead, the current implementation only enforces a > partial ordering, which can lead to unsafe interleavings. In particular, > some partial orders can cause underflows in free slot or available > element computations, potentially resulting in data corruption. > > The issue manifests when a CPU first acts as a producer and later as a > consumer. In this scenario, the barrier assumption may fail when another > core takes the consumer role. A Herd7 litmus test in C11 can demonstrate > this violation. The problem has not been widely observed so far because: > (a) on strong memory models (e.g., x86-64) the assumption holds, and > (b) on relaxed models with RCsc semantics the ordering is still strong > enough to prevent hazards. > The problem becomes visible only on weaker models, when load-acquire is > implemented with RCpc semantics (e.g. some AArch64 CPUs which support > the LDAPR and LDAPUR instructions). > > Three possible solutions exist: > 1. Strengthen ordering by upgrading release/acquire semantics to > sequential consistency. This requires using seq-cst for stores, > loads, and CAS operations. However, this approach introduces a > significant performance penalty on relaxed-memory architectures. > > 2. Establish a safe partial order by enforcing a pair-wise > happens-before relationship between thread of same role by changing > the CAS and the preceding load of the head by converting them to > release and acquire respectively. This approach makes the original > barrier assumption unnecessary and allows its removal. > > 3. Retain partial ordering but ensure only safe partial orders are > committed. This can be done by detecting underflow conditions > (producer < consumer) and quashing the update in such cases. > This approach makes the original barrier assumption unnecessary > and allows its removal. > > This patch implements solution (2) to preserve the “enqueue always > succeeds” contract expected by dependent libraries (e.g., mempool). > While solution (3) offers higher performance, adopting it now would > break that assumption. > > Fixes: 49594a63147a9 ("ring/c11: relax ordering for load and store of the head") > > Signed-off-by: Wathsala Vithanage > Signed-off-by: Ola Liljedahl > Reviewed-by: Honnappa Nagarahalli > Reviewed-by: Dhruv Tripathi > Acked-by: Konstantin Ananyev > Tested-by: Konstantin Ananyev > --- > lib/ring/rte_ring_c11_pvt.h | 37 +++++++++++++++++++++++++++++-------- > 1 file changed, 29 insertions(+), 8 deletions(-) > > diff --git a/lib/ring/rte_ring_c11_pvt.h b/lib/ring/rte_ring_c11_pvt.h > index f895950df4..5c04a001e1 100644 > --- a/lib/ring/rte_ring_c11_pvt.h > +++ b/lib/ring/rte_ring_c11_pvt.h > @@ -24,6 +24,11 @@ __rte_ring_update_tail(struct rte_ring_headtail *ht, uint32_t old_val, > if (!single) > rte_wait_until_equal_32(&ht->tail, old_val, __ATOMIC_RELAXED); > > + /* > + * R0: Establishes a synchronizing edge with load-acquire of tail at A1. > + * Ensures that memory effects by this thread on ring elements array > + * is observed by a different thread of the other type. > + */ > __atomic_store_n(&ht->tail, new_val, __ATOMIC_RELEASE); > } > > @@ -61,16 +66,23 @@ __rte_ring_move_prod_head(struct rte_ring *r, unsigned int is_sp, > unsigned int max = n; > int success; > > - *old_head = __atomic_load_n(&r->prod.head, __ATOMIC_RELAXED); > + /* > + * A0: Establishes a synchronizing edge with R1. > + * Ensure that this thread observes same values > + * to stail observed by the thread that updated > + * d->head. > + * If not, an unsafe partial order may ensue. > + */ > + *old_head = __atomic_load_n(&r->prod.head, __ATOMIC_ACQUIRE); > do { > /* Reset n to the initial burst count */ > n = max; > > - /* Ensure the head is read before tail */ > - __atomic_thread_fence(__ATOMIC_ACQUIRE); > - > - /* load-acquire synchronize with store-release of ht->tail > - * in update_tail. > + /* > + * A1: Establishes a synchronizing edge with R0. > + * Ensures that other thread's memory effects on > + * ring elements array is observed by the time > + * this thread observes its tail update. > */ > cons_tail = __atomic_load_n(&r->cons.tail, > __ATOMIC_ACQUIRE); > @@ -170,10 +182,19 @@ __rte_ring_move_cons_head(struct rte_ring *r, int is_sc, > r->cons.head = *new_head, success = 1; > else > /* on failure, *old_head will be updated */ > + /* > + * R1/A2. > + * R1: Establishes a synchronizing edge with A0 of a > + * different thread. > + * A2: Establishes a synchronizing edge with R1 of a > + * different thread to observe same value for stail > + * observed by that thread on CAS failure (to retry > + * with an updated *old_head). > + */ > success = __atomic_compare_exchange_n(&r->cons.head, > old_head, *new_head, > - 0, __ATOMIC_RELAXED, > - __ATOMIC_RELAXED); > + 0, __ATOMIC_RELEASE, > + __ATOMIC_ACQUIRE); > } while (unlikely(success == 0)); > return n; > }