feat(s2n-quic-core): add sync::worker module #1728
Conversation
| } | ||
|
|
||
| struct State { | ||
| remaining: CachePadded<AtomicUsize>, |
There was a problem hiding this comment.
Did you test with and without CachePadded and did it make a noticeable difference in performance?
Note that N is just a reasonable guess and is not guaranteed to match the actual cache line length of the machine the program is running on.
From the crate, which makes me a bit wary of premature optimization.
| macro_rules! acquire { | ||
| () => {{ | ||
| // take the credits that we've been given by the senders | ||
| self.credits += state.remaining.swap(0, Ordering::Acquire); |
There was a problem hiding this comment.
Do you want AcqRel since swap does a store and load
There was a problem hiding this comment.
spoke offline: in the swap operation itself is atomic so the load and store happen atomically. We read the credits and reset them to 0.
The ordering simply implies ordering with other atomic store operations, which is not critical to correct operation and happen in a relaxed manner.
Note that using Acquire makes the store part of this operation Relaxed
| // make one last effort to acquire credits in case a sender submitted some while we were | ||
| // registering the waker | ||
| acquire!(); |
There was a problem hiding this comment.
What happens if we dont call this? We would just wake up right? Should we de-register the waker if we were able to acquire credits?
There was a problem hiding this comment.
spoke offline: we need to check otherwise it possible to deadlock. sender tried waking receive but since it wasnt registered, it will never wake.
It not critical to de-register since we will simply wake up pre-maturely at worse, but thats not expected to happen or be detrimental.
| // loom tests will still run after returning so we don't need to join | ||
| if cfg!(not(loom)) { | ||
| sender.join().unwrap(); | ||
| receiver.join().unwrap(); | ||
| } |
There was a problem hiding this comment.
what does this mean? how do they keep running after returning?
There was a problem hiding this comment.
spoke offline: these tests can be run under loom or not. under loom we dont need to join, without it we do.
Description of changes:
The change adds a synchronization module that I've extracted from the AF_XDP work. The module is useful for scenarios where a worker needs a mechanism to know if it has outstanding work to do and a way for work producers to wake it up if it's gone to sleep.
In the AF_XDP provider, there isn't a mechanism to
pollon the completion queue so we're left with busy polling. The problem is we only need to busy poll while there is outstanding packets in the TX queue.Call-outs:
I've refactored some of the
spsccode to make it easier to share the testing facilities between the two modules.Testing:
This includes a few
loomtests to check that it behaves correctly in all ordering interleavings. It actually caught an error where the Sender didn't implement drop and notified the Receiver, thus resulting in a deadlock. Pretty cool!By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.