Re: [PATCH 1/1] ring: safe partial ordering for head/tail update

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 17 Sep 2025 08:47:53 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Tue, Sep 16, 2025 at 06:19:41PM +0000, Ola Liljedahl wrote:
> > (I am sending this from Outlook, I hope I have been able to fake a proper
> > email client)
> >   
> > > On 2025-09-16, 17:43, "Bruce Richardson" <bruce.richardson@intel.com > <mailto:bruce.richardson@intel.com>> wrote:
> > >
> > >
> > > On Mon, Sep 15, 2025 at 06:54:50PM +0000, Wathsala Vithanage wrote:  
> > > > 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 (3) for performance reasons.
> > > >
> > > > Signed-off-by: Wathsala Vithanage <wathsala.vithanage@arm.com <mailto:wathsala.vithanage@arm.com>>
> > > > Signed-off-by: Ola Liljedahl <ola.liljedahl@arm.com <mailto:ola.liljedahl@arm.com>>
> > > > Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com <mailto:honnappa.nagarahalli@arm.com>>
> > > > Reviewed-by: Dhruv Tripathi <dhruv.tripathi@arm.com <mailto:dhruv.tripathi@arm.com>>
> > > > ---
> > > > lib/ring/rte_ring_c11_pvt.h | 10 +++++++---
> > > > 1 file changed, 7 insertions(+), 3 deletions(-)
> > > >  
> > > Thank you for the very comprehensive write-up in the article about this.
> > > It was very educational.
> > >
> > >
> > > On the patch, are we sure that option #3 is safe to take as an approach? It
> > > seems wrong to me to deliberately leave ordering issues in the code and
> > > just try and fix them up later. Would there be a noticable performance
> > > difference for real-world apps if we took option #2, and actually used
> > > correct ordering semantics?  
> > I am pretty sure that all necessary orderings for safely transferring elements
> > from producers to consumers (and empty slots from consumers to producers)
> > are present in the code (I still have some questions on the use of memory
> > order relaxed in __rte_ring_update_tail, we should create a litmus test for
> > this, to see what is required by the C memory model). What other metrics
> > for correctness do you suggest?
> >  
> 
> Not suggesting any other metrics for correctness. I'm instead just wanting
> to double-check the cost-benefit of taking the approach of putting in a
> fix-up, or workaround, in the code here, rather than actually correcting
> the memory ordering use.  Also, given that the workaround uses subtraction
> to detect underflow, are we 100% sure that we have guaranteed correct
> behaviour on counter wraparound at uint32_t max?

If you look at the code rabbit review demo, it flagged the same possible
underflow issue.