diff mbox

[iproute2,3/3] ss: Unify tcp stats output

Message ID 1421613815-6635-4-git-send-email-vadim4j@gmail.com
State Rejected, archived
Headers show

Commit Message

Vadym Kochan Jan. 18, 2015, 8:43 p.m. UTC
From: Vadim Kochan <vadim4j@gmail.com>

Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
---
 misc/ss.c | 362 +++++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 231 insertions(+), 131 deletions(-)

Comments

Hagen Paul Pfeifer Jan. 19, 2015, 1:57 p.m. UTC | #1
On 18 January 2015 at 21:43, Vadim Kochan <vadim4j@gmail.com> wrote:

> Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
> ---
>  misc/ss.c | 362 +++++++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 231 insertions(+), 131 deletions(-)

Hey Vadin,

your patch do *not* change the output format, right? ss output is
parsed by scripts and tools.

hgn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vadym Kochan Jan. 19, 2015, 2:04 p.m. UTC | #2
On Mon, Jan 19, 2015 at 02:57:50PM +0100, Hagen Paul Pfeifer wrote:
> On 18 January 2015 at 21:43, Vadim Kochan <vadim4j@gmail.com> wrote:
> 
> > Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
> > ---
> >  misc/ss.c | 362 +++++++++++++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 231 insertions(+), 131 deletions(-)
> 
> Hey Vadin,
> 
> your patch do *not* change the output format, right? ss output is
> parsed by scripts and tools.
> 
> hgn

Hi,

It should not for tcp but does for memory info in the 1st patch where
the memeinfo param names were changed.

Regarding parsing of ss by scripts and tools, thats really painful
for me to see how ss outputs additional info, it is not human readable,
actually I'd like to change the output layout in the future, and add
something like online output option '-O'.

May be you can test these patches if they breaks output parsing ?

Anyway I will give up with ss output changes if we really carrying about
to keep the same output for scripts/tools.

Here are some comments from Stephen:
    http://marc.info/?l=linux-netdev&m=142129033800881&w=2

Regards,
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vadym Kochan Jan. 19, 2015, 2:28 p.m. UTC | #3
On Mon, Jan 19, 2015 at 03:28:21PM +0100, Hagen Paul Pfeifer wrote:
> Hey Vadin,
> 
> to make this short. We already discussed about changing the layout and
> Stephen nacked this. My proposal was to key:value the output. Because
> nearly all outputed data is already in this format - except the
> congestion control algo, ts, sack and tx'ed data. Where I proposed
> cc:<algo>. The key:value format has the advantages that the ordering
> do not mather anymore, An python parser would be something like split
> for whitespaces and later split for colon. Currently parsing this is a
> mess, see [1].
> 
> Anyway, the more clever idea is to add an json outputer like already
> supported by some ss modules and get rid of this mess.
> 
> Hagen
> 
> 
> [1] https://github.com/hgn/captcp/blob/master/captcp.py#L4861

Seems these patches will break your script at least for memory info
output.

I am thinking may be 1st of all it is better to make output in json
format and after this trying to make changes with human readability.

What do you think ?

Regards,
Vadim Kochan
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hagen Paul Pfeifer Jan. 19, 2015, 2:28 p.m. UTC | #4
Hey Vadin,

to make this short. We already discussed about changing the layout and
Stephen nacked this. My proposal was to key:value the output. Because
nearly all outputed data is already in this format - except the
congestion control algo, ts, sack and tx'ed data. Where I proposed
cc:<algo>. The key:value format has the advantages that the ordering
do not mather anymore, An python parser would be something like split
for whitespaces and later split for colon. Currently parsing this is a
mess, see [1].

Anyway, the more clever idea is to add an json outputer like already
supported by some ss modules and get rid of this mess.

Hagen


[1] https://github.com/hgn/captcp/blob/master/captcp.py#L4861
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Borkmann Jan. 19, 2015, 2:48 p.m. UTC | #5
On 01/19/2015 03:28 PM, Hagen Paul Pfeifer wrote:
> Hey Vadin,
>
> to make this short. We already discussed about changing the layout and
> Stephen nacked this. My proposal was to key:value the output. Because
> nearly all outputed data is already in this format - except the
> congestion control algo, ts, sack and tx'ed data. Where I proposed
> cc:<algo>. The key:value format has the advantages that the ordering
> do not mather anymore, An python parser would be something like split
> for whitespaces and later split for colon. Currently parsing this is a
> mess, see [1].
>
> Anyway, the more clever idea is to add an json outputer like already
> supported by some ss modules and get rid of this mess.

+1

I was also thinking in addition to json, that it might be useful to have
an optional ncurses top-like mode in ss. The level of detail could be
unfolded for a specific entry on demand, etc. I would not add it as a
hard library requirement, but in case ncurses headers are detected by
the configure script, it could be compiled in then. It's also easily
changeable since there's no such requirement that the way data is being
displayed needs to be stable for scripts.

