Add sysctl flag CTLFLAG_TUN to loader tunables
ClosedPublic
Actions

Authored by zlei on Sep 21 2023, 7:44 AM.

Details

Reviewers

glebius
rscheff

Group Reviewers

network
transport

Commits

Summary

These are loader tunables. Add sysctl flag CTLFLAG_TUN for them so that sysctl -T will report them correctly.

No functional change intended.

MFC after: 3 days

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

zlei requested review of this revision.Sep 21 2023, 7:44 AM

rscheff requested changes to this revision.Sep 24 2023, 11:13 AM

rscheff added a subscriber: rscheff.

rscheff added inline comments.

sys/netinet/tcp_sack.c
129 ↗	(On Diff #127592)	Not sure why tcp_sack made it here; SACK itself is not normally a stand-alone, loadable module; SACK is an integral part of the base stack, which itself can be make a loadable module (provided another stack is there). Also, why only flag one variable here (even though I don't think SACK should have this; neither do the sysctl variable kept in tcp_ecn.c ).

This revision now requires changes to proceed.Sep 24 2023, 11:13 AM

sys/netinet/tcp_sack.c
129 ↗	(On Diff #127592)	Not sure why tcp_sack made it here; I think it is self contained. `net.inet.tcp.sack.xxx` goes into `sys/netinet/tcp_sack.c`. SACK itself is not normally a stand-alone, loadable module; SACK is an integral part of the base stack, which itself can be make a loadable module (provided another stack is there). Yes that's true. Well, it does not matter whether `SACK` is a stand-alone or an integral part of the base stack. SYSCTLs tunable works for both cases. Also, why only flag one variable here (even though I don't think SACK should have this; neither do the sysctl variable kept in tcp_ecn.c ). I'm not getting you. Do you hint we should not have tunable `net.inet.tcp.sack.enable` ? It is actually a loader tunable, it is initialized by `tcp_vnet_init()` in `sys/netinet/tcp_subr.c`: TUNABLE_INT_FETCH("net.inet.tcp.sack.enable", &V_tcp_do_sack);

tuexen added inline comments.

sys/netinet/tcp_sack.c
129 ↗	(On Diff #127592)	Not sure why tcp_sack made it here; I think it is self contained. `net.inet.tcp.sack.xxx` goes into `sys/netinet/tcp_sack.c`. SACK itself is not normally a stand-alone, loadable module; SACK is an integral part of the base stack, which itself can be make a loadable module (provided another stack is there). Yes that's true. Well, it does not matter whether `SACK` is a stand-alone or an integral part of the base stack. SYSCTLs tunable works for both cases. Also, why only flag one variable here (even though I don't think SACK should have this; neither do the sysctl variable kept in tcp_ecn.c ). I'm not getting you. Do you hint we should not have tunable `net.inet.tcp.sack.enable` ? It is actually a loader tunable, it is initialized by `tcp_vnet_init()` in `sys/netinet/tcp_subr.c`: TUNABLE_INT_FETCH("net.inet.tcp.sack.enable", &V_tcp_do_sack); And why is it a loader tunable?

zlei added inline comments.

sys/netinet/tcp_sack.c
129 ↗	(On Diff #127592)	And why is it a loader tunable? That's a good question ;) A quick look at the usage of `V_tcp_do_sack`, I do not think it is necessary to be a loader tunable (at least for stable/12). It was first introduced by @rwatson in R136967 [1] about 19 years ago. I do not have more context about that, so no clues why it was introduced. I guess at that time the tcp stack requires it to be a loader tunable so that the stack can initialize properly. So comes a new concern. It sounds like a breakage if we remove `TUNABLE_INT_FETCH("net.inet.tcp.sack.enable", &V_tcp_do_sack);` . Or we can bravely do that, presuming nobody have `net.inet.tcp.sack.enable` in his / her `/boot/loader.conf` . https://svnweb.freebsd.org/base?view=revision&revision=136967

sys/netinet/tcp_sack.c
129 ↗	(On Diff #127592)	OK, I'll leave it as is. We can do that, remove `TUNABLE_INT_FETCH("net.inet.tcp.sack.enable", &V_tcp_do_sack)` , for current/15 anyway.

sys/netinet/tcp_sack.c
129 ↗	(On Diff #127592)	It was first introduced by @rwatson in R136967 [1] about 19 years ago. I do not have more context about that, so no clues why it was introduced. I guess at that time the tcp stack requires it to be a loader tunable so that the stack can initialize properly. I looked at the `stable/5` code and don't see a reason why it should be a load tunable. I don't think it is needed for initializing the TCP stack.
129 ↗	(On Diff #127592)	OK, I'll leave it as is. We can do that, remove `TUNABLE_INT_FETCH("net.inet.tcp.sack.enable", &V_tcp_do_sack)` , for current/15 anyway. I will bring this up in the FreeBSD Transport call. Maybe some knows a reason why it is needed.