Page MenuHomeFreeBSD

siftr: flush pkt_nodes to the log file in batch
ClosedPublic

Authored by cc on Jul 25 2023, 5:57 PM.
Tags
None
Referenced Files
F101993391: D41175.diff
Wed, Nov 6, 7:32 AM
Unknown Object (File)
Tue, Oct 29, 3:04 AM
Unknown Object (File)
Mon, Oct 28, 9:50 PM
Unknown Object (File)
Oct 4 2024, 8:29 AM
Unknown Object (File)
Oct 2 2024, 10:20 AM
Unknown Object (File)
Oct 2 2024, 4:42 AM
Unknown Object (File)
Sep 23 2024, 1:18 PM
Unknown Object (File)
Sep 21 2024, 5:30 PM

Details

Summary

Improves iperf thread performance by 13.5% in a dual-core cpu-bound testbed.

Test Plan

test result: test result of D41175

From lock_profile statistics, lock contention on "kern_alq.c:215 (spin mutex:ALD Queue)" has 50% reduction.
Compared with the iperf performance in base, this change brings 13.5% improvement in iperf performance.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 53510
Build 50401: arc lint + arc unit

Event Timeline

cc requested review of this revision.Jul 25 2023, 5:57 PM
cc edited the test plan for this revision. (Show Details)

I'd feel better if the remaining size of the buffer was length checked to avoid any possible overruns...

adds back snprintf() with buffer length check

Kindly remind of this change review. No need to rebase.

This revision is now accepted and ready to land.Sep 5 2023, 8:41 PM
rscheff added inline comments.
sys/netinet/siftr.c
506

if you do the buffer length check like this (statically assuming that the buffer will always have sufficient space for one log string), make sure that the buffer can hold MAX_LOG_MSG_LEN or more before calling down to the formatter...

I was thinking more of providing the remaining size of the allocated buffer down, and allow the snprintf to verify it won't run over. Someone may want to change the printf() string, and some of these are variable length.

BTW is there a reason for cycling over 3 messages only and not more? Was that the sweet spot between CPU savings and timeliness?

Correct a rare case when log_buf is NULL, which is returned by alq_getn().

This revision now requires review to proceed.Sep 8 2023, 9:18 PM
This revision is now accepted and ready to land.Sep 8 2023, 9:30 PM
sys/netinet/siftr.c
506

I couldn't figure out a better solution to the case if the string size is larger than MAX_LOG_MSG_LEN. The current result does not change the output. If the string is larger than MAX_LOG_MSG_LEN, the strange output will alert the user there is a string length problem.

In my private version, I have a counter that captures the max string size and it is printed in the foot note when siftr is disabled. Do you think this is acceptable?

The cycling over 3 messages is the sweet spot. I didn't find further performance improvement for "4" and larger messages. Logically, "3 == 2 + 1", it is already a waste of 50% messages allocation for low rate traffic, but 100% hit prediction for high rate traffic. Increase the messages in a batch allocation more than "3" does not seem to be reasonable either to me.

This revision was automatically updated to reflect the committed changes.