> Hagen
>
> [1] https://github.com/hgn/captcp/blob/master/captcp.py#L4861
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vadym Kochan Jan. 19, 2015, 2:50 p.m. UTC | #6
On Mon, Jan 19, 2015 at 03:48:56PM +0100, Daniel Borkmann wrote:
> On 01/19/2015 03:28 PM, Hagen Paul Pfeifer wrote:
> >Hey Vadin,
> >
> >to make this short. We already discussed about changing the layout and
> >Stephen nacked this. My proposal was to key:value the output. Because
> >nearly all outputed data is already in this format - except the
> >congestion control algo, ts, sack and tx'ed data. Where I proposed
> >cc:<algo>. The key:value format has the advantages that the ordering
> >do not mather anymore, An python parser would be something like split
> >for whitespaces and later split for colon. Currently parsing this is a
> >mess, see [1].
> >
> >Anyway, the more clever idea is to add an json outputer like already
> >supported by some ss modules and get rid of this mess.
> 
> +1
> 
> I was also thinking in addition to json, that it might be useful to have
> an optional ncurses top-like mode in ss. The level of detail could be
> unfolded for a specific entry on demand, etc. I would not add it as a
> hard library requirement, but in case ncurses headers are detected by
> the configure script, it could be compiled in then. It's also easily
> changeable since there's no such requirement that the way data is being
> displayed needs to be stable for scripts.
> 
> >Hagen
> >
> >[1] https://github.com/hgn/captcp/blob/master/captcp.py#L4861

OK I will re-work series to do only refactoring/cleanups. And in future
I will keep in mind about any surprises for ss parsers.

Regards,
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hagen Paul Pfeifer Jan. 19, 2015, 3:01 p.m. UTC | #7
On 19 January 2015 at 15:28, Vadim Kochan <vadim4j@gmail.com> wrote:

> I am thinking may be 1st of all it is better to make output in json
> format and after this trying to make changes with human readability.
>
> What do you think ?

Mhh, if an JSON outputer is also provided a prioi *I* had no problems
with an incompatible change. It is awful to parse the current output.
*But* I am not sure if this opinion is shared by Stephen too. We
probably break several scripts/applications with this change. Breaking
the output *and* do not provide an stable alternative (JSON) is bad.

+1 for JSON (which is not that hard to implement)

hgn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hagen Paul Pfeifer Jan. 20, 2015, 10:29 a.m. UTC | #8
On 18 January 2015 at 21:43, Vadim Kochan <vadim4j@gmail.com> wrote:

Hey Stephen,

it is time to think about the format of the ss output - it starts to
get unreadable. Neither humans nor scripts will parse the output. See
the patch at the end,to get idea what I mean: who will understand
"fallback_mode"? I don't want to blame someone - not at all, it is
just one example. My proposal:

1) support grouping. E.g. DCTCP could be separated in the following
manner. I.e. "dctcp:[ ce_state: %d, alpha: %d, ...  ]" or
"dctcp:ce_state.%d;alpha,%d" like the socket memory output.
    This makes it easier for humans and scripts to parse the output.
Since some time ss output is extended really extensive (especially
Eric make use of this). This is not the end and additional values are
added - make the current babylon even worse.

2) add a JSON formater as soon as possible to make the output
parseable. I would do this - it is required anyway.

Any comments on this?

> +       if (s->ssthresh)
> +               printf(" ssthresh:%d", s->ssthresh);
> +
> +       if (s->dctcp && s->dctcp->enabled) {
> +               struct dctcpstat *dctcp = s->dctcp;
> +
> +               printf(" ce_state %u alpha %u ab_ecn %u ab_tot %u",
> +                               dctcp->ce_state, dctcp->alpha, dctcp->ab_ecn,
> +                               dctcp->ab_tot);
> +       } else if (s->dctcp) {
> +               printf(" fallback_mode");
> +       }
> +
> +       if (s->send_bps)
> +               printf(" send %sbps", sprint_bandw(b1, s->send_bps));
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vadym Kochan Jan. 20, 2015, 10:52 a.m. UTC | #9
On Tue, Jan 20, 2015 at 11:29:53AM +0100, Hagen Paul Pfeifer wrote:
> On 18 January 2015 at 21:43, Vadim Kochan <vadim4j@gmail.com> wrote:
> 
> Hey Stephen,
> 
> it is time to think about the format of the ss output - it starts to
> get unreadable. Neither humans nor scripts will parse the output. See
> the patch at the end,to get idea what I mean: who will understand
> "fallback_mode"? I don't want to blame someone - not at all, it is
> just one example. My proposal:
> 
> 1) support grouping. E.g. DCTCP could be separated in the following
> manner. I.e. "dctcp:[ ce_state: %d, alpha: %d, ...  ]" or
> "dctcp:ce_state.%d;alpha,%d" like the socket memory output.
>     This makes it easier for humans and scripts to parse the output.
> Since some time ss output is extended really extensive (especially
> Eric make use of this). This is not the end and additional values are
> added - make the current babylon even worse.
> 
> 2) add a JSON formater as soon as possible to make the output
> parseable. I would do this - it is required anyway.
> 
> Any comments on this?
> 
> > +       if (s->ssthresh)
> > +               printf(" ssthresh:%d", s->ssthresh);
> > +
> > +       if (s->dctcp && s->dctcp->enabled) {
> > +               struct dctcpstat *dctcp = s->dctcp;
> > +
> > +               printf(" ce_state %u alpha %u ab_ecn %u ab_tot %u",
> > +                               dctcp->ce_state, dctcp->alpha, dctcp->ab_ecn,
> > +                               dctcp->ab_tot);
> > +       } else if (s->dctcp) {
> > +               printf(" fallback_mode");
> > +       }
> > +
> > +       if (s->send_bps)
> > +               printf(" send %sbps", sprint_bandw(b1, s->send_bps));

