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

API refinement: make RX pipeline zero-copy as a side effect of the OOO reassembly support #38

Merged
merged 14 commits into from
Jul 22, 2023

Conversation

pavel-kirienko
Copy link
Member

@pavel-kirienko pavel-kirienko commented Jul 13, 2023

Close #36

Add RX frame parser (a simple function) with tests.

Update the RX pipeline as described in https://forum.opencyphal.org/t/refined-requirements-for-the-rx-pipeline-in-libudpard/1938.

Pros:

  • Zero-copy RX pipeline

Cons:

  • The udpardRx*Receive functions take ownership of the payload buffer.
  • The memory usage pattern depends on the MTU of the transmitting node even for the same extent. Small MTU --> more payload fragments --> more memory fragments to keep, and vice versa.

The docs are a bit rough around the edges but the idea should be clear. One missing feature is a trivial helper function that takes payload fragments and returns a contiguous chunk of memory instead (this is expensive both memory-wise and time-wise).

libudpard/udpard.c Show resolved Hide resolved
libudpard/udpard.c Show resolved Hide resolved
libudpard/udpard.c Outdated Show resolved Hide resolved
libudpard/udpard.h Outdated Show resolved Hide resolved
libudpard/udpard.h Show resolved Hide resolved
libudpard/udpard.h Outdated Show resolved Hide resolved
libudpard/udpard.h Outdated Show resolved Hide resolved
tests/src/test_intrusive_rx.c Outdated Show resolved Hide resolved
…ld naming scheme doesn't fit in the 31 character limit required by MISRA C:2012. Alternatively we could replace some other part of the name but it is hard to ensure consistency with the subject RX entities
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

86.0% 86.0% Coverage
0.0% 0.0% Duplication

@lydia-at-amazon
Copy link
Collaborator

Cons:

The udpardRx*Receive functions take ownership of the payload buffer.

Just want to document what we talked about in the dev call, the zero-copy design assumes that udpard can directly take over ownership of the buffer from the network stack. A lot of microcontrollers use DMA to store the payload buffer in a section of memory that udpard would not be able to take ownership of and would require making a copy. Are we going to provide an API to allow for this case?

@lydia-at-amazon
Copy link
Collaborator

One missing feature is a trivial helper function that takes payload fragments and returns a contiguous chunk of memory instead (this is expensive both memory-wise and time-wise).

Do we have a ticket for this? I think almost all applications are going to want the contiguous chunk of memory and not the fragments, so we're kind of feature incomplete without this function.

libudpard/udpard.h Show resolved Hide resolved
libudpard/udpard.h Show resolved Hide resolved
libudpard/udpard.h Show resolved Hide resolved
libudpard/udpard.h Show resolved Hide resolved
libudpard/udpard.c Show resolved Hide resolved
libudpard/udpard.c Show resolved Hide resolved
@pavel-kirienko
Copy link
Member Author

Are we going to provide an API to allow for this case?

The plan is to let the application copy the data from the memory managed by the NIC driver into a new buffer and move it into the library. This solution does not require new API. We can optionally provide convenience functions for this later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants