Re: [PATCH v9] eal: add seqlock

[David Marchand <david.marchand@redhat.com>]

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://github.com/david-marchand/dpdk/actions/runs/2373533127

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.

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


-- 
David Marchand