This is not topic related but I think that it would be good to move
ss to separate dir (like bridge, tc) and split this 3700 lines into
smaller modules.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vadym Kochan Jan. 20, 2015, 11:06 a.m. UTC | #10
On Tue, Jan 20, 2015 at 11:29:53AM +0100, Hagen Paul Pfeifer wrote:
> On 18 January 2015 at 21:43, Vadim Kochan <vadim4j@gmail.com> wrote:
> 
> Hey Stephen,
> 
> it is time to think about the format of the ss output - it starts to
> get unreadable. Neither humans nor scripts will parse the output. See
> the patch at the end,to get idea what I mean: who will understand
> "fallback_mode"? I don't want to blame someone - not at all, it is
> just one example. My proposal:
> 
> 1) support grouping. E.g. DCTCP could be separated in the following
> manner. I.e. "dctcp:[ ce_state: %d, alpha: %d, ...  ]" or
> "dctcp:ce_state.%d;alpha,%d" like the socket memory output.

For me the current socket memory output looks weird, I think this one can be
better:
    skmem:(rd=%s,rcv_buf=%s,wr=%s,snd_buf=%s,fwd=%s,wr_queue=%s,oth=%s)

>     This makes it easier for humans and scripts to parse the output.
> Since some time ss output is extended really extensive (especially
> Eric make use of this). This is not the end and additional values are
> added - make the current babylon even worse.
> 
> 2) add a JSON formater as soon as possible to make the output
> parseable. I would do this - it is required anyway.
> 
> Any comments on this?
> 
> > +       if (s->ssthresh)
> > +               printf(" ssthresh:%d", s->ssthresh);
> > +
> > +       if (s->dctcp && s->dctcp->enabled) {
> > +               struct dctcpstat *dctcp = s->dctcp;
> > +
> > +               printf(" ce_state %u alpha %u ab_ecn %u ab_tot %u",
> > +                               dctcp->ce_state, dctcp->alpha, dctcp->ab_ecn,
> > +                               dctcp->ab_tot);
> > +       } else if (s->dctcp) {
> > +               printf(" fallback_mode");
> > +       }
> > +
> > +       if (s->send_bps)
> > +               printf(" send %sbps", sprint_bandw(b1, s->send_bps));

And what about processes list ? I think it would be good to have
something like:

u_str  ESTAB      0      0                  * 14046            * 8801
    process:("process#0",pid=535,fd=2)
            ("process#1",pid=535,fd=1)
            ("process#2",pid=535,fd=1)

Instead of:
u_str  ESTAB      0      0                  * 14046            * 8801
    users:(("qtile",pid=535,fd=2),("qtile",pid=535,fd=1))
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vadym Kochan Jan. 20, 2015, 11:09 a.m. UTC | #11
On Tue, Jan 20, 2015 at 12:17:03PM +0100, Daniel Borkmann wrote:
> Hi Hagen,
> 
> On 01/20/2015 11:29 AM, Hagen Paul Pfeifer wrote:
> >On 18 January 2015 at 21:43, Vadim Kochan <vadim4j@gmail.com> wrote:
> ...
> >My proposal:
> >
> >1) support grouping. E.g. DCTCP could be separated in the following
> >manner. I.e. "dctcp:[ ce_state: %d, alpha: %d, ...  ]" or
> >"dctcp:ce_state.%d;alpha,%d" like the socket memory output.
> >     This makes it easier for humans and scripts to parse the output.
> >Since some time ss output is extended really extensive (especially
> >Eric make use of this). This is not the end and additional values are
> >added - make the current babylon even worse.
> 
> I have no strong opinion on this, but I also have a limited view on
> what applications try to parse ss output in general.
> 
> As mentioned, for human readability, we should implement a top-like
> display option which is allowed to have a rather 'instable' output
> by nature and levels of detail can be folded/unfolded on demand.

