Re: [PATCH v2] eal: add seqlock

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

On 2022-03-31 11:04, Ola Liljedahl wrote:
> 
> 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.
> 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.
> 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.
> 

I like that your proposal is consistent with rwlock API, although I tend 
to think about a seqlock more like an arbitrary-size atomic load/store, 
where begin() is the beginning of the read transaction.

What I don't like so much with "tryunlock" is that it's not obvious what 
return type and values it should have. I seem not to be the only one 
which suffers from a lack of intuition here, since the DPDK spinlock 
trylock() function returns '1' in case lock is taken (using an int, but 
treating it like a bool), while the rwlock equivalent returns '0' (also 
int, but treating it as an error code).

"lock" also suggests you prevent something from occurring, which is not 
the case on the reader side. A calling application also need not call 
the reader unlock (or retry) function for all seqlocks it has locked, 
although I don't see a point why it wouldn't. (I don't see why a 
read-side critical section should contain much logic at all, since you 
can't act on the just-read data.)