DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Ola Liljedahl" <ola.liljedahl@arm.com>,
	"Mattias Rönnblom" <mattias.ronnblom@ericsson.com>,
	dev@dpdk.org
Cc: "Thomas Monjalon" <thomas@monjalon.net>,
	"David Marchand" <david.marchand@redhat.com>,
	"Onar Olsen" <onar.olsen@ericsson.com>,
	<Honnappa.Nagarahalli@arm.com>, <nd@arm.com>,
	<konstantin.ananyev@intel.com>, <stephen@networkplumber.org>
Subject: RE: [PATCH v2] eal: add seqlock
Date: Thu, 31 Mar 2022 12:03:22 +0200	[thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D86F8E@smartserver.smartshare.dk> (raw)
In-Reply-To: <a44ce729-f877-8b81-a21c-5a9f70ed82e5@arm.com>

> From: Ola Liljedahl [mailto:ola.liljedahl@arm.com]
> Sent: Thursday, 31 March 2022 11.39
> 
> On 3/31/22 11:25, Morten Brørup wrote:
> >> From: Ola Liljedahl [mailto:ola.liljedahl@arm.com]
> >> Sent: Thursday, 31 March 2022 11.05
> >>
> >> On 3/31/22 09:46, Mattias Rönnblom wrote:
> >>> On 2022-03-30 16:26, Mattias Rönnblom wrote:
> >>>> A sequence lock (seqlock) is synchronization primitive which
> >>>> allows
> >>>> for data-race free, low-overhead, high-frequency reads, especially
> >>>> for
> >>>> data structures shared across many cores and which are updated
> >>>> with relatively infrequently.
> >>>>
> >>>>
> >>>
> >>> <snip>
> >>>
> >>> Some questions I have:
> >>>
> >>> Is a variant of the seqlock without the spinlock required? The
> >>> reason I
> >>> left such out was that I thought that in most cases where only a
> >>> single
> >>> writer is used (or serialization is external to the seqlock), the
> >>> spinlock overhead is negligible, since updates are relatively
> >>> infrequent.
> >
> > Mattias, when you suggested adding the seqlock, I considered this
> > too, and came to the same conclusion as you.
> >
> >> You can combine the spinlock and the sequence number. Odd sequence
> >> number means the seqlock is busy. That would replace a non-atomic
> >> RMW of
> >> the sequence number with an atomic RMW CAS and avoid the spin lock
> >> atomic RMW operation. Not sure how much it helps.
> >>
> >>>
> >>> Should the rte_seqlock_read_retry() be called
> >>> rte_seqlock_read_end(), or
> >>> some third alternative? I wanted to make clear it's not just a
> >>> "release the lock" function. You could use
> >>> the|||__attribute__((warn_unused_result)) annotation to make clear
> >>> the
> >>> return value cannot be ignored, although I'm not sure DPDK ever use
> >>> that attribute.
> >
> > I strongly support adding __attribute__((warn_unused_result)) to the
> > function. There's a first time for everything, and this attribute is
> > very relevant here!
> >
> >> We have to decide how to use the seqlock API from the application
> >> perspective.
> >> Your current proposal:
> >> do {
> >>       sn = rte_seqlock_read_begin(&seqlock)
> >>       //read protected data
> >> } while (rte_seqlock_read_retry(&seqlock, sn));
> >>
> >> or perhaps
> >> sn = rte_seqlock_read_lock(&seqlock);
> >> do {
> >>       //read protected data
> >> } while (!rte_seqlock_read_tryunlock(&seqlock, &sn));
> >>
> >> Tryunlock should signal to the user that the unlock operation might
> >> not succeed and something needs to be repeated.
> >
> > Perhaps rename rte_seqlock_read_retry() to rte_seqlock_read_tryend()?
> > As Ola mentions, this also inverses the boolean result value. If you
> > consider this, please check that the resulting assembly output remains
> > efficient.
> >
> > I think lock()/unlock() should be avoided in the read operation
> > names, because no lock is taken during read. I like the critical region
> > begin()/end() names.
> I was following the naming convention of rte_rwlock. Isn't the seqlock
> just a more scalable implementation of a reader/writer lock?

I see your point. However, no lock is taken, so using lock()/unlock() is somewhat misleading.

I have no strong opinion about this, so I'll leave it up to Mattias.

> > Regarding naming, you should also consider renaming
> > rte_seqlock_write_begin/end() to rte_seqlock_write_lock/unlock(),
> > following the naming convention of the other locks. This could prepare
> > for future extensions, such as rte_seqlock_write_trylock(). Just a
> > thought; I don't feel strongly about this.
> >
> > Ola, the rte_seqlock_read_lock(&seqlock) must remain inside the loop,
> > because retries can be triggered by a write operation happening between
> > the read_begin() and read_tryend(), and then the new sn must be used by
> > the read operation.
> That's why my rte_seqlock_read_tryunlock() function takes the sequence
> number as a parameter passed by reference. Then the sequence number can
> be updated if necessary. I didn't want to force a new call to
> rte_seqlock_read_lock() because there should be a one-to-one match
> between rte_seqlock_read_lock() and a successful call to
> rte_seqlock_read_tryunlock().

Uhh... I missed that point.

In that case, consider passing sn as output parameter to read_begin() by reference too, like the Linux kernel's spin_lock_irqsave() takes the flags as output parameter. I don't have a strong opinion here; just mentioning the possibility.

Performance wise, the resulting assembly output is probably the same, regardless if sn is the return value or an output parameter passed by reference.


  reply	other threads:[~2022-03-31 10:03 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
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 [this message]
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=98CBD80474FA8B44BF855DF32C47DC35D86F8E@smartserver.smartshare.dk \
    --to=mb@smartsharesystems.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=konstantin.ananyev@intel.com \
    --cc=mattias.ronnblom@ericsson.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).