DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: mattias.ronnblom <mattias.ronnblom@ericsson.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: Thomas Monjalon <thomas@monjalon.net>,
	David Marchand <david.marchand@redhat.com>,
	"Olsen, Onar" <onar.olsen@ericsson.com>,
	"Honnappa.Nagarahalli@arm.com" <Honnappa.Nagarahalli@arm.com>,
	"nd@arm.com" <nd@arm.com>,
	"mb@smartsharesystems.com" <mb@smartsharesystems.com>,
	"stephen@networkplumber.org" <stephen@networkplumber.org>,
	Ola Liljedahl <ola.liljedahl@arm.com>
Subject: RE: [RFC] eal: add seqlock
Date: Mon, 28 Mar 2022 10:53:34 +0000	[thread overview]
Message-ID: <DM6PR11MB4491FDC957C312689FD633169A1D9@DM6PR11MB4491.namprd11.prod.outlook.com> (raw)
In-Reply-To: <77168168-eadb-9a0b-b51e-9ccdf8ad7230@ericsson.com>


> >> diff --git a/lib/eal/include/meson.build b/lib/eal/include/meson.build
> >> index 9700494816..48df5f1a21 100644
> >> --- a/lib/eal/include/meson.build
> >> +++ b/lib/eal/include/meson.build
> >> @@ -36,6 +36,7 @@ headers += files(
> >>           'rte_per_lcore.h',
> >>           'rte_random.h',
> >>           'rte_reciprocal.h',
> >> +        'rte_seqlock.h',
> >>           'rte_service.h',
> >>           'rte_service_component.h',
> >>           'rte_string_fns.h',
> >> diff --git a/lib/eal/include/rte_seqlock.h b/lib/eal/include/rte_seqlock.h
> >> new file mode 100644
> >> index 0000000000..b975ca848a
> >> --- /dev/null
> >> +++ b/lib/eal/include/rte_seqlock.h
> >> @@ -0,0 +1,84 @@
> >> +/* SPDX-License-Identifier: BSD-3-Clause
> >> + * Copyright(c) 2022 Ericsson AB
> >> + */
> >> +
> >> +#ifndef _RTE_SEQLOCK_H_
> >> +#define _RTE_SEQLOCK_H_
> >> +
> >> +#include <stdbool.h>
> >> +#include <stdint.h>
> >> +
> >> +#include <rte_atomic.h>
> >> +#include <rte_branch_prediction.h>
> >> +#include <rte_spinlock.h>
> >> +
> >> +struct rte_seqlock {
> >> +	uint64_t sn;
> >> +	rte_spinlock_t lock;
> >> +};
> >> +
> >> +typedef struct rte_seqlock rte_seqlock_t;
> >> +
> >> +__rte_experimental
> >> +void
> >> +rte_seqlock_init(rte_seqlock_t *seqlock);
> > Probably worth to have static initializer too.
> >
> 
> I will add that in the next version, thanks.
> 
> >> +
> >> +__rte_experimental
> >> +static inline uint64_t
> >> +rte_seqlock_read_begin(const rte_seqlock_t *seqlock)
> >> +{
> >> +	/* __ATOMIC_ACQUIRE to prevent loads after (in program order)
> >> +	 * from happening before the sn load. Syncronizes-with the
> >> +	 * store release in rte_seqlock_end().
> >> +	 */
> >> +	return __atomic_load_n(&seqlock->sn, __ATOMIC_ACQUIRE);
> >> +}
> >> +
> >> +__rte_experimental
> >> +static inline bool
> >> +rte_seqlock_read_retry(const rte_seqlock_t *seqlock, uint64_t begin_sn)
> >> +{
> >> +	uint64_t end_sn;
> >> +
> >> +	/* make sure the data loads happens before the sn load */
> >> +	rte_atomic_thread_fence(__ATOMIC_ACQUIRE);
> > That's sort of 'read_end' correct?
> > If so, shouldn't it be '__ATOMIC_RELEASE' instead here,
> > and
> > end_sn = __atomic_load_n(..., (__ATOMIC_ACQUIRE)
> > on the line below?
> 
> A release fence prevents reordering of stores. The reader doesn't do any
> stores, so I don't understand why you would use a release fence here.
> Could you elaborate?

From my understanding:  
rte_atomic_thread_fence(__ATOMIC_ACQUIRE);
serves as a hoist barrier here, so it would only prevent later instructions
to be executed before that point.
But it wouldn't prevent earlier instructions to be executed after that point.
While we do need to guarantee that cpu will finish all previous reads before
progressing further. 

Suppose we have something like that:

struct {
	uint64_t shared;
	rte_seqlock_t lock;
} data;

...
sn = ...
uint64_t x = data.shared; 
/* inside rte_seqlock_read_retry(): */
...
rte_atomic_thread_fence(__ATOMIC_ACQUIRE);
end_sn = __atomic_load_n(&data.lock.sn, __ATOMIC_RELAXED);

Here we need to make sure that read of data.shared will always happen
before reading of data.lock.sn. 
It is not a problem on IA (as reads are not reordered), but on machines with 
relaxed memory ordering (ARM, etc.)  it can happen.
So to prevent it we do need a sink barrier here first (ATOMIC_RELEASE).

Honnappa and other ARM & atomics experts, please correct me if I am wrong here.    

> >> +
> >> +	end_sn = __atomic_load_n(&seqlock->sn, __ATOMIC_RELAXED);
> >> +
> >> +	return unlikely(begin_sn & 1 || begin_sn != end_sn);
> >> +}
> >> +
> >> +__rte_experimental
> >> +static inline void
> >> +rte_seqlock_write_begin(rte_seqlock_t *seqlock)
> >> +{
> >> +	uint64_t sn;
> >> +
> >> +	/* to synchronize with other writers */
> >> +	rte_spinlock_lock(&seqlock->lock);
> >> +
> >> +	sn = seqlock->sn + 1;
> >> +
> >> +	__atomic_store_n(&seqlock->sn, sn, __ATOMIC_RELAXED);
> >> +
> >> +	/* __ATOMIC_RELEASE to prevent stores after (in program order)
> >> +	 * from happening before the sn store.
> >> +	 */
> >> +	rte_atomic_thread_fence(__ATOMIC_RELEASE);
> > I think it needs to be '__ATOMIC_ACQUIRE' here instead of '__ATOMIC_RELEASE'.
> 
> Please elaborate on why.

As you said in the comments above, we need to prevent later stores
to be executed before that point. So we do need a hoist barrier here.
AFAIK to guarantee a hoist barrier '__ATOMIC_ACQUIRE' is required.

> 
> >> +}
> >> +
> >> +__rte_experimental
> >> +static inline void
> >> +rte_seqlock_write_end(rte_seqlock_t *seqlock)
> >> +{
> >> +	uint64_t sn;
> >> +
> >> +	sn = seqlock->sn + 1;
> >> +
> >> +	/* synchronizes-with the load acquire in rte_seqlock_begin() */
> >> +	__atomic_store_n(&seqlock->sn, sn, __ATOMIC_RELEASE);
> >> +
> >> +	rte_spinlock_unlock(&seqlock->lock);
> >> +}
> >> +


  reply	other threads:[~2022-03-28 10:54 UTC|newest]

Thread overview: 104+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-03-22 16:10 DPDK seqlock Mattias Rönnblom
2022-03-22 16:46 ` Ananyev, Konstantin
2022-03-24  4:52   ` Honnappa Nagarahalli
2022-03-24  5:06     ` Stephen Hemminger
2022-03-24 11:34     ` Mattias Rönnblom
2022-03-25 20:24       ` [RFC] eal: add seqlock Mattias Rönnblom
2022-03-25 21:10         ` Stephen Hemminger
2022-03-26 14:57           ` Mattias Rönnblom
2022-03-27 14:49         ` Ananyev, Konstantin
2022-03-27 17:42           ` Mattias Rönnblom
2022-03-28 10:53             ` Ananyev, Konstantin [this message]
2022-03-28 14:06               ` Ola Liljedahl
2022-03-29  8:32                 ` Mattias Rönnblom
2022-03-29 13:20                   ` Ananyev, Konstantin
2022-03-30 10:07                     ` [PATCH] " Mattias Rönnblom
2022-03-30 10:50                       ` Morten Brørup
2022-03-30 11:24                         ` Tyler Retzlaff
2022-03-30 11:25                         ` Mattias Rönnblom
2022-03-30 14:26                         ` [PATCH v2] " Mattias Rönnblom
2022-03-31  7:46                           ` Mattias Rönnblom
2022-03-31  9:04                             ` Ola Liljedahl
2022-03-31  9:25                               ` Morten Brørup
2022-03-31  9:38                                 ` Ola Liljedahl
2022-03-31 10:03                                   ` Morten Brørup
2022-03-31 11:44                                     ` Ola Liljedahl
2022-03-31 11:50                                       ` Morten Brørup
2022-03-31 14:02                                       ` Mattias Rönnblom
2022-04-01 15:07                                         ` [PATCH v3] " Mattias Rönnblom
2022-04-02  0:21                                           ` Honnappa Nagarahalli
2022-04-02 11:01                                             ` Morten Brørup
2022-04-02 19:38                                               ` Honnappa Nagarahalli
2022-04-10 13:51                                                 ` [RFC 1/3] eal: add macro to warn for unused function return values Mattias Rönnblom
2022-04-10 13:51                                                   ` [RFC 2/3] eal: emit warning for unused trylock return value Mattias Rönnblom
2022-04-10 13:51                                                   ` [RFC 3/3] examples/bond: fix invalid use of trylock Mattias Rönnblom
2022-04-11  1:01                                                     ` Min Hu (Connor)
2022-04-11 14:32                                                       ` Mattias Rönnblom
2022-04-11 11:25                                                     ` David Marchand
2022-04-11 14:33                                                       ` Mattias Rönnblom
2022-04-10 18:02                                                   ` [RFC 1/3] eal: add macro to warn for unused function return values Stephen Hemminger
2022-04-10 18:50                                                     ` Mattias Rönnblom
2022-04-11  7:17                                                   ` Morten Brørup
2022-04-11 14:29                                                     ` Mattias Rönnblom
2022-04-11  9:16                                                   ` Bruce Richardson
2022-04-11 14:27                                                     ` Mattias Rönnblom
2022-04-11 15:15                                                     ` [PATCH " Mattias Rönnblom
2022-04-11 15:15                                                       ` [PATCH 2/3] eal: emit warning for unused trylock return value Mattias Rönnblom
2022-04-11 15:29                                                         ` Morten Brørup
2022-04-11 15:15                                                       ` [PATCH 3/3] examples/bond: fix invalid use of trylock Mattias Rönnblom
2022-04-14 12:06                                                         ` David Marchand
2022-04-11 15:25                                                       ` [PATCH 1/3] eal: add macro to warn for unused function return values Morten Brørup
2022-04-11 18:24                                                     ` [RFC " Tyler Retzlaff
2022-04-03  6:10                                             ` [PATCH v3] eal: add seqlock Mattias Rönnblom
2022-04-03 17:27                                               ` Honnappa Nagarahalli
2022-04-03 18:37                                                 ` Ola Liljedahl
2022-04-04 21:56                                                   ` Honnappa Nagarahalli
2022-04-03  6:33                                             ` Mattias Rönnblom
2022-04-03 17:37                                               ` Honnappa Nagarahalli
2022-04-08 13:45                                                 ` Mattias Rönnblom
2022-04-02 18:15                                           ` Ola Liljedahl
2022-04-02 19:31                                             ` Honnappa Nagarahalli
2022-04-02 20:36                                               ` Morten Brørup
2022-04-02 22:01                                                 ` Honnappa Nagarahalli
2022-04-03 18:11                                               ` Ola Liljedahl
2022-04-03  6:51                                             ` Mattias Rönnblom
2022-03-31 13:51                                 ` [PATCH v2] " Mattias Rönnblom
2022-04-02  0:54                                   ` Stephen Hemminger
2022-04-02 10:25                                     ` Morten Brørup
2022-04-02 17:43                                       ` Ola Liljedahl
2022-03-31 13:38                               ` Mattias Rönnblom
2022-03-31 14:53                                 ` Ola Liljedahl
2022-04-02  0:52                                   ` Stephen Hemminger
2022-04-03  6:23                                     ` Mattias Rönnblom
2022-04-02  0:50                           ` Stephen Hemminger
2022-04-02 17:54                             ` Ola Liljedahl
2022-04-02 19:37                               ` Honnappa Nagarahalli
2022-04-05 20:16                           ` Stephen Hemminger
2022-04-08 13:50                             ` Mattias Rönnblom
2022-04-08 14:24                               ` [PATCH v4] " Mattias Rönnblom
2022-04-08 15:17                                 ` Stephen Hemminger
2022-04-08 16:24                                   ` Mattias Rönnblom
2022-04-08 15:19                                 ` Stephen Hemminger
2022-04-08 16:37                                   ` Mattias Rönnblom
2022-04-08 16:48                                 ` Mattias Rönnblom
2022-04-12 17:27                                 ` Ananyev, Konstantin
2022-04-28 10:28                                 ` David Marchand
2022-05-01 13:46                                   ` Mattias Rönnblom
2022-05-01 14:03                                     ` [PATCH v5] " Mattias Rönnblom
2022-05-01 14:22                                       ` Mattias Rönnblom
2022-05-02  6:47                                         ` David Marchand
2022-05-01 20:17                                       ` Stephen Hemminger
2022-05-02  4:51                                         ` Mattias Rönnblom
2022-05-06  1:26                                       ` fengchengwen
2022-05-06  1:33                                         ` Honnappa Nagarahalli
2022-05-06  4:17                                           ` fengchengwen
2022-05-06  5:19                                             ` Honnappa Nagarahalli
2022-05-06  7:03                                               ` fengchengwen
2022-05-08 11:56                                         ` Mattias Rönnblom
2022-05-08 12:12                                           ` [PATCH v6] " Mattias Rönnblom
2022-05-08 16:10                                             ` Stephen Hemminger
2022-05-08 19:40                                               ` Mattias Rönnblom
2022-05-09  3:48                                                 ` Stephen Hemminger
2022-05-09  6:26                                                   ` Morten Brørup
2022-05-13  6:27                                                   ` Mattias Rönnblom
2022-03-23 12:04 ` DPDK seqlock Morten Brørup

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DM6PR11MB4491FDC957C312689FD633169A1D9@DM6PR11MB4491.namprd11.prod.outlook.com \
    --to=konstantin.ananyev@intel.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=mattias.ronnblom@ericsson.com \
    --cc=mb@smartsharesystems.com \
    --cc=nd@arm.com \
    --cc=ola.liljedahl@arm.com \
    --cc=onar.olsen@ericsson.com \
    --cc=stephen@networkplumber.org \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).