Skip to content

Add dynamic packet batching to take advantage of sendmmsg on Linux#751

Merged
phillip-stephens merged 35 commits intomainfrom
phillip/batch-packets
Dec 27, 2023
Merged

Add dynamic packet batching to take advantage of sendmmsg on Linux#751
phillip-stephens merged 35 commits intomainfrom
phillip/batch-packets

Conversation

@phillip-stephens
Copy link
Contributor

@phillip-stephens phillip-stephens commented Dec 8, 2023

  • Updated manpages
  • Updated --help text

Testing

Tested on both Mac and Ubuntu

Correctness

Ran against a smaller subnet with both main and Phillip/batch-packets and both returned same output

Performance

Inputs

  • sender-threads=4
  • batch=64
    12% speedup compared to main on lab Ubuntu box (1.40 Mp/s vs. 1.25 Mp/s)

Inputs

  • sender-threads=1
  • batch=64
    48% speedup compared to main on lab Ubuntu box (860 Kp/s vs. 551 Kp/s)

@phillip-stephens phillip-stephens marked this pull request as ready for review December 8, 2023 22:16
@phillip-stephens phillip-stephens added this to the ZMap 4.0 milestone Dec 8, 2023
@zakird
Copy link
Member

zakird commented Dec 18, 2023

@phillip-stephens it looks like this is now conflicting with another PR that we merged. Can you resolve conflicts?

@zakird zakird self-assigned this Dec 18, 2023
@phillip-stephens
Copy link
Contributor Author

@zakird LMK if you think differently, but I'm thinking that we wait on this PR and I'll integrate it after I finalize the liburing work. liburing will be used by most linux users on newer distros anyway. Either way, I've pushed my retry logic updates to address your fair point here.

@zakird
Copy link
Member

zakird commented Dec 20, 2023

@phillip-stephens I would err towards getting this merged now since the work is complete. I don't see a ton of reason to hang around as a PR. Can you tag me once you've had a chance to address @dadrian's comments?

@zakird zakird assigned phillip-stephens and unassigned zakird Dec 20, 2023
@phillip-stephens
Copy link
Contributor Author

phillip-stephens commented Dec 20, 2023

Addressed @dadrian's suggestions (thanks for that neat cache locality idea, I'm going to have to use that in the future). Assigning back to you, @zakird. Double-checked correctness after changes by running/comparing against main, all looks good.

}
}
}
if (packets_sent == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that I follow why there needs to be different logic here for packets_sent == 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should have added a comment explaining the special handling to mimic linux's send_mmsg call. I've updated this!

@zakird zakird modified the milestones: ZMap 4.0, ZMap 4.1 Dec 20, 2023
@phillip-stephens phillip-stephens merged commit cc348ab into main Dec 27, 2023
@phillip-stephens phillip-stephens deleted the phillip/batch-packets branch December 27, 2023 15:13
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.

3 participants