> What do you think ? If you want, as a first step we can merge your patch > as-is with surrounding #ifdef __linux__ and drop the two parts that are > not compatible with 2.4. Probably that we'll have to think about dropping > support for linux < 2.6.something for version 1.8.
I think this seems like the most reasonable solution for the time being. I agree in the long run it probably makes sense to have a wrapper around tcp_info to make things agnostic. -Joe On Wed, Aug 10, 2016 at 11:39 AM, Willy Tarreau <[email protected]> wrote: > Hi again Joe, > > On Wed, Aug 10, 2016 at 05:36:16PM +0200, Willy Tarreau wrote: > > On Wed, Aug 10, 2016 at 07:11:44AM -0700, Joe Williams wrote: > > > Hello list, > > > > > > Adding on to Theirry's work ( > > > http://git.haproxy.org/?p=haproxy.git;a=commit;h= > 6310bef51148b747f9019bd0bd67fd285eff0ae3) > > > I have added a few more fetchers for counters based on the tcp_info > struct > > > maintained by the kernel. > > > > Thanks for this! As I told you initially I thought we wouldn't need > > the extra metrics, but you proved me wrong :-) > > > > I've merged it and added your comment as the commit message. However > > I was having a doubt about the presence of older fields on older kernels, > > so I gave it a try with linux-2.6.32 and glibc 2.3.6 and it failed on me > > like this : > > > > src/proto_tcp.c: In function `get_tcp_info': > > src/proto_tcp.c:2407: error: structure has no member named `tcpi_rcv_rtt' > > src/proto_tcp.c:2408: error: structure has no member named > `tcpi_total_retrans' > > make: *** [src/proto_tcp.o] Error 1 > > make: *** Waiting for unfinished jobs.... > > > > So I'm seeing two possibilities : > > - either you don't need these ones and we simply drop them from the > patch > > (the most likely solution given that total_retrans is meaningless in > HTTP > > since it applies to the whole connection) > > > > - or we find a way to detect them and disable them at build time (I'm > > looking at this now). > > > > Please let me know, I've not pushed the commit yet, and I'd admit that > > the first option still seems the easiest to me :-/ > > From what I'm seeing it also breaks FreeBSD where only Thierry's entries > are declared, and some of yours exist with a "__" in front of their name : > > clang -Iinclude -Iebtree -Wall -O2 -g -fno-strict-aliasing > -Wdeclaration-after-statement -DTPROXY -DCONFIG_HAP_CRYPT > -DENABLE_POLL -DENABLE_KQUEUE -DCONFIG_HAPROXY_VERSION=\"1.7-dev3-f5f03ef\" > -DCONFIG_HAPROXY_DATE=\"2016/08/10\" -c -o src/proto_tcp.o src/proto_tcp.c > src/proto_tcp.c:2392:34: error: use of undeclared identifier 'SOL_TCP' > if (getsockopt(conn->t.sock.fd, SOL_TCP, TCP_INFO, &info, &optlen) > == -1) > ^ > src/proto_tcp.c:2400:35: error: no member named 'tcpi_unacked' in 'struct > tcp_info'; did you mean '__tcpi_unacked'? > case 2: smp->data.u.sint = info.tcpi_unacked; break; > ^~~~~~~~~~~~ > __tcpi_unacked > /usr/include/netinet/tcp.h:212:12: note: '__tcpi_unacked' declared here > u_int32_t __tcpi_unacked; > ^ > src/proto_tcp.c:2401:35: error: no member named 'tcpi_sacked' in 'struct > tcp_info'; did you mean '__tcpi_sacked'? > case 3: smp->data.u.sint = info.tcpi_sacked; break; > ^~~~~~~~~~~ > __tcpi_sacked > /usr/include/netinet/tcp.h:213:12: note: '__tcpi_sacked' declared here > u_int32_t __tcpi_sacked; > ^ > src/proto_tcp.c:2402:35: error: no member named 'tcpi_lost' in 'struct > tcp_info'; did you mean '__tcpi_lost'? > case 4: smp->data.u.sint = info.tcpi_lost; break; > ^~~~~~~~~ > __tcpi_lost > /usr/include/netinet/tcp.h:214:12: note: '__tcpi_lost' declared here > u_int32_t __tcpi_lost; > ^ > src/proto_tcp.c:2403:35: error: no member named 'tcpi_retrans' in 'struct > tcp_info'; did you mean '__tcpi_retrans'? > case 5: smp->data.u.sint = info.tcpi_retrans; break; > ^~~~~~~~~~~~ > __tcpi_retrans > /usr/include/netinet/tcp.h:215:12: note: '__tcpi_retrans' declared here > u_int32_t __tcpi_retrans; > ^ > src/proto_tcp.c:2404:35: error: no member named 'tcpi_fackets' in 'struct > tcp_info'; did you mean '__tcpi_fackets'? > case 6: smp->data.u.sint = info.tcpi_fackets; break; > ^~~~~~~~~~~~ > __tcpi_fackets > /usr/include/netinet/tcp.h:216:12: note: '__tcpi_fackets' declared here > u_int32_t __tcpi_fackets; > ^ > src/proto_tcp.c:2405:35: error: no member named 'tcpi_reordering' in > 'struct tcp_info'; did you mean '__tcpi_reordering'? > case 7: smp->data.u.sint = info.tcpi_reordering; break; > ^~~~~~~~~~~~~~~ > __tcpi_reordering > /usr/include/netinet/tcp.h:232:12: note: '__tcpi_reordering' declared here > u_int32_t __tcpi_reordering; > ^ > src/proto_tcp.c:2406:35: error: no member named 'tcpi_rcv_rtt' in 'struct > tcp_info' > case 8: smp->data.u.sint = info.tcpi_rcv_rtt; break; > ~~~~ ^ > src/proto_tcp.c:2407:35: error: no member named 'tcpi_total_retrans' in > 'struct tcp_info' > case 9: smp->data.u.sint = info.tcpi_total_retrans; break; > ~~~~ ^ > 9 errors generated. > Makefile:791: recipe for target 'src/proto_tcp.o' failed > gmake: *** [src/proto_tcp.o] Error 1 > > So I think we'll have to be tricky here : > > - the basic info are accessible when TCP_INFO is declared. These ones > are common between Linux 2.4-4.x and FreeBSD 10.3 : > > u_int8_t tcpi_state; /* TCP FSM state. */ > u_int8_t tcpi_options; /* Options enabled on > conn. */ > u_int8_t tcpi_snd_wscale:4, /* RFC1323 send shift > value. */ > tcpi_rcv_wscale:4; /* RFC1323 recv shift > value. */ > u_int32_t tcpi_rto; /* Retransmission timeout > (usec). */ > u_int32_t tcpi_snd_mss; /* Max segment size for > send. */ > u_int32_t tcpi_rcv_mss; /* Max segment size for > receive. */ > u_int32_t tcpi_last_data_recv; /* Time since last recv > data. */ > u_int32_t tcpi_rtt; /* Smoothed RTT in usecs. > */ > u_int32_t tcpi_rttvar; /* RTT variance in usecs. > */ > u_int32_t tcpi_snd_ssthresh; /* Slow start threshold. */ > u_int32_t tcpi_snd_cwnd; /* Send congestion window. > */ > > Thus I consider it is safe to rely only on these ones by default for now. > > - A number of other fields are available on FreeBSD with the same names as > on Linux but with "__" prepended (eg: __tcp_sacked). We could have a > #define > for this in the compat.h file. > > - some more fields are available and differ on these OSes. Afteri > "tcpi_reordering" which is the last common one, we find : > > Linux 2.4 : (nothing) > > Linux 2.6.12-3.14 : > __u32 tcpi_rcv_rtt; > __u32 tcpi_rcv_space; > > __u32 tcpi_total_retrans; > > Linux 3.15-4.0 further add : > __u64 tcpi_pacing_rate; > __u64 tcpi_max_pacing_rate; > > Linux 4.1 adds : > __u64 tcpi_bytes_acked; /* RFC4898 tcpEStatsAppHCThruOctetsAcked > */ > __u64 tcpi_bytes_received; /* RFC4898 > tcpEStatsAppHCThruOctetsReceived */ > > Linux 4.2-4.5 add : > __u32 tcpi_segs_out; /* RFC4898 tcpEStatsPerfSegsOut */ > __u32 tcpi_segs_in; /* RFC4898 tcpEStatsPerfSegsIn */ > > Linux 4.6-4.7 add : > __u32 tcpi_notsent_bytes; > __u32 tcpi_min_rtt; > __u32 tcpi_data_segs_in; /* RFC4898 tcpEStatsDataSegsIn */ > __u32 tcpi_data_segs_out; /* RFC4898 tcpEStatsDataSegsOut */ > > FreeBSD 10.3 has this instead : > u_int32_t __tcpi_rcv_rtt; > u_int32_t tcpi_rcv_space; /* Advertised recv window. > */ > > /* FreeBSD extensions to tcp_info. */ > u_int32_t tcpi_snd_wnd; /* Advertised send window. > */ > u_int32_t tcpi_snd_bwnd; /* No longer used. */ > u_int32_t tcpi_snd_nxt; /* Next egress seqno */ > u_int32_t tcpi_rcv_nxt; /* Next ingress seqno */ > u_int32_t tcpi_toe_tid; /* HWTID for TOE endpoints > */ > u_int32_t tcpi_snd_rexmitpack; /* Retransmitted packets */ > u_int32_t tcpi_rcv_ooopack; /* Out-of-order packets */ > u_int32_t tcpi_snd_zerowin; /* Zero-sized windows sent > */ > > /* Padding to grow without breaking ABI. */ > u_int32_t __tcpi_pad[26]; /* Padding. */ > > u_int32_t tcpi_rcv_space; /* Advertised recv window. > */ > > One interesting point is that FreeBSD explicitly mentions their intent to > be compatible with Linux's struct : > > * The TCP_INFO socket option comes from the Linux 2.6 TCP API, and permits > * the caller to query certain information about the state of a TCP > * connection. We provide an overlapping set of fields with the Linux > * implementation, but since this is a fixed size structure, room has been > * left for growth. In order to maximize potential future compatibility > with > * the Linux API, the same variable names and order have been adopted, and > * padding left to make room for omitted fields in case they are added > later. > > Given that we won't have the proper knowledge of these fields from the > libc itself which is not always in sync with the kernel (especially since > people run whatever OS in containers on a different kernel), I tend to > think > that in order to maximize the portability we should simply define our own > structure with our own fields, and possibly adjust it a bit depending on > the operating system (eg: linux vs freebsd). > > What do you think ? If you want, as a first step we can merge your patch > as-is with surrounding #ifdef __linux__ and drop the two parts that are > not compatible with 2.4. Probably that we'll have to think about dropping > support for linux < 2.6.something for version 1.8. > > CCing Thierry in case he has other ideas on the subject. > > Thanks, > Willy > > PS: I'm appending your current patch in which I only adjusted the commit > message > >
