Message-ID: <AANLkTi=W-=8zhQpfJZ=DrDyUCwrT==5JkwRBiHaDkVXK@mail.gmail.com>
Date:	Thu, 17 Mar 2011 21:33:50 -0700
From:	Jerry Chu <hkchu@...gle.com>
To:	Stephen Hemminger <shemminger@...tta.com>
Cc:	Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH] Add useful per-connection TCP stats for diagnosis purpose.

On Thu, Mar 17, 2011 at 2:20 PM, Stephen Hemminger
<shemminger@...tta.com> wrote:
> On Thu, 17 Mar 2011 13:16:15 -0700
> Jerry Chu <hkchu@...gle.com> wrote:
>
>> Eric, thanks for the prompt feedback.
>>
>> On Thu, Mar 17, 2011 at 1:42 AM, Eric Dumazet <eric.dumazet@...il.com> wrote:
>> > Le jeudi 17 mars 2011 à 01:06 -0700, H.K. Jerry Chu a écrit :
>> >> From: Jerry Chu <hkchu@...gle.com>
>> >>
>> >> This patch add a number of very useful counters/stats (defined in
>> >> tcp_stats.h) to help diagnosing TCP related problems.
>> >>
>> >> create_time     - when the connection was created (in jiffies)
>> >> total_inbytes   - total inbytes as consumed by the receiving apps.
>> >> total_outbytes  - total outbytes sent down from the transmitting apps.
>> >>
>> >> total_outdatasegs - total data carrying segments sent so far, including
>> >>               retransmitted ones.
>> >>
>> >> total_xmit      - total accumulated time (usecs) when the connection
>> >>               has something to send.
>> >>
>> >> total_retrans_time - total time (usecs, accumulated) the connection
>> >>               spends trying to recover lost packets. For each
>> >>               loss event the time is measured from the lost packet
>> >>               was first sent till the retransmitted packet was
>> >>               eventually ack'ed.
>> >>
>> >> total_cwnd_limit - total time (usecs, excluding time spent on loss
>> >>               recovery) the xmit is stopped due to cwnd limited
>> >>
>> >> total_swnd_limit - total time (usecs) theconnection is swnd limited
>> >>
>> >> The following two counters are for listeners only:
>> >>
>> >> accepted_reqs   - total # of accepted connection requests.
>> >> listen_drops    - total # of dropped SYN reqs (SYN cookies excluded) due
>> >>               to listener's queue overflow.
>> >>
>> >> total_retrans_time/total_retrans ratio gives a rough picture of how
>> >> quickly in average the connection can recover from a pkt loss. E.g.,
>> >> when the network is more congested, or the traffic contains mainly
>> >> smaller RPC where tail drop often requires RTO to recover,
>> >> the total_retrans_time/total_retrans ratio tends to be higher.
>> >>
>> >> Currently the new counters/stats are exported through /proc/net/tcp.
>> >
>> > Please dont. Use iproute2 instead.
>> >
>> >> Some simple, abbreviated field names have been added to the output of
>> >> /proc/net/tcp in order to allow backward/forward compatibility in the
>> >> future. Obviously the new counters/stats can also be easily exported
>> >> through other APIs.
>> >>
>> >
>> > /proc/net/tcp is legacy. You should touch it eventually, but after
>> > "other APIS" are done. It was the old way (quick but a bit ugly)
>>
>> Understood. /proc/net/tcp is a much more expedient way of exporting these
>> counters because it doesn't requires any additional, special tool to read it,
>> unless other APIs (e.g., netlink). Note that backward compatibility to
>> /proc/net/tcp has been ensured by adding field names in the heading.
>>
>> >
>> >
>> >
>> >> Signed-off-by: H.K. Jerry Chu <hkchu@...gle.com>
>> >> ---
>> >>  include/linux/ktime.h    |    3 ++
>> >>  include/linux/tcp.h      |    1 +
>> >>  include/net/tcp_stats.h  |   65 ++++++++++++++++++++++++++++++++++++++++++++++
>> >>  net/ipv4/tcp.c           |   30 ++++++++++++++++++---
>> >>  net/ipv4/tcp_input.c     |   13 +++++++++
>> >>  net/ipv4/tcp_ipv4.c      |   41 ++++++++++++++++++++++++++---
>> >>  net/ipv4/tcp_minisocks.c |    9 ++++++
>> >>  net/ipv4/tcp_output.c    |   47 +++++++++++++++++++++++++++++++--
>> >>  net/ipv6/tcp_ipv6.c      |    8 +++++
>> >>  9 files changed, 206 insertions(+), 11 deletions(-)
>> >>  create mode 100644 include/net/tcp_stats.h
>> >>
>> >> diff --git a/include/linux/ktime.h b/include/linux/ktime.h
>> >> index e1ceaa9..e60e758 100644
>> >> --- a/include/linux/ktime.h
>> >> +++ b/include/linux/ktime.h
>> >> @@ -333,6 +333,9 @@ extern void ktime_get_ts(struct timespec *ts);
>> >>  /* Get the real (wall-) time in timespec format: */
>> >>  #define ktime_get_real_ts(ts)        getnstimeofday(ts)
>> >
>> > Hmm, this kind of changes are out of netdev scope and should be avoided
>>
>> Ok. (It was moved out of tcp_stats.h only at the last minute.)
>>
>> >
>> >>
>> >> +#define ktime_since(a)               ktime_to_us(ktime_sub(ktime_get(), (a)))
>> >
>> > us are implied in ktime_since() ? thats strange.
>>
>> Ok.
>>
>> >
>> >> +#define ktime_zero(a)                ktime_equal((a), ktime_set(0, 0))
>> >
>> > ktime_zero() sounds like : "give me zero time" or "clear the ktime
>> > field".
>>
>> Yes I actually have been flip-flopping on the name...
>>
>> >
>> >> +
>> >>  static inline ktime_t ns_to_ktime(u64 ns)
>> >>  {
>> >>       static const ktime_t ktime_zero = { .tv64 = 0 };
>> >> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
>> >> index e64f4c6..ea5cb5d 100644
>> >> --- a/include/linux/tcp.h
>> >> +++ b/include/linux/tcp.h
>> >> @@ -460,6 +460,7 @@ struct tcp_sock {
>> >>        * contains related tcp_cookie_transactions fields.
>> >>        */
>> >>       struct tcp_cookie_values  *cookie_values;
>> >> +     struct tcp_stats        *conn_stats;
>> >>  };
>> >
>> > Really, using separate cache lines to store some stats is expensive.
>> > You should add counters in existing structure, to avoid additional cache
>> > line dirties. Carefully placing stats in already dirtied cache lines.
>>
>> This was how it was done initially but then we wanted to allow future
>> extension to include possibly a lot more counters, something like Web100
>> (RFC4898). For the latter the memory/performance hit will likely require
>> a config option, and a separate structure will make this easier. Does it
>> make sense?
>>
>> >
>> > You also should use native ktime_t infrastructure, to make the maths
>> > really fast in fast path.
>> >
>> > Only when stats are to be returned to user, you'll have to convert the
>> > native timestamps to user exportable ones.
>>
>> Good point! Will do. (I mistakenly thought ktime_t is a larger structure.)
>>
>> >
>> > Quite frankly, using u64 fields allow nanosec resolution.
>>
>> I wish to use less bits because the final report only needs ms or even
>> sec resolution but the intermediate computation needs to capture usec
>> resolution.
>>
>> >
>> > BTW, we probably could 'export' sk->sk_drops for TCP, like we do for
>> > UDP.
>>
>> There are many other potentially useful counters/stats (spurious_retrans,
>> min_rtt, total_rto,...) but there is a tradeoff against memory/performance hit
>> so for the first round I'm focusing on what i feel is the most useful set.
>>
>> Thanks,
>>
>> Jerry
>
> These stats are best added via netlink, the tool ss already prints lots
> of similar stats. Look at INET_DIAG_INFO and the output of:
>  ss -i -t

Yes I'm familiar with ss and how inet_csk_diag_fill() calls into
tcp_diag_get_info(),..., etc. Like I mentioned previously the netlink
interface
requires a new version of reader app (e.g., ss) to ship every time a new
counter is added and exported by the kernel, whereas /proc/net/tcp does
not have such a problem.

Anyway I can add a INET_DIAG_INFO_EXT of some sort to netlink in
addition to /proc/net/tcp. That's easy to do.

Thanks,

Jerry

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

Powered by blists - more mailing lists