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

net-test: packetdrill: tcpdump: reduce sleep #81

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matttbe
Copy link
Contributor

@matttbe matttbe commented Oct 22, 2023

As suggested by Davide, TCPDump's '--immediate-mode' will force the kernel to flush packets to userspace immediately after the reception. Thanks to that, the 'sleep 1' at the end of the test is no longer needed: the userspace will have all the packets when the kill signal will be received. Just to be on the safe side, we can keep a sleep 0.1, as recommended by Willem.

It looks like this TCPDump option is usually not recommended but because there should not be too many packets generated here with Packetdrill, it seems fine to have it and saves 1 second for each test. Note that it is not an exotic option as it is used by default when packets are not written in a file. Also, we now use '--packet-buffered' to reduce IO.

Now that the "immediate mode" is being used, it seems enough to just wait for the '.pcap' file to be created to know when TCPDump is ready.

While at it, the directory where the capture will be store is created if it didn't exist.

@nealcardwell
Copy link
Collaborator

Thanks. Should we also add the --packet-buffered flag for tcpdump?

@nealcardwell
Copy link
Collaborator

Adding author @wdebruij to the thread...

@wdebruij
Copy link
Contributor

Should it keep a 0.1s sleep to avoid a tight race between last packet and close?

The proof is in the pudding. If there are no regressions in any of the (by now many) .pkt tests, sounds like a very nice speed-up indeed.

As suggested by Davide, TCPDump's '--immediate-mode' will force the
kernel to flush packets to userspace immediately after the reception.
Thanks to that, the 'sleep 1' at the end of the test is no longer
needed: the userspace will have all the packets when the kill signal
will be received. Just to be on the safe side, we can keep a sleep 0.1,
as recommended by Willem.

It looks like this TCPDump option is usually not recommended but because
there should not be too many packets generated here with Packetdrill, it
seems fine to have it and saves 1 second for each test. Note that it is
not an exotic option as it is used by default when packets are not
written in a file. Also, we now use '--packet-buffered' to reduce IO.

Now that the "immediate mode" is being used, it seems enough to just
wait for the '.pcap' file to be created to know when TCPDump is ready.

While at it, the directory where the capture will be store is created if
it didn't exist.

Suggested-by: Davide Caratti <[email protected]>
Signed-off-by: Matthieu Baerts <[email protected]>
@matttbe matttbe force-pushed the tcpdump-immediate branch 2 times, most recently from ce6716c to 74e1ded Compare October 22, 2023 16:51
@matttbe
Copy link
Contributor Author

matttbe commented Oct 22, 2023

Thank you both for the quick feedback!

Should we also add the --packet-buffered flag for tcpdump?

@nealcardwell: Good idea!

Should it keep a 0.1s sleep to avoid a tight race between last packet and close?

@wdebruij: Good idea, just to be on the safe side.

The proof is in the pudding. If there are no regressions in any of the (by now many) .pkt tests, sounds like a very nice speed-up indeed.

That's quite hard to track down regressions. I ran 'tcp' tests and check the differences of size of the produced .pkt files: most of them were the same but some not. I didn't check all of them but from what I saw, the differences were due to instabilities with some tests (e.g. tolerance issues) and packets like ICMP6, router solicitation present in some but not all.

@nealcardwell @wdebruij: I just pushed a new version containing your two suggestions but also a way to reduce the sleep just after having started tcpdump. So we can save up to 2 seconds per test now :)

On my side, it seems to do the job well but I was able to do the tests only on one setup. I would be glad if someone else can try on their side, just to be on the safe side :)

@matttbe matttbe changed the title net-test: packetdrill: tcpdump: use immediate mode net-test: packetdrill: tcpdump: reduce sleep Oct 22, 2023
Copy link
Contributor

@wdebruij wdebruij left a comment

Choose a reason for hiding this comment

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

Looks great to me as is, thanks.

@matttbe
Copy link
Contributor Author

matttbe commented Mar 22, 2024

@nealcardwell : is there anything else I need to do for this PR? :)

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