Looks like you do modify the BBR/RACK stack too, unlike the high level description. In both places the same checks used to be done in a mirror image - and this again mirrors each other.

In D58408#1342736, @glebius wrote:

In D58408#1342421, @rscheff wrote:

Yes, the external struct I have now named tcp_xhc_metrics in my branch; I'll keep the individual ip4/ip6 structs though, as including the full in_conninfo seems excessive to me,

I agree about in_conninfo. I'd actually be happy if nothing outside inpcb code uses it. But why don't you want to union IPv4/IPv6 using your own anonymous union?

• make sysctl configurables uint

In D58424#1342278, @glebius wrote:

I agree that switching to unsigned for time intervals should be more correct. But the struct tcp_hostcache members now are int and that makes the sysctls to be signed. If structure members are also changed, a more thorough review of all the code that uses expire and prune needed. @rscheff since you already catched one difficult signedness bug in this code, maybe you can do such sweep, too?

• complete renaming from hc_metrics_lite to tcp_hc_metrics, including abbreviated variable names

Yes, the external struct I have now named tcp_xhc_metrics in my branch; I'll keep the individual ip4/ip6 structs though, as including the full in_conninfo seems excessive to me,

• also restart the callout so the next pruning will occur at the expected time

In D58408#1340479, @glebius wrote:

This structure is used for KPI between TCP and the TCP hostcache module. An API structure for userland tools to fetch (possibly inject?) TCP hostcache entries should not be mixed with this one.

Suggested structure:

struct tcp_hc_entry {
        u_int             thc_version;        /* current version is 0 */
        uint32_t        thc_flags;
#define THC_IP4 0x00000001
#define THC_IP6 0x00000002
        union {
		struct in_addr	thc_ip4;            /* IP address */
		struct in6_addr	thc_ip6;            /* IP6 address */
        };
        uint32_t        thc_mtu;         /* MTU for this path */
        uint32_t        thc_ssthresh;    /* outbound gateway buffer limit */
        uint32_t        thc_rtt;         /* estimated round trip time */
        uint32_t        thc_rttvar;      /* estimated rtt variance */
        uint32_t        thc_cwnd;        /* congestion window */
        uint32_t        thc_sendpipe;    /* outbound delay-bandwidth product */
        uint32_t        thc_recvpipe;    /* inbound delay-bandwidth product */
        uint32_t        thc_expire;      /* lifetime for object */
};

• fill new fields with relevant data

Note to self: rrs mentioned a difference in the internal rtt formats between base and rack stacks - verify the current state here

• add versioning and sysctl blob fields to userland hostcache struct

In D58356#1339602, @glebius wrote:

• provide a way to check for the mere existance, and contents of an entry for a given IP address.

I found that there is actual precedence in sysctl to have custom handling code for CTL_OPAQUE datastructures there - from very long ago (checked in 24 yrs ago).

Since there appear only two instances of comparisons where negative values are relevant in the full kernel source tree, I think the most progressive way would then be to simply also typecast the atomic_load_int here.

There are apparently is only one other instances with a comparison for lower than zero, and in this other instance, a typecast is also used; everything else compares to non-zero values, exactly zero, etc...

Adding Marc as he initially put in the typechecking - which doesn't appear to be doing all that much though.

In D58360#1338657, @glebius wrote:

That sounds like pretty serious compiler bug. What is the compiler and what is the machine arch?

In D58358#1338485, @glebius wrote:

This is definitely a useful functionality but the implementation will raise concerns from developers outside of network stack area. We don't want to add string parsing procedures to the kernel.

The correct implementation should use a binary API and string parsing should happen in the userspace. The tcpdrop(8) seems like a nice place to add this functionality to avoid a new utility.

• just do the refactory here

• add a tcp_hlen check oversight and indentation

• fix typo

• updating phabricator to show proper diff

@tuexen is currently looking to create standard compliant packetdrill scripts to check various test cases.

unintentional creation instead of updating D56535

you can close/abandon the diff as the original submitter. or change it entirely to what Gleb suggests.

Reading these changes, i looks good to perform more consistently and properly handling the pointers; the logical flow is reordered as described - while the small formatting nits make this change appear a bit more expansive while not changing the flow dramatically.

Thanks,
I'll discuss this with all the transport committers in the next call.

Checking a connecting in TIMEWAIT's irs doesn't make any sense to me - the actually in-window sequence numbers of the previous connection could be anywhere, depending on how much data was sent by the time the session got closed.

I understand that this helps with debugging, as ENUMs get printed as symbols. However, since these are, as Michael points out, quite unrelated to each other, and defining multiple enum identifiers may help catch incorrect assignments by the compiler, why not split the ENUM up into multiple, independently identified enums, still with explicit assignments?

In D55460#1269353, @rrs wrote:

Have a look at state diagram.
When you close your writing end, you send a FIN and enter FIN WAIT 1. When you receive the ACK for this FIN, you enter FIN WAIT 2. From the perspective of your peer, there is no difference if you are in FIN WAIT 1 or FIN WAIT 2 (or ESTABLISHED). The peer can send data. As long as it can send data, ECN should be usable, I think.

I will update the review.. however I have trepidation on this whole issue. You have two ack-regimes going on.

1. TCP's normal acking -- delayed ack or ack every packet
2. ECN commanding acks.

Now the interaction of the two regimes could prove interesting with un-anticipated consequences. Especially in
state machine transitions. So though yes, FIN WAIT 1/2 probably should be included in the exception list, I just
feel we could have issues down the road.. not sure what, but just the interactions of these algorithms could
be a src of problems... In ESTABLISHED I see no problem worse that happens is you end up acking every packet.
But in any other state I wonder...

I will increase your exclusions.. but I am concerned...

Personally I would like a TCP session NOT fail because it has ECN negotiated over a virtio driver, who then throws an error when a TSO superframe with CWR is handed down...

LGTM

No. lets abandon it.

LGTM

Reducing the number of challenge ACKs when closing out the connection makes sense.

• bump man date, use roundup(), fix various places which would indicate more than 0.1 sec granularity

• touch man date, rebase

In D54718#1250105, @amy.vargas_netapp.com wrote:

I have not. I'm still getting familiar with the process. Should I submit there first and if so, would the new version of tcpdump just be picked up by FreeBSD with no need for this review?

• bump date of tcp.4

• bump date of tcp.4

In D52225#1193159, @tuexen wrote:

Is D52226 good enough or do you have suggestions to improve the clarity?

content appears to be a better matching what the code does. Thx.

Maybe also put an updated syncookies(4) man page update into this diff. Currently, that man page is not really clear on the distinctions between sysctl net.inet.tcp.syncookies and sysctl net.inet.tcp.syncookies_only. (the _only is mentioned, but not the relationship between syncache, syncookie and syncookie_only ).

Thanks for catching

I now wonder why i had the left_edge adjustment conditional on an fack adjustment only. But that may have preceeded some other modifications. LGTM

• rebase before commit