Add io_uring support #5061
Add io_uring support #5061
Conversation
| CxPlatFree(Entry, Pool->Tag); | ||
| } | ||
| CxPlatLockUninitialize(&Pool->Lock); | ||
| CxPlatPoolInitializeEx(IsPaged, Size, Tag, NULL, NULL, Pool); |
There was a problem hiding this comment.
Instead of NULLs and always doing if checks in every alloc/free, should we have defaults that call what we want?
There was a problem hiding this comment.
I might back out this change if I don't get as far as zerocopy TX, which is the only use case for this model.
| #ifdef DEBUG | ||
| #define CXPLAT_DBG_ASSERT_CMSG(CMsg, type) \ | ||
| if (CMsg->cmsg_len < CMSG_LEN(sizeof(type))) { \ | ||
| printf("%u: cmsg[%u:%u] len (%u) < exp_len (%u)\n", \ |
There was a problem hiding this comment.
I never noticed we had a printf assert. We should replace this with a real assert.
| void *Ring; | ||
| uint8_t *Buffers; |
There was a problem hiding this comment.
nit, style puts the * with the type
| void *Ring; | |
| uint8_t *Buffers; | |
| void* Ring; | |
| uint8_t* Buffers; |
There was a problem hiding this comment.
Yeah, I will have to do one sweep for these after, because they keep creeping in.
src/platform/platform_worker.c
Outdated
| for (uint32_t i = 0; i < CqeCount; ++i) { | ||
| CXPLAT_SQE* Sqe = CxPlatCqeGetSqe(&Cqes[i]); | ||
| Sqe->Completion(&Cqes[i]); | ||
| while (CqeCount > 0) { | ||
| CXPLAT_SQE* Sqe = CxPlatCqeGetSqe(CurrentCqe); | ||
| Sqe->Completion(&CurrentCqe, &CurrentCqeCount); |
There was a problem hiding this comment.
Every SQE has its own completion function. Why are you changing that?
There was a problem hiding this comment.
The new completion function supports batching completions, which is better than calling Foo() 100 times, and having Foo() do boilerplate (lock, submit, etc.) each time.
There was a problem hiding this comment.
Batching of what exactly though? If you have a list of 10 completion functions to call, what is being batched? The callbacks are all outside of a lock, right?
There was a problem hiding this comment.
The CurrentCqe and CurrentCqeCount are both _Inout_ parameters, so the routine can consume [1, CurrentCqeCount] CQEs.
Here's the only routine taking advantage:
msquic/src/platform/datapath_iouring.c
Lines 2106 to 2116 in 0752c6d
| inline | ||
| _IRQL_requires_max_(PASSIVE_LEVEL) | ||
| void | ||
| CxPlatSqeClassicCompletion( |
There was a problem hiding this comment.
Why add this interface to just do what the caller was doing anyways?
There was a problem hiding this comment.
See the other comment (The new completion function supports batching completions) and the comment in this function (it's a hack to avoid updating all code).
|
|
||
| Abstract: | ||
|
|
||
| QUIC datapath Abstraction Layer. |
There was a problem hiding this comment.
nit: The abstract should be updated (or removed, if it doesn't provide useful info)
There was a problem hiding this comment.
Just to add some context, this is currently a draft PR.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5061 +/- ##
==========================================
+ Coverage 85.92% 87.00% +1.08%
==========================================
Files 59 59
Lines 18035 18035
==========================================
+ Hits 15496 15691 +195
+ Misses 2539 2344 -195 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Describe the purpose of and changes within this Pull Request.
Testing
Do any existing tests cover this change? Are new tests needed?
Documentation
Is there any documentation impact for this change?