[5/5] lib/stack: remove pop cas release ordering
Checks
Commit Message
Replace the store-release by relaxed for the CAS success at the end of
pop. Release isn't needed, because there is not write to data that need
to be synchronized.
The only preceding write is when the length is decreased, but the length
CAS loop already ensures the right synchronization.
The situation to avoid is when a thread sees the old length but the new
list, that doesn't have enough items for pop to success.
But the CAS success on length before the pop loop ensures any core reads
and updates the latest length, preventing this situation.
The store-release is also used to make sure that the items are read
before the head is updated, in order to prevent a core in pop to read an
incorrect value because another core rewrites it with push.
But this isn't needed, because items are read only when removed from the
used list. Right after this, they are pushed to the free list, and the
store-release in push makes sure the items are read before they are
visible in the free list.
Signed-off-by: Steven Lariau <steven.lariau@arm.com>
Reviewed-by: Dharmik Thakkar <dharmik.thakkar@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
lib/librte_stack/rte_stack_lf_c11.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
> -----Original Message-----
> From: Steven Lariau <steven.lariau@arm.com>
> Sent: Friday, September 11, 2020 10:30 AM
> To: Eads, Gage <gage.eads@intel.com>; Olivier Matz <olivier.matz@6wind.com>
> Cc: dev@dpdk.org; nd@arm.com; dharmik.thakkar@arm.com; Steven Lariau
> <steven.lariau@arm.com>
> Subject: [PATCH 5/5] lib/stack: remove pop cas release ordering
>
> Replace the store-release by relaxed for the CAS success at the end of
> pop. Release isn't needed, because there is not write to data that need
> to be synchronized.
> The only preceding write is when the length is decreased, but the length
> CAS loop already ensures the right synchronization.
> The situation to avoid is when a thread sees the old length but the new
> list, that doesn't have enough items for pop to success.
> But the CAS success on length before the pop loop ensures any core reads
> and updates the latest length, preventing this situation.
>
> The store-release is also used to make sure that the items are read
> before the head is updated, in order to prevent a core in pop to read an
> incorrect value because another core rewrites it with push.
> But this isn't needed, because items are read only when removed from the
> used list. Right after this, they are pushed to the free list, and the
> store-release in push makes sure the items are read before they are
> visible in the free list.
Please add a comment to this effect above the compare-exchange call. Depending
on this caller behavior for correctness is a little risky, but since this header
is private to the library I think it's ok as long as it's well-documented.
Thanks,
Gage
Hello Steven,
On Mon, Sep 21, 2020 at 7:17 PM Eads, Gage <gage.eads@intel.com> wrote:
> > -----Original Message-----
> > From: Steven Lariau <steven.lariau@arm.com>
> > Sent: Friday, September 11, 2020 10:30 AM
> > To: Eads, Gage <gage.eads@intel.com>; Olivier Matz <olivier.matz@6wind.com>
> > Cc: dev@dpdk.org; nd@arm.com; dharmik.thakkar@arm.com; Steven Lariau
> > <steven.lariau@arm.com>
> > Subject: [PATCH 5/5] lib/stack: remove pop cas release ordering
> >
> > Replace the store-release by relaxed for the CAS success at the end of
> > pop. Release isn't needed, because there is not write to data that need
> > to be synchronized.
> > The only preceding write is when the length is decreased, but the length
> > CAS loop already ensures the right synchronization.
> > The situation to avoid is when a thread sees the old length but the new
> > list, that doesn't have enough items for pop to success.
> > But the CAS success on length before the pop loop ensures any core reads
> > and updates the latest length, preventing this situation.
> >
> > The store-release is also used to make sure that the items are read
> > before the head is updated, in order to prevent a core in pop to read an
> > incorrect value because another core rewrites it with push.
> > But this isn't needed, because items are read only when removed from the
> > used list. Right after this, they are pushed to the free list, and the
> > store-release in push makes sure the items are read before they are
> > visible in the free list.
>
> Please add a comment to this effect above the compare-exchange call. Depending
> on this caller behavior for correctness is a little risky, but since this header
> is private to the library I think it's ok as long as it's well-documented.
>
Can you prepare a v2 for this comment?
Thanks.
@@ -145,7 +145,7 @@ __rte_stack_lf_pop_elems(struct rte_stack_lf_list *list,
(rte_int128_t *)&list->head,
(rte_int128_t *)&old_head,
(rte_int128_t *)&new_head,
- 0, __ATOMIC_RELEASE,
+ 0, __ATOMIC_RELAXED,
__ATOMIC_RELAXED);
} while (success == 0);