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.

Sign up for GitHub

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

Jump to bottom

timestamp: support multiple trailer timestamp formats #181

Merged
merged 1 commit into from
Nov 29, 2023
Merged

timestamp: support multiple trailer timestamp formats #181

merged 1 commit into from
Nov 29, 2023

Conversation

wdebruij
Copy link
Contributor

Make packet trailer timestamp format configurable.

EF_RX_TIMESTAMPING_ORDERING allows selecting timestamps embedded in packet trailers for ordering and SO_TIMESTAMPING. The current implementation assumes a single fixed trailer format, cPacket.

Make the format selectable, with EF_RX_TIMESTAMPING_TRAILER_FORMAT. Introduce initially two new formats to demonstrate the feature.

Implementation:

  • expand ci_rx_pkt_timestamp_cpacket into ci_rx_pkt_timestamp_trailer, to demultiplex type. Minimal refactoring of the existing function to avoid code duplication. Cpacket func can be simplified further, but with more churn, so left for a v2 revision.
  • expand src/tests/unit/header/ci/internal/ip_timestamp.c with basic unit tests for the new types.

For more information on cpacket and ttag trailer formats, also see

Tested: scripts/run_unit_tests.sh

@wdebruij wdebruij requested a review from a team as a code owner October 20, 2023 15:46
mseymour-xilinx
mseymour-xilinx approved these changes Oct 24, 2023
View reviewed changes
Copy link
Contributor

@mseymour-xilinx mseymour-xilinx left a comment

Choose a reason for hiding this comment

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

Looks good. It would be nice to refactor ci_rx_pk_timestamp_cpacket as described.

@wdebruij wdebruij force-pushed the trailer-timestamp-v1 branch from df5f3a2 to 3364e68 Compare October 25, 2023 23:28
@wdebruij
Copy link
Contributor Author

Thanks for the review! I pushed a new versions with the suggestions incorporated. And a few other changes. Added a changelog to the bottom of the commit message (below the ---)

@wdebruij
timestamp: support multiple trailer timestamp formats
433a1d7 
Make packet trailer timestamp format configurable.

EF_RX_TIMESTAMPING_ORDERING allows selecting timestamps embedded in
packet trailers for ordering and SO_TIMESTAMPING. The current
implementation assumes a single fixed trailer format, cPacket.

Make the format selectable, with EF_RX_TIMESTAMPING_TRAILER_FORMAT.
Introduce initially two new formats to demonstrate the feature.

Implementation:

- expand ci_rx_pkt_timestamp_cpacket into ci_rx_pkt_timestamp_trailer,
  to demultiplex type. Minimal refactoring of the existing function to
  avoid code duplication. Cpacket func can be simplified further, but
  with more churn, so left for a v2 revision.
- expand src/tests/unit/header/ci/internal/ip_timestamp.c with basic
  unit tests for the new types.

For more information on cpacket and ttag trailer formats, also see
- https://github.com/wireshark/wireshark/blob/master/epan/dissectors/packet-metamako.c
- https://github.com/wireshark/wireshark/blob/master/epan/dissectors/packet-cisco-ttag.c

Tested: scripts/run_unit_tests.sh

---

Changes v1->v2
  - add nsec_frac initialization to ci_rx_pkt_timestamp_ttag
    and nsec_frac validation to test_rx_pkt_timestamp_trailer_ttag
  - rename Type3 to Broadcom, after getting approval
    - update origin field and document FCS behavior
  - convert ci_rx_pkt_timestamp_ttag to common ci_rx_pkt_.. arg fmt:
    - now remove duplicative tests
    - remove two levels of indentation
    - (unfortunately) have to update all direct callers in tests
    - no other changes to the logic (hard to spot without --word-diff)
  - tighten ttag and brcm tests: encode largest supported value
  - avoid ISO C90 forbids mixed declarations and code warnings
@wdebruij wdebruij force-pushed the trailer-timestamp-v1 branch from 3364e68 to 433a1d7 Compare October 26, 2023 01:35
@wdebruij
Copy link
Contributor Author

wdebruij commented Nov 9, 2023

Just a note that I pushed a commit to my own repo two tools with which I tested this end-to-end.

  • trailer_tx is a small packet socket program to send an UDP packet, optionally with a trailer timestamp in one of the three formats. This does not require Onload.
  • trailer_rx exercises onload_timestamping_request to read both the NIC and trailer timestamp (if present). This duplicates some test already in Onload, but the current format works particularly well with trailer_tx.

@cns-ci-onload-xilinx cns-ci-onload-xilinx merged commit 47c5d50 into Xilinx-CNS:master Nov 29, 2023
@abower-amd
Copy link
Collaborator

Thanks for the contribution, @wdebruij !

@wdebruij
Copy link
Contributor Author

Thanks for accepting it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mseymour-xilinx mseymour-xilinx mseymour-xilinx approved these changes

@abower-amd abower-amd abower-amd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wdebruij @abower-amd @mseymour-xilinx @cns-ci-onload-xilinx