I think it would be good to have as option and output some default basic
info ?
> 
> >2) add a JSON formater as soon as possible to make the output
> >parseable. I would do this - it is required anyway.
> 
> Given the recent discussion on web10g, json output option might
> be very useful to provide i.e. when stats are being further extended.
> 
> Cheers,
> Daniel
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Borkmann Jan. 20, 2015, 11:17 a.m. UTC | #12
Hi Hagen,

On 01/20/2015 11:29 AM, Hagen Paul Pfeifer wrote:
> On 18 January 2015 at 21:43, Vadim Kochan <vadim4j@gmail.com> wrote:
...
> My proposal:
>
> 1) support grouping. E.g. DCTCP could be separated in the following
> manner. I.e. "dctcp:[ ce_state: %d, alpha: %d, ...  ]" or
> "dctcp:ce_state.%d;alpha,%d" like the socket memory output.
>      This makes it easier for humans and scripts to parse the output.
> Since some time ss output is extended really extensive (especially
> Eric make use of this). This is not the end and additional values are
> added - make the current babylon even worse.

I have no strong opinion on this, but I also have a limited view on
what applications try to parse ss output in general.

As mentioned, for human readability, we should implement a top-like
display option which is allowed to have a rather 'instable' output
by nature and levels of detail can be folded/unfolded on demand.

> 2) add a JSON formater as soon as possible to make the output
> parseable. I would do this - it is required anyway.

Given the recent discussion on web10g, json output option might
be very useful to provide i.e. when stats are being further extended.

Cheers,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hagen Paul Pfeifer Jan. 20, 2015, 11:43 a.m. UTC | #13
On 20 January 2015 at 12:17, Daniel Borkmann <dborkman@redhat.com> wrote:

> I have no strong opinion on this, but I also have a limited view on
> what applications try to parse ss output in general.
>
> As mentioned, for human readability, we should implement a top-like
> display option which is allowed to have a rather 'instable' output
> by nature and levels of detail can be folded/unfolded on demand.

+1 on that.

> Given the recent discussion on web10g, json output option might
> be very useful to provide i.e. when stats are being further extended.

Ok, there seems conses on JSON output.

Stephan, what do you think?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Cong Wang Jan. 20, 2015, 6:36 p.m. UTC | #14
On Tue, Jan 20, 2015 at 2:29 AM, Hagen Paul Pfeifer <hagen@jauu.net> wrote:
> On 18 January 2015 at 21:43, Vadim Kochan <vadim4j@gmail.com> wrote:
>
> Hey Stephen,
>
> it is time to think about the format of the ss output - it starts to
> get unreadable. Neither humans nor scripts will parse the output. See
> the patch at the end,to get idea what I mean: who will understand
> "fallback_mode"? I don't want to blame someone - not at all, it is
> just one example. My proposal:
>

On the other hand, what blocks you from parsing the netlink message
by yourself? Don't get me wrong, I am not a fan of netlink, but
generally speaking, ss is not alone, too many scripts and applications
nowadays parse iproute2 tools output.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hagen Paul Pfeifer Jan. 21, 2015, 8:54 a.m. UTC | #15
20 January 2015 at 19:36, Cong Wang <cwang@twopensource.com> wrote:

> On the other hand, what blocks you from parsing the netlink message
> by yourself? Don't get me wrong, I am not a fan of netlink, but
> generally speaking, ss is not alone, too many scripts and applications
> nowadays parse iproute2 tools output.

Agree, thus the proposal:

1) *new* ss values are "prefixed" by an name to make it possible that
*humans* can understand the value ("make ss output human readable").
Concrete rule for scalar value: key:value. Concrete rule for list of
values: subsystem:(key1:val1,key1:val2). The actual ss output is
nearly formatted matching these rules. E.g. see socket memory for list
formated output. Sure, there are values like SACK that do not need any
prefix and can assumed to be known.

2) add JSON format for scripts and tools.

Hagen
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/misc/ss.c b/misc/ss.c
index 40439b3..73097b2 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -691,24 +691,59 @@  static const char *sstate_namel[] = {
 	[SS_CLOSING] = "closing",
 };
 
+struct dctcpstat
+{
+	unsigned int	ce_state;
+	unsigned int	alpha;
+	unsigned int	ab_ecn;
+	unsigned int	ab_tot;
+	bool		enabled;
+};
+
 struct tcpstat
 {
-	inet_prefix	local;
-	inet_prefix	remote;
-	int		lport;
-	int		rport;
-	int		state;
-	int		rq, wq;
-	int		timer;
-	int		timeout;
-	int		retrs;
-	unsigned	ino;
-	int		probes;
-	unsigned	uid;
-	int		refcnt;
-	unsigned long long sk;
-	int		rto, ato, qack, cwnd, ssthresh;
-	unsigned int	iface;
+	inet_prefix	    local;
+	inet_prefix	    remote;
+	int		    lport;
+	int		    rport;
+	int		    state;
+	int		    rq, wq;
+	unsigned	    ino;
+	unsigned	    uid;
+	int		    refcnt;
+	unsigned int	    iface;
+	unsigned long long  sk;
+	int		    timer;
+	int		    timeout;
+	int		    probes;
+	char		    *cong_alg;
+	double		    rto, ato, rtt, rttvar;
+	int		    qack, cwnd, ssthresh, backoff;
+	double		    send_bps;
+	int		    snd_wscale;
+	int		    rcv_wscale;
+	int		    mss;
+	unsigned int	    lastsnd;
+	unsigned int	    lastrcv;
+	unsigned int	    lastack;
+	double		    pacing_rate;
+	double		    pacing_rate_max;
+	unsigned int	    unacked;
+	unsigned int	    retrans;
+	unsigned int	    retrans_total;
+	unsigned int	    lost;
+	unsigned int	    sacked;
+	unsigned int	    fackets;
+	unsigned int	    reordering;
+	double		    rcv_rtt;
+	int		    rcv_space;
+	bool		    has_ts_opt;
+	bool		    has_sack_opt;
+	bool		    has_ecn_opt;
+	bool		    has_ecnseen_opt;
+	bool		    has_fastopen_opt;
+	bool		    has_wscale_opt;
+	struct dctcpstat    *dctcp;
 };
 
 static const char *tmr_name[] = {
@@ -1471,7 +1506,7 @@  static void inet_stats_print(struct tcpstat *s, int protocol)
 			printf(" timer:(%s,%s,%d)",
 			       tmr_name[s->timer],
 			       print_ms_timer(s->timeout),
-			       s->retrs);
+			       s->retrans);
 		}
 	}
 
@@ -1538,8 +1573,107 @@  static int proc_inet_split_line(char *line, char **loc, char **rem, char **data)
 	return 0;
 }
 
