Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple Receive API Support #4182

Merged
merged 53 commits into from
Aug 5, 2024
Merged

Multiple Receive API Support #4182

merged 53 commits into from
Aug 5, 2024

Conversation

nibanks
Copy link
Member

@nibanks nibanks commented Mar 8, 2024

Description

Adds a settings for apps to configure streams to support multiple parallel receives.

Testing

TODO

Documentation

TODO

@nibanks nibanks added Area: API Area: Core Related to the shared, core protocol logic labels Mar 8, 2024
Copy link

codecov bot commented Apr 26, 2024

Codecov Report

Attention: Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 85.88%. Comparing base (7483ce2) to head (e765cf3).

Files Patch % Lines
src/core/settings.c 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4182      +/-   ##
==========================================
+ Coverage   85.31%   85.88%   +0.56%     
==========================================
  Files          56       56              
  Lines       15457    15475      +18     
==========================================
+ Hits        13187    13290     +103     
+ Misses       2270     2185      -85     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ami-GS ami-GS force-pushed the nibanks/multi-recv-api branch from 9398cd9 to e2500a6 Compare June 19, 2024 17:55
@ami-GS ami-GS force-pushed the nibanks/multi-recv-api branch from e2500a6 to a1f2339 Compare June 19, 2024 17:56
@ami-GS
Copy link
Contributor

ami-GS commented Jun 19, 2024

1G send sometimes hit assert here

CXPLAT_DBG_ASSERT(ChunkLength);

@ami-GS
Copy link
Contributor

ami-GS commented Jun 19, 2024

hit

CXPLAT_DBG_ASSERT((uint64_t)AllocLength + (uint64_t)Chunk->AllocLength < UINT32_MAX);

@ami-GS
Copy link
Contributor

ami-GS commented Jun 19, 2024

RecvBuffer->ReadLength seems to have synchronization issue. Especially after resizing chunk

@ami-GS
Copy link
Contributor

ami-GS commented Jun 21, 2024

This guy does bad. Not necessarily the cap split the draining range. Or simply split ranges maybe not indicated to an app.

QuicRangeSize(&RecvBuffer->WrittenRanges) > 1) {

@ami-GS ami-GS force-pushed the nibanks/multi-recv-api branch from ddb7d2e to 2fcb778 Compare July 8, 2024 17:43
@ami-GS
Copy link
Contributor

ami-GS commented Jul 8, 2024

Where to handle remaining ReadPendingLength. App? api.c? stream_recv.c? recv_buff.c?

@ami-GS ami-GS force-pushed the nibanks/multi-recv-api branch from 30d6067 to 714042c Compare July 8, 2024 20:12
@ami-GS ami-GS force-pushed the nibanks/multi-recv-api branch from d621df0 to 8893063 Compare July 8, 2024 23:13
@ami-GS
Copy link
Contributor

ami-GS commented Aug 1, 2024

ready to go

@nibanks
Copy link
Member Author

nibanks commented Aug 1, 2024

@ami-GS can you update the docs too? I review the code and it looks good to me. But since it's my PR I can't approve. You will have to and then merge once you get the docs in.

@ami-GS
Copy link
Contributor

ami-GS commented Aug 1, 2024

@ami-GS can you update the docs too? I review the code and it looks good to me. But since it's my PR I can't approve. You will have to and then merge once you get the docs in.

Is the doc this?
#4384

Or general API doc?

@nibanks
Copy link
Member Author

nibanks commented Aug 1, 2024

The general API doc. I will update that other PR sometime later, after this is merged.

ami-GS
ami-GS previously approved these changes Aug 1, 2024
@ami-GS ami-GS enabled auto-merge (squash) August 5, 2024 17:05
@ami-GS ami-GS merged commit f960155 into main Aug 5, 2024
446 of 449 checks passed
@ami-GS ami-GS deleted the nibanks/multi-recv-api branch August 5, 2024 17:11
ami-GS added a commit that referenced this pull request Aug 5, 2024
* WIP

* Fixes

* Simplify

* fix

* Fix

* Fix more bugs

* Fix clog and .net

* Fixes

* Improvements

* Undo a merge issue

* Another merge issue

* Simple test

* remove fprintf

* fix signature to upto 255. add Large send case

* 1G Multi receive

* fix drain bug

* kernel test

* fix

* fix

* unused variable

* compare data

* tmp

* test cases

* 95% works

* remove fprintf in core

* Fix Range copy

* fix type mimatch

* cleanup

* Fix stall issue

* rollback

* retry if length is 0

* remove continue to reset Readpending in QuicStreamReceiveComplete

* stop enablling send with pending data (race condition exists)

* fix QuicRecvBufferHasUnreadData

* rollback for recv_buff lock bug then return earlier not to indicate FIN bit

* fix

* fix

* reduce test buffer size for CI speed

* update document

* add back buffer count caution

* Update docs/api/StreamReceiveComplete.md

Co-authored-by: Nick Banks <[email protected]>

* Update docs/api/StreamReceiveComplete.md

Co-authored-by: Nick Banks <[email protected]>

* logical conflicts

---------

Co-authored-by: ami-GS <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: API Area: Core Related to the shared, core protocol logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants