> From: Ola Liljedahl [mailto:ola.liljed...@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.
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.