+static char *sprint_bandw(char *buf, double bw)
+{
+	return sprint_num(buf, bw, false);
+}
+
+static char *sprint_bytes(char *buf, uint64_t bytes)
+{
+	if (!show_human) {
+		sprintf(buf, "%"PRIu64, bytes);
+		return buf;
+	}
+
+	return sprint_num(buf, bytes, false);
+}
+
+static void tcp_stats_print(struct tcpstat *s)
+{
+	char b1[64];
+
+	if (s->has_ts_opt)
+		printf(" ts");
+	if (s->has_sack_opt)
+		printf(" sack");
+	if (s->has_ecn_opt)
+		printf(" ecn");
+	if (s->has_ecnseen_opt)
+		printf(" ecnseen");
+	if (s->has_fastopen_opt)
+		printf(" fastopen");
+	if (s->cong_alg)
+		printf(" %s", s->cong_alg);
+	if (s->has_wscale_opt)
+		printf(" wscale:%d,%d", s->snd_wscale, s->rcv_wscale);
+	if (s->rto)
+		printf(" rto:%g", s->rto);
+	if (s->backoff)
+		printf(" backoff:%u", s->backoff);
+	if (s->rtt)
+		printf(" rtt:%g/%g", s->rtt, s->rttvar);
+	if (s->ato)
+		printf(" ato:%g", s->ato);
+
+	if (s->qack)
+		printf(" qack:%d", s->qack);
+	if (s->qack & 1)
+		printf(" bidir");
+
+	if (s->mss)
+		printf(" mss:%d", s->mss);
+	if (s->cwnd && s->cwnd != 2)
+		printf(" cwnd:%d", s->cwnd);
+	if (s->ssthresh)
+		printf(" ssthresh:%d", s->ssthresh);
+
+	if (s->dctcp && s->dctcp->enabled) {
+		struct dctcpstat *dctcp = s->dctcp;
+
+		printf(" ce_state %u alpha %u ab_ecn %u ab_tot %u",
+				dctcp->ce_state, dctcp->alpha, dctcp->ab_ecn,
+				dctcp->ab_tot);
+	} else if (s->dctcp) {
+		printf(" fallback_mode");
+	}
+
+	if (s->send_bps)
+		printf(" send %sbps", sprint_bandw(b1, s->send_bps));
+	if (s->lastsnd)
+		printf(" lastsnd:%u", s->lastsnd);
+	if (s->lastrcv)
+		printf(" lastrcv:%u", s->lastrcv);
+	if (s->lastack)
+		printf(" lastack:%u", s->lastack);
+
+	if (s->pacing_rate) {
+		printf(" pacing_rate %sbps", sprint_bandw(b1, s->pacing_rate));
+		if (s->pacing_rate_max)
+				printf("/%sbps", sprint_bandw(b1,
+							s->pacing_rate_max));
+	}
+
+	if (s->unacked)
+		printf(" unacked:%u", s->unacked);
+	if (s->retrans || s->retrans_total)
+		printf(" retrans:%u/%u", s->retrans, s->retrans_total);
+	if (s->lost)
+		printf(" lost:%u", s->lost);
+	if (s->sacked && s->state != SS_LISTEN)
+		printf(" sacked:%u", s->sacked);
+	if (s->fackets)
+		printf(" fackets:%u", s->fackets);
+	if (s->reordering != 3)
+		printf(" reordering:%d", s->reordering);
+	if (s->rcv_rtt)
+		printf(" rcv_rtt:%g", s->rcv_rtt);
+	if (s->rcv_space)
+		printf(" rcv_space:%d", s->rcv_space);
+}
+
 static int tcp_show_line(char *line, const struct filter *f, int family)
 {
+	int rto = 0, ato = 0;
 	struct tcpstat s = {};
 	char *loc, *rem, *data;
 	char opt[256];
@@ -1561,22 +1695,27 @@  static int tcp_show_line(char *line, const struct filter *f, int family)
 	opt[0] = 0;
 	n = sscanf(data, "%x %x:%x %x:%x %x %d %d %u %d %llx %d %d %d %d %d %[^\n]\n",
 		   &s.state, &s.wq, &s.rq,
-		   &s.timer, &s.timeout, &s.retrs, &s.uid, &s.probes, &s.ino,
-		   &s.refcnt, &s.sk, &s.rto, &s.ato, &s.qack,
+		   &s.timer, &s.timeout, &s.retrans, &s.uid, &s.probes, &s.ino,
+		   &s.refcnt, &s.sk, &rto, &ato, &s.qack,
 		   &s.cwnd, &s.ssthresh, opt);
 
 	if (n < 17)
 		opt[0] = 0;
 
 	if (n < 12) {
-		s.rto = 0;
+		rto = 0;
 		s.cwnd = 2;
 		s.ssthresh = -1;
-		s.ato = s.qack = 0;
+		ato = s.qack = 0;
 	}
 
-	s.retrs = s.timer != 1 ? s.probes : s.retrs;
-	s.timeout = (s.timeout * 1000 + hz - 1) / hz;
+	s.retrans   = s.timer != 1 ? s.probes : s.retrans;
+	s.timeout   = (s.timeout * 1000 + hz - 1) / hz;
+	s.ato	    = (double)ato / hz;
+	s.qack	   /= 2;
+	s.rto	    = (double)rto;
+	s.ssthresh  = s.ssthresh == -1 ? 0 : s.ssthresh;
+	s.rto	    = s.rto != 3 * hz  ? s.rto / hz : 0;
 
 	inet_stats_print(&s, IPPROTO_TCP);
 
@@ -1589,20 +1728,8 @@  static int tcp_show_line(char *line, const struct filter *f, int family)
 			printf(" opt:\"%s\"", opt);
 	}
 
-	if (show_tcpinfo) {
-		if (s.rto && s.rto != 3 * hz)
-			printf(" rto:%g", (double)s.rto / hz);
-		if (s.ato)
-			printf(" ato:%g", (double)s.ato / hz);
-		if (s.cwnd != 2)
-			printf(" cwnd:%d", s.cwnd);
-		if (s.ssthresh != -1)
-			printf(" ssthresh:%d", s.ssthresh);
-		if (s.qack / 2)
-			printf(" qack:%d", s.qack / 2);
-		if (s.qack & 1)
-			printf(" bidir");
-	}
+	if (show_tcpinfo)
+		tcp_stats_print(&s);
 
 	printf("\n");
 	return 0;
@@ -1634,21 +1761,6 @@  outerr:
 	return ferror(fp) ? -1 : 0;
 }
 
-static char *sprint_bandw(char *buf, double bw)
-{
-	return sprint_num(buf, bw, false);
-}
-
-static char *sprint_bytes(char *buf, uint64_t bytes)
-{
-	if (!show_human) {
-		sprintf(buf, "%"PRIu64, bytes);
-		return buf;
-	}
-
-	return sprint_num(buf, bytes, false);
-}
-
 static void print_skmeminfo(struct rtattr *tb[], int attrtype)
 {
 	char buf[64];
@@ -1688,11 +1800,13 @@  static void print_skmeminfo(struct rtattr *tb[], int attrtype)
 	printf(")");
 }
 
