On 2022-03-31 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!
>
That would be a separate patch, I assume. Does anyone know if this
attribute is available in all supported compilers?
>> 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.
>
