On 2022-05-31 13:52, David Marchand wrote: > On Mon, May 23, 2022 at 4:24 PM Mattias Rönnblom > <mattias.ronnb...@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 <m...@smartsharesystems.com> > Acked-by: Konstantin Ananyev <konstantin.anan...@intel.com> > Reviewed-by: Ola Liljedahl <ola.liljed...@arm.com> > Reviewed-by: Chengwen Feng <fengcheng...@huawei.com> > Signed-off-by: Mattias Rönnblom <mattias.ronnb...@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.