+#define TCPI_HAS_OPT(info, opt) !!(info->tcpi_options & (opt))
+
 static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
 		struct rtattr *tb[])
 {
-	char b1[64];
 	double rtt = 0;
+	struct tcpstat s = {};
 
 	print_skmeminfo(tb, INET_DIAG_SKMEMINFO);
 
@@ -1709,39 +1823,49 @@  static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
 			info = RTA_DATA(tb[INET_DIAG_INFO]);
 
 		if (show_options) {
-			if (info->tcpi_options & TCPI_OPT_TIMESTAMPS)
-				printf(" ts");
-			if (info->tcpi_options & TCPI_OPT_SACK)
-				printf(" sack");
-			if (info->tcpi_options & TCPI_OPT_ECN)
-				printf(" ecn");
-			if (info->tcpi_options & TCPI_OPT_ECN_SEEN)
-				printf(" ecnseen");
-			if (info->tcpi_options & TCPI_OPT_SYN_DATA)
-				printf(" fastopen");
-		}
-
-		if (tb[INET_DIAG_CONG])
-			printf(" %s", rta_getattr_str(tb[INET_DIAG_CONG]));
-
-		if (info->tcpi_options & TCPI_OPT_WSCALE)
-			printf(" wscale:%d,%d", info->tcpi_snd_wscale,
-			       info->tcpi_rcv_wscale);
+			s.has_ts_opt	   = TCPI_HAS_OPT(info, TCPI_OPT_TIMESTAMPS);
+			s.has_sack_opt	   = TCPI_HAS_OPT(info, TCPI_OPT_SACK);
+			s.has_ecn_opt	   = TCPI_HAS_OPT(info, TCPI_OPT_ECN);
+			s.has_ecnseen_opt  = TCPI_HAS_OPT(info, TCPI_OPT_ECN_SEEN);
+			s.has_fastopen_opt = TCPI_HAS_OPT(info, TCPI_OPT_SYN_DATA);
+		}
+
+		if (tb[INET_DIAG_CONG]) {
+			const char *cong_attr = rta_getattr_str(tb[INET_DIAG_CONG]);
+			s.cong_alg = malloc(strlen(cong_attr + 1));
+			strcpy(s.cong_alg, cong_attr);
+		}
+
+		if (TCPI_HAS_OPT(info, TCPI_OPT_WSCALE)) {
+			s.has_wscale_opt  = true;
+			s.snd_wscale	  = info->tcpi_snd_wscale;
+			s.rcv_wscale	  = info->tcpi_rcv_wscale;
+		}
+
 		if (info->tcpi_rto && info->tcpi_rto != 3000000)
-			printf(" rto:%g", (double)info->tcpi_rto/1000);
-		if (info->tcpi_backoff)
-			printf(" backoff:%u", info->tcpi_backoff);
-		if (info->tcpi_rtt)
-			printf(" rtt:%g/%g", (double)info->tcpi_rtt/1000,
-			       (double)info->tcpi_rttvar/1000);
-		if (info->tcpi_ato)
-			printf(" ato:%g", (double)info->tcpi_ato/1000);
-		if (info->tcpi_snd_mss)
-			printf(" mss:%d", info->tcpi_snd_mss);
-		if (info->tcpi_snd_cwnd != 2)
-			printf(" cwnd:%d", info->tcpi_snd_cwnd);
+			s.rto = (double)info->tcpi_rto / 1000;
+
+		s.backoff	 = info->tcpi_backoff;
+		s.rtt		 = (double)info->tcpi_rtt / 1000;
+		s.rttvar	 = (double)info->tcpi_rttvar / 1000;
+		s.ato		 = (double)info->tcpi_rttvar / 1000;
+		s.mss		 = info->tcpi_snd_mss;
+		s.rcv_space	 = info->tcpi_rcv_space;
+		s.rcv_rtt	 = (double)info->tcpi_rcv_rtt / 1000;
+		s.lastsnd	 = info->tcpi_last_data_sent;
+		s.lastrcv	 = info->tcpi_last_data_recv;
+		s.lastack	 = info->tcpi_last_ack_recv;
+		s.unacked	 = info->tcpi_unacked;
+		s.retrans	 = info->tcpi_retrans;
+		s.retrans_total  = info->tcpi_total_retrans;
+		s.lost		 = info->tcpi_lost;
+		s.sacked	 = info->tcpi_sacked;
+		s.reordering	 = info->tcpi_reordering;
+		s.rcv_space	 = info->tcpi_rcv_space;
+		s.cwnd		 = info->tcpi_snd_cwnd;
+
 		if (info->tcpi_snd_ssthresh < 0xFFFF)
