> From: Ola Liljedahl [mailto:ola.liljed...@arm.com] > Sent: Thursday, 31 March 2022 11.39 > > On 3/31/22 11:25, Morten Brørup wrote: > >> From: Ola Liljedahl [mailto:ola.liljed...@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! > > > >> 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. > I was following the naming convention of rte_rwlock. Isn't the seqlock > just a more scalable implementation of a reader/writer lock?
I see your point. However, no lock is taken, so using lock()/unlock() is somewhat misleading. I have no strong opinion about this, so I'll leave it up to Mattias. > > 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. > That's why my rte_seqlock_read_tryunlock() function takes the sequence > number as a parameter passed by reference. Then the sequence number can > be updated if necessary. I didn't want to force a new call to > rte_seqlock_read_lock() because there should be a one-to-one match > between rte_seqlock_read_lock() and a successful call to > rte_seqlock_read_tryunlock(). Uhh... I missed that point. In that case, consider passing sn as output parameter to read_begin() by reference too, like the Linux kernel's spin_lock_irqsave() takes the flags as output parameter. I don't have a strong opinion here; just mentioning the possibility. Performance wise, the resulting assembly output is probably the same, regardless if sn is the return value or an output parameter passed by reference.
