Re: [PATCH v2] eal: add seqlock

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2022-03-31 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!
> 

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.
>