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

Refactor the RX Datapath Object Abstraction Model #3826

Merged
merged 9 commits into from
Aug 23, 2023
Merged

Conversation

nibanks
Copy link
Member

@nibanks nibanks commented Aug 22, 2023

Description

Cleans up a few things:

  1. Creates a clear abstraction between QUIC and platform layer.
  2. Renames QUIC layer objects to remove CXPLAT name.
  3. Refactors internals of datapaths to have similar structs/names to make it easier to understand (and maybe converge eventually).

Testing

Existing automation

Documentation

N/A

@nibanks nibanks requested a review from a team as a code owner August 22, 2023 18:17
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #3826 (9796ddd) into main (cd49fbb) will increase coverage by 0.00%.
The diff coverage is 86.48%.

@@           Coverage Diff           @@
##             main    #3826   +/-   ##
=======================================
  Coverage   86.57%   86.57%           
=======================================
  Files          56       56           
  Lines       16576    16581    +5     
=======================================
+ Hits        14350    14355    +5     
  Misses       2226     2226           
Files Changed Coverage Δ
src/core/binding.h 63.15% <ø> (ø)
src/core/datagram.c 90.31% <ø> (-1.17%) ⬇️
src/core/library.c 83.10% <ø> (-0.81%) ⬇️
src/core/packet.h 97.93% <ø> (ø)
src/core/stream.h 93.33% <ø> (ø)
src/core/stream_recv.c 87.77% <ø> (-0.63%) ⬇️
src/core/binding.c 87.55% <77.41%> (+0.14%) ⬆️
src/core/connection.c 81.50% <88.00%> (-0.07%) ⬇️
src/core/packet.c 76.81% <100.00%> (-0.25%) ⬇️
src/core/worker.c 89.45% <100.00%> (+1.70%) ⬆️

... and 7 files with indirect coverage changes

src/inc/quic_datapath.h Outdated Show resolved Hide resolved
// Internal receive context.
//
typedef struct CXPLAT_DATAPATH_INTERNAL_RECV_BUFFER_CONTEXT {
typedef struct DECLSPEC_ALIGN(MEMORY_ALLOCATION_ALIGNMENT) DATAPATH_RX_PACKET {
Copy link
Contributor

Choose a reason for hiding this comment

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

DECLSPEC_ALIGN(MEMORY_ALLOCATION_ALIGNMENT)

Why do we need this alignment requirement here?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's actually needed, we then need a comment similar to what we have in the XDP datapath.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if it's strictly necessary. It was copy/paste.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like it's necessary. There is no SLIST operations in this file.

I will leave it up to you.


} CXPLAT_DATAPATH_INTERNAL_RECV_BUFFER_CONTEXT;
} DATAPATH_RX_PACKET;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a comment saying there's a client data following the last field.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is already a comment at the end of CXPLAT_RECV_DATA to that effect. I'm going to leave that as is for now.

@nibanks nibanks merged commit 972e677 into main Aug 23, 2023
435 of 436 checks passed
@nibanks nibanks deleted the nibanks/dal-rx-packet branch August 23, 2023 19:14
@wfurt wfurt mentioned this pull request Nov 14, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants