Page MenuHomeFreeBSD

siftr: expose t_flags2 in siftr output
ClosedPublic

Authored by thj on Mar 25 2022, 2:50 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, May 3, 6:53 PM
Unknown Object (File)
Apr 5 2024, 7:33 AM
Unknown Object (File)
Mar 29 2024, 10:33 AM
Unknown Object (File)
Mar 18 2024, 9:02 AM
Unknown Object (File)
Mar 10 2024, 10:28 PM
Unknown Object (File)
Mar 4 2024, 8:09 AM
Unknown Object (File)
Feb 1 2024, 12:35 AM
Unknown Object (File)
Jan 24 2024, 10:42 PM

Details

Summary

Replace the old snd_bwnd field which was kept for compatibility with the
t_flags2 field from the tcpcb. This exposes in siftr logs interesting things
such as ECN, PLPMTUD, Accurate ECN and if first bytes are complete.

Test Plan

I tested this with the following script with the siftr kernel module loaded and
sysctl net.inet.tcp.ecn.enable set to 1. The output shows the unique values of
t_flags2 and flags for input and output. Creating a TCP connection with ECN
shows the correct flags are set in the siftr log.

#!/bin/sh

# stop siftr
sudo sysctl net.inet.siftr.enabled=0
# remove any old siftr log
sudo rm $(sysctl -n net.inet.siftr.logfile)

sudo sysctl net.inet.siftr.enabled=1

#iperf3 -c 192.168.100.50
nc -w 1 192.168.100.50 5201

sudo sysctl net.inet.siftr.enabled=0

echo done with the siftr part of this

cat /var/log/siftr.log | grep -E "^i|^o" | awk -F "," '{ printf("%s t_flags2\t0x%08x\n", $1, $10) }' | sort | uniq
cat /var/log/siftr.log | grep -E "^i|^o" | awk -F "," '{ printf("%s tcp flags\t0x%08x\n", $1, $19) }' | sort | uniq

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

thj requested review of this revision.Mar 25 2022, 2:50 PM
sys/netinet/siftr.c
135

Do you need 100 characters more? How many are currently needed?

202

On LP64 platforms this introduces an implicit 32 bit padding. Is this intended? Is it OK?

pauamma_gundo.com added inline comments.
share/man/man4/siftr.4
265

s/state/value/ maybe? Otherwise, needs an audience check and maybe clarification of what you mean.

This for patching this up. My comments are inline.

sys/netinet/siftr.c
135

At NetApp, we also use 300 chars because sometimes the IPv6 records are too long. I think the max IPv6 record can be over 280 chars.

Thanks for patching this up.

202

The types of snd_cwnd, snd_wnd, rcv_wnd and snd_ssthresh can also be changed to uint32_t. I think the size will be reduced this way. Will the padding be gone after these uint32_t changes?

thj marked an inline comment as not done.
  • Update field sizes to match tcpcb
  • Update format string
cc requested changes to this revision.Apr 4 2022, 1:24 PM
cc added inline comments.
sys/netinet/siftr.c
460

Looks one extra "%u" is added by mistake.

This revision now requires changes to proceed.Apr 4 2022, 1:24 PM
  • Remove extra format specifier

Looks good to me. Thanks!

I will let other reviewers do the final accept for transport.

rscheff added inline comments.
sys/netinet/siftr.c
202

Do we need to retain binary compatibility in this struct? If yes, that retain u_long; if not then move t_flags2 down below flags (which should also address the padding; personally I prefer uint32_t to get a known field site over int).

This revision is now accepted and ready to land.Apr 7 2022, 7:15 AM
debdrup added a subscriber: debdrup.

Manual page looks good to me, but please remember to bump .Dd before pushing. ;)

sys/netinet/siftr.c
202

I think we only offer a stable interface for the output.

The struct is local to this file. I am happy to land this and roll back if it causes trouble in main.

This revision was automatically updated to reflect the committed changes.