Re: [PATCH v9] eal: add seqlock

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

On 2022-05-31 13:52, David Marchand wrote:
> On Mon, May 23, 2022 at 4:24 PM Mattias Rönnblom
> <mattias.ronnblom@ericsson.com> wrote:
>>
>> A sequence lock (seqlock) is a synchronization primitive which allows
>> for data-race free, low-overhead, high-frequency reads, suitable for
>> data structures shared across many cores and which are updated
>> relatively infrequently.
>>
>> A seqlock permits multiple parallel readers. A spinlock is used to
>> serialize writers. In cases where there is only a single writer, or
>> writer-writer synchronization is done by some external means, the
>> "raw" sequence counter type (and accompanying rte_seqcount_*()
>> functions) may be used instead.
>>
>> To avoid resource reclamation and other issues, the data protected by
>> a seqlock is best off being self-contained (i.e., no pointers [except
>> to constant data]).
>>
>> One way to think about seqlocks is that they provide means to perform
>> atomic operations on data objects larger than what the native atomic
>> machine instructions allow for.
>>
>> DPDK seqlocks (and the underlying sequence counters) are not
>> preemption safe on the writer side. A thread preemption affects
>> performance, not correctness.
>>
>> A seqlock contains a sequence number, which can be thought of as the
>> generation of the data it protects.
>>
>> A reader will
>>    1. Load the sequence number (sn).
>>    2. Load, in arbitrary order, the seqlock-protected data.
>>    3. Load the sn again.
>>    4. Check if the first and second sn are equal, and even numbered.
>>       If they are not, discard the loaded data, and restart from 1.
>>
>> The first three steps need to be ordered using suitable memory fences.
>>
>> A writer will
>>    1. Take the spinlock, to serialize writer access.
>>    2. Load the sn.
>>    3. Store the original sn + 1 as the new sn.
>>    4. Perform load and stores to the seqlock-protected data.
>>    5. Store the original sn + 2 as the new sn.
>>    6. Release the spinlock.
>>
>> Proper memory fencing is required to make sure the first sn store, the
>> data stores, and the second sn store appear to the reader in the
>> mentioned order.
>>
>> The sn loads and stores must be atomic, but the data loads and stores
>> need not be.
>>
>> The original seqlock design and implementation was done by Stephen
>> Hemminger. This is an independent implementation, using C11 atomics.
>>
>> For more information on seqlocks, see
>> https://en.wikipedia.org/wiki/Seqlock
> 
> The SoB and other tags were with annotations, just repeating them here
> (in the hope patchwork will catch them):
> 
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Reviewed-by: Ola Liljedahl <ola.liljedahl@arm.com>
> Reviewed-by: Chengwen Feng <fengchengwen@huawei.com>
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> 
> GHA had a hiccup on Ubuntu vm installation at the time of the v9 reception.
> I had run the checks on my side:
> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af-454445555731-b0b15dcc7275f17b&q=1&e=d209ff4b-9405-4dbe-8e79-b8505f61ca68&u=https%3A%2F%2Fgithub.com%2Fdavid-marchand%2Fdpdk%2Factions%2Fruns%2F2373533127
> 
> I noticed a little issue in the example in the doxygen comments that I
> can fix myself:
> --- a/lib/eal/include/rte_seqlock.h
> +++ b/lib/eal/include/rte_seqlock.h
> @@ -63,7 +63,7 @@ extern "C" {
>    *                 // An alternative to an immediate retry is to abort and
>    *                 // try again at some later time, assuming progress is
>    *                 // possible without the data.
> - *         } while (rte_seqlock_read_retry(&config->lock));
> + *         } while (rte_seqlock_read_retry(&config->lock, sn));
>    * }
>    *
>    * // Accessor function for writing config fields.
> 
> I also have some whitespace fixes here and there.
> 
> I may shorten the release notes update to keep only the first
> paragraph, as the rest is too verbose and repeated in the
> documentation.

Makes sense.

> 
> Overall, comments were addressed and the patch looks ready.
> Last call before merging?
> 
> 

I don't have anything to add. Thanks.