-			printf(" ssthresh:%d", info->tcpi_snd_ssthresh);
+			s.ssthresh = info->tcpi_snd_ssthresh;
 
 		rtt = (double) info->tcpi_rtt;
 		if (tb[INET_DIAG_VEGASINFO]) {
@@ -1749,67 +1873,43 @@  static void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r,
 				= RTA_DATA(tb[INET_DIAG_VEGASINFO]);
 
 			if (vinfo->tcpv_enabled &&
-			    vinfo->tcpv_rtt && vinfo->tcpv_rtt != 0x7fffffff)
+					vinfo->tcpv_rtt && vinfo->tcpv_rtt != 0x7fffffff)
 				rtt =  vinfo->tcpv_rtt;
 		}
 
 		if (tb[INET_DIAG_DCTCPINFO]) {
+			struct dctcpstat *dctcp = malloc(sizeof(struct
+						dctcpstat));
+
 			const struct tcp_dctcp_info *dinfo
 				= RTA_DATA(tb[INET_DIAG_DCTCPINFO]);
 
-			if (dinfo->dctcp_enabled) {
-				printf(" ce_state %u alpha %u ab_ecn %u ab_tot %u",
-				       dinfo->dctcp_ce_state, dinfo->dctcp_alpha,
-				       dinfo->dctcp_ab_ecn, dinfo->dctcp_ab_tot);
-			} else {
-				printf(" fallback_mode");
-			}
+			dctcp->enabled	= !!dinfo->dctcp_enabled;
+			dctcp->ce_state = dinfo->dctcp_ce_state;
+			dctcp->alpha	= dinfo->dctcp_alpha;
+			dctcp->ab_ecn	= dinfo->dctcp_ab_ecn;
+			dctcp->ab_tot	= dinfo->dctcp_ab_tot;
+			s.dctcp		= dctcp;
 		}
 
 		if (rtt > 0 && info->tcpi_snd_mss && info->tcpi_snd_cwnd) {
-			printf(" send %sbps",
-			       sprint_bandw(b1, (double) info->tcpi_snd_cwnd *
-					 (double) info->tcpi_snd_mss * 8000000.
-					 / rtt));
+			s.send_bps = (double) info->tcpi_snd_cwnd *
+				(double)info->tcpi_snd_mss * 8000000. / rtt;
 		}
 
-		if (info->tcpi_last_data_sent)
-			printf(" lastsnd:%u", info->tcpi_last_data_sent);
-
-		if (info->tcpi_last_data_recv)
-			printf(" lastrcv:%u", info->tcpi_last_data_recv);
-
-		if (info->tcpi_last_ack_recv)
-			printf(" lastack:%u", info->tcpi_last_ack_recv);
-
 		if (info->tcpi_pacing_rate &&
-		    info->tcpi_pacing_rate != ~0ULL) {
-			printf(" pacing_rate %sbps",
-				sprint_bandw(b1, info->tcpi_pacing_rate * 8.));
+				info->tcpi_pacing_rate != ~0ULL) {
+			s.pacing_rate = info->tcpi_pacing_rate * 8.;
 
 			if (info->tcpi_max_pacing_rate &&
-			    info->tcpi_max_pacing_rate != ~0ULL)
-				printf("/%sbps",
-					sprint_bandw(b1, info->tcpi_max_pacing_rate * 8.));
-		}
-		if (info->tcpi_unacked)
-			printf(" unacked:%u", info->tcpi_unacked);
-		if (info->tcpi_retrans || info->tcpi_total_retrans)
-			printf(" retrans:%u/%u", info->tcpi_retrans,
-			       info->tcpi_total_retrans);
-		if (info->tcpi_lost)
-			printf(" lost:%u", info->tcpi_lost);
-		if (info->tcpi_sacked && r->idiag_state != SS_LISTEN)
-			printf(" sacked:%u", info->tcpi_sacked);
-		if (info->tcpi_fackets)
-			printf(" fackets:%u", info->tcpi_fackets);
-		if (info->tcpi_reordering != 3)
-			printf(" reordering:%d", info->tcpi_reordering);
-		if (info->tcpi_rcv_rtt)
-			printf(" rcv_rtt:%g", (double) info->tcpi_rcv_rtt/1000);
-		if (info->tcpi_rcv_space)
-			printf(" rcv_space:%d", info->tcpi_rcv_space);
-
+					info->tcpi_max_pacing_rate != ~0ULL)
+				s.pacing_rate_max = info->tcpi_max_pacing_rate * 8.;
+		}
+		tcp_stats_print(&s);
+		if (s.dctcp)
+			free(s.dctcp);
+		if (s.cong_alg)
+			free(s.cong_alg);
 	}
 }
 
@@ -1830,7 +1930,7 @@  static int inet_show_sock(struct nlmsghdr *nlh, struct filter *f, int protocol)
 	s.rq = r->idiag_rqueue;
 	s.timer = r->idiag_timer;
 	s.timeout = r->idiag_expires;
-	s.retrs = r->idiag_retrans;
+	s.retrans = r->idiag_retrans;
 	s.ino = r->idiag_inode;
 	s.uid = r->idiag_uid;
 	s.iface = r->id.idiag_if;