Page MenuHomeFreeBSD

siftr: expose t_flags2 in siftr output
ClosedPublic

Authored by thj on Mar 25 2022, 2:50 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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; 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?

201

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

pauamma 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.

201

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
chengc_netapp.com added inline comments.
sys/netinet/siftr.c
459

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

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

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

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
201

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.