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

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 */
};

Well, the hc_metrics_lite struct is defined with visibility to non-kernel utlities (which so far haven't made use of it). Why not have only one common struct between TCP and hostcache, as well as hostcache and userland? (Will add the union).

Do you plan to update this revision to a new version after landing D58440?

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,

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?

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?

Again, i don't see any space savings - checking if a 4-byte aligned int is non-zero (IPADDR_ANY) or a flag field (also consuming as much space) and then accessing either an ip4 or ip4 struct in a union seems to have the same overhead for now. Since there is versioning, unless something clearly asks for a flag field and also allow all-zero IP addresses, I'd keep the struct like this. If it needs to be changed later, we can roll the version number. (Also, I like the version to start with 1 here, not zero - in case someone just hands over a bzeroed struct...)

Again, i don't see any space savings - checking if a 4-byte aligned int is non-zero (IPADDR_ANY) or a flag field (also consuming as much space) and then accessing either an ip4 or ip4 struct in a union seems to have the same overhead for now. Since there is versioning, unless something clearly asks for a flag field and also allow all-zero IP addresses, I'd keep the struct like this. If it needs to be changed later, we can roll the version number. (Also, I like the version to start with 1 here, not zero - in case someone just hands over a bzeroed struct...)

It is not as much about space saving as about a structure layout that is self documenting.

Not insisting on zero initial version. If you like to start with 1, go for it. Then you need to declare a #define TCP_HC_API_VERSION 1 also. What I like about zero initial version in APIs is that as long as you document that API structure shall be initialized with C99 initializer, you can pretend it is unversioned at first, while it actually can be switched to versioned later.

struct tcp_hc_entry hc = {
	.thc_ip4 = foo,
	.thc_ip6 = foo,
}

The above is definitely API misuse. With union compiler will err, without union it won't.