On 3/31/22 11:25, Morten Brørup wrote:
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.
I was following the naming convention of rte_rwlock. Isn't the seqlock
just a more scalable implementation of a reader/writer lock?
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().
- Ola
