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

Add a mode to the receive buffer to handle external buffers #4758

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

guhetier
Copy link

@guhetier guhetier commented Jan 22, 2025

Description

As part of #4747, the receive buffer needs a mode to operate on buffers provided by the application.
This PR defines a new QUIC_RECV_BUF_MODE_EXTERNAL mode.

  • QUIC_RECV_CHUNKs now have a pointer to the actual buffer instead of an end-of-struct array
    • Except for the EXTERNAL mode, the buffer is still part of the same allocation as the chunk
    • For EXTERNAL mode, buffers are provided by the application and chunks point to them
  • The EXTERNAL mode writes only once to the provided buffers and discard them when they are drained. Buffers are never re-used.
  • Reads and writes can span through multiple chunks. They are never treated as circular buffers.
  • The EXTERNAL mode never allocates / re-allocate buffers
    • The virtual buffer size is set to the sum of the provided buffer sizes to avoid receiving more data than we can buffer
    • The virtual buffer size can now decrease when data is drained - but the maximum byte offset that can be received still only increases.

Testing

Unit tests have been added

Documentation

To be done

@guhetier guhetier force-pushed the guhetier_app_buffers branch from e7d3261 to 07dc4d9 Compare January 22, 2025 00:14
@guhetier guhetier force-pushed the guhetier_app_buffers branch from 07dc4d9 to 06d5bcf Compare January 22, 2025 00:44
@nibanks nibanks added Area: API Area: Core Related to the shared, core protocol logic labels Jan 22, 2025
@@ -22,9 +25,19 @@ typedef struct QUIC_RECV_CHUNK {
CXPLAT_LIST_ENTRY Link; // Link in the list of chunks.
uint32_t AllocLength : 31; // Allocation size of Buffer
uint32_t ExternalReference : 1; // Indicates the buffer is being used externally.
uint8_t Buffer[0];
uint8_t *Buffer; // Pointer to the buffer itself. Doesn't need to be freed independently:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat independent of this PR, I am wondering if we could/should annotate this pointer:

Suggested change
uint8_t *Buffer; // Pointer to the buffer itself. Doesn't need to be freed independently:
_Field_size_bytes_(AllocLength)
uint8_t *Buffer; // Pointer to the buffer itself. Doesn't need to be freed independently:

QuicRecvChunkInitialize(
_Inout_ QUIC_RECV_CHUNK* Chunk,
_In_ uint32_t AllocLength,
_In_ uint8_t* Buffer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe improve the annotation a bit more? I think this is it:

Suggested change
_In_ uint8_t* Buffer
_In_updates_(AllocLength) uint8_t* Buffer

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I also need to add the corresponding annotations on the chunk struct itself.

// External chunks can be provided only if already in external mode, or if
// nothing has been written to the buffer yet.
//
uint64_t rangeMax;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, our style capitalizes all variables and fields.

Suggested change
uint64_t rangeMax;
uint64_t RangeMax;

Please update for everything else too. Thanks!

src/core/recv_buffer.c Outdated Show resolved Hide resolved
@@ -186,6 +262,7 @@ QuicRecvBufferResize(
_In_ uint32_t TargetBufferLength
)
{
CXPLAT_DBG_ASSERT(RecvBuffer->RecvMode != QUIC_RECV_BUF_MODE_EXTERNAL); // Should never resize in external mode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, we also have CXPLAT_DBG_ASSERTMSG which could be used here instead of the comment:

Suggested change
CXPLAT_DBG_ASSERT(RecvBuffer->RecvMode != QUIC_RECV_BUF_MODE_EXTERNAL); // Should never resize in external mode
CXPLAT_DBG_ASSERTMSG(RecvBuffer->RecvMode != QUIC_RECV_BUF_MODE_EXTERNAL, "Should never resize in external mode");

But you don't have to use it. We don't use it that much.

@@ -573,24 +660,28 @@ QuicRecvBufferWrite(
// N.B. We do this before updating the written ranges below so we don't have
// to support rolling back those changes on the possible allocation failure
// here.
// This is skiped in external mode since the entire virtual length is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// This is skiped in external mode since the entire virtual length is
// This is skipped in external mode since the entire virtual length is

Comment on lines 918 to 919
for (uint32_t i = 0; i < *BufferCount && remainingDataToRead > 0; ++i)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit:

Suggested change
for (uint32_t i = 0; i < *BufferCount && remainingDataToRead > 0; ++i)
{
for (uint32_t i = 0; i < *BufferCount && remainingDataToRead > 0; ++i) {

src/core/recv_buffer.c Outdated Show resolved Hide resolved
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