Closed HeroicKatora closed 4 months ago
Like I mentioned in the linked issue, I think the best place to put the SeqCst
fence is in the submit[_...]
functions right before calling sq_need_wakeup
in submit.rs. I don't think the after_intermittent_seqcst
function is necessary. We only need the full barrier when submitting new work to the SQ with SQPOLL enabled so executing the full barrier on submit makes the most sense.
I do disagree with that, the library exposes access to the SubmissionQueue
directly along with a raw Submitter::enter
. If we're concerned with incorrect usage, this interface should itself already provide the sensible defaults instead of only fixing the high-level usage through the library. (Besides there are already two paths with sq_need_wakeup
so this is also a code-duplication and evolution issue).
I should put the fix in the right location though and add a barrier to the Submitter::sq_need_wakeup
:weary:
The high-level interface in the Submitter
doesn't call the need_wakeup
function you modified, it uses its own function. I think at a minimum the safe high-level interface should behave correctly by default so regardless of the changes you made here, I think my suggested change should also probably be implemented.
I honestly don't know why need_wakeup
is even exposed as a separate function in SubmissionQueue
, I don't ever see a situation where you would want to query that separately from SQE submission. The equivalent function in liburing is not exposed. Submitter::enter
is an unsafe function and the documentation for it says you should probably use submit
instead. I think if you want to do all of this manually, it's fine to require users to insert the SeqCst
fence manually, maybe with some documentation mentioning that it's necessary. I think forcing a SeqCst
every time you want to check need_wakeup
even if it's not needed isn't the best solution.
Sure, the fix on Submitter
can use separate barriers. There seems to be an existing open issue within the code about the duplicate read from flags
that'll complicate the question further. If consolidated, the library definitely doesn't want to perform the barrier without sqpoll, maybe it's more straightforward to refactor.
I believe SubmissionQueue
seems natural to me, the constraint is on the order of updating head
with respect to reading the flag. As an external caller I do appreciate doing both through the same struct.
I'll ultimately leave this to maintainer's approach to the library interface. Commits are structure to include sufficient relevant documentation for each individual change (where it concerns public vs. private API).
Thank you! Anyway I agree align to liburing.
A memory fence is required to ensure the decision of the kernel needing waking is consistent between user-space and kernel. The kernel must not go to sleep without us reliably observing the wake flag, respectively it must see updates to the submission head if our load does not contain updated wake flag. The fence is paired with one in the kernel which ensures that one of the two stores is observed by the respective load of the other party.
Fix: #197