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

fix(iroh-net): split packets on send #1380

Merged
merged 3 commits into from
Aug 21, 2023
Merged

fix(iroh-net): split packets on send #1380

merged 3 commits into from
Aug 21, 2023

Conversation

rklaehn
Copy link
Contributor

@rklaehn rklaehn commented Aug 21, 2023

this seems to be the safest way to fix possible GSO/GRO issues on all platforms (Linux and ???) that support them.

Description

This is a fix for the possible issues described in #1357 that does the simplest possible thing - just split up the packets before sending them to the derper.

Note that we still get GRO/GSO when we have a direct connection. And since the stream to the derper is a byte stream, unless we flush after each send this should still be rather efficient. The only downside is that the derper has to deal with more individual packets.

Notes & open questions

Does this fix the issue?

Change checklist

  • Self-review.
  • Documentation updates if relevant.
  • Tests if relevant.

this seems to be the safest way to fix possible GSO/GRO issues on all
platforms (Linux and ???) that support them.
@rklaehn
Copy link
Contributor Author

rklaehn commented Aug 21, 2023

@Arqu once you have a way to test things, can you check if this solves the issue?

Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

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

I think generally this approach is probably the right call for fixing this. we need to do some GSO-like thing properly on the derper channel anyway later.

iroh-net/src/magicsock.rs Outdated Show resolved Hide resolved
iroh-net/src/magicsock.rs Show resolved Hide resolved
that way we don't have to reimplement the chunking logic. The downside is
that we have to use slice_ref which is a bit scary since it just panics
if the content is not a subset. But in this case it is fine I think.
@Arqu Arqu closed this Aug 21, 2023
@Arqu Arqu reopened this Aug 21, 2023
@rklaehn rklaehn marked this pull request as ready for review August 21, 2023 10:33
@rklaehn
Copy link
Contributor Author

rklaehn commented Aug 21, 2023

I did the following to validate this:

  • log in to a linux box
  • enforce derper use
    image
  • configure a local derper
  • run a local derper
  • try with split_packets being a noop - not splitting => transfer does not work
  • try with split_packets as in this PR - splitting on segment_size => transfer does work

If things are messed up, you get messages like this:

2023-08-21T11:49:53.757198Z TRACE quinn_proto::connection: decryption failed with packet number 5
2023-08-21T11:49:53.757211Z DEBUG quinn_proto::connection: failed to authenticate packet

@rklaehn rklaehn requested review from dignifiedquire and flub August 21, 2023 12:48
@rklaehn rklaehn added this pull request to the merge queue Aug 21, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 21, 2023
@rklaehn rklaehn added this pull request to the merge queue Aug 21, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 21, 2023
@rklaehn rklaehn added this pull request to the merge queue Aug 21, 2023
Merged via the queue into main with commit 57a2dee Aug 21, 2023
@dignifiedquire dignifiedquire deleted the use-segment-size branch November 1, 2023 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants