(Thunderbird suddenly refuses to edit in plain text mode, hope the mail
gets sent as text anyway)
On 3/31/22 15:38, Mattias Rönnblom wrote:
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:
<snip>
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.
I can see the evolution of an application where is starts to use plain
spin locks, moves to reader/writer locks for better performance and
eventually moves to seqlocks. The usage is the same, only the
characteristics (including performance) differ.
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).
Then you have two different ways of doing it! Or invent a third since
there seems to be no consistent pattern.
"lock" also suggests you prevent something from occurring, which is not
the case on the reader side.
That's why my implementations in Progress64 use the terms acquire and
release. Locks are acquired and released (with acquire and release
semantics!). Hazard pointers are acquired and released (with acquire and
release semantics!). Slots in reorder buffers are acquired and released.
Etc.
https://github.com/ARM-software/progress64
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.)
Lock without unlock/retry is meaningless and not something we need to
consider IMO.
- Ola
