Re: resurrecting tcphealth

[Stephen Hemminger]

I am not sure if the is really necessary since the most
of the stats are available elsewhere.

Here are some comments on getting the simplified to match
the kernel style.

>
> static inline struct tcp_sock *tcp_sk(const struct sock *sk)
>diff -rub A/net/ipv4/tcp_input.c B/net/ipv4/tcp_input.c
>--- A/net/ipv4/tcp_input.c	2012-06-22 20:37:50.000000000 +0200
>+++ B/net/ipv4/tcp_input.c	2012-07-06 10:12:12.000000000 +0200
>@@ -4414,6 +4415,8 @@
> 		}
>
> 		if (!after(TCP_SKB_CB(skb)->end_seq, tp->rcv_nxt)) {
>+			/* Course retransmit inefficiency- this packet has been received twice. */
>+			tp->dup_pkts_recv++;

I don't understand that comment, could you use a better sentence please?

>
> 	tp->rx_opt.saw_tstamp = 0;
>
>+	/*
>+	 *	Tcp health monitoring is interested in
>+	 *	total per-connection packet arrivals.
>+	 *	This is in the fast path, but is quick.
>+	 */
>+	tp->pkts_recv++;
>+

Comment seems bigger justification than necessary for simple
operation.

>diff -rub A/net/ipv4/tcp_ipv4.c B/net/ipv4/tcp_ipv4.c
>--- A/net/ipv4/tcp_ipv4.c	2012-06-22 20:37:50.000000000 +0200
>+++ B/net/ipv4/tcp_ipv4.c	2012-07-11 09:34:22.000000000 +0200
>@@ -2533,6 +2533,82 @@
> 	return 0;
> }
>
>+
>+/*
>+ *	Output /proc/net/tcphealth
>+ */
>+#define LINESZ 128
>+
>+int tcp_health_seq_show(struct seq_file *seq, void *v)
>+{
>+	int len, num;
>+	char srcIP[32], destIP[32];
Unnecessary see below

>+
>+	unsigned long  SmoothedRttEstimate,
>+		AcksSent, DupAcksSent, PktsRecv, DupPktsRecv;

Do not use CamelCase in kernel code.

>+	struct tcp_iter_state *st;
>+
>+	if (v == SEQ_START_TOKEN) {
>+		seq_printf(seq,
>+		"TCP Health Monitoring (established connections only)\n"
>+		" -Duplicate ACKs indicate lost or reordered packets on the
>connection.\n"
>+		" -Duplicate Packets Received signal a slow and badly inefficient
>connection.\n"
>+		" -RttEst estimates how long future packets will take on a round trip
>over the connection.\n"
>+		"id   Local Address        Remote Address       RttEst(ms) AcksSent "

Header seems excessive, just put one line of header please.


>+		"DupAcksSent PktsRecv DupPktsRecv\n");
>+		goto out;
>+	}
>+
>+	/* Loop through established TCP connections */
>+	st = seq->private;
>+
>+
>+	if (st->state == TCP_SEQ_STATE_ESTABLISHED)
>+	{
>+/*	; //insert read-lock here */

Don't think you need read-lock

>+		const struct tcp_sock *tp = tcp_sk(v);
>+		const struct inet_sock *inet = inet_sk(v);
>+		__be32 dest = inet->inet_daddr;
>+		__be32 src = inet->inet_rcv_saddr;
>+		__u16 destp = ntohs(inet->inet_dport);
>+		__u16 srcp = ntohs(inet->inet_sport);
>+

These temp variables aren't redundant.

>+		num = st->num;
>+		SmoothedRttEstimate = (tp->srtt >> 3);
>+		AcksSent = tp->acks_sent;
>+		DupAcksSent = tp->dup_acks_sent;
>+		PktsRecv = tp->pkts_recv;
>+		DupPktsRecv = tp->dup_pkts_recv;
>+
>+		sprintf(srcIP, "%lu.%lu.%lu.%lu:%u",
>+			((src >> 24) & 0xFF), ((src >> 16) & 0xFF), ((src >> 8) & 0xFF), (src &
>0xFF),
>+			srcp);
>+		sprintf(destIP, "%3d.%3d.%3d.%3d:%u",
>+			((dest >> 24) & 0xFF), ((dest >> 16) & 0xFF), ((dest >> 8) & 0xFF),
>(dest & 0xFF),
>+			destp);
>+
>+		seq_printf(seq, "%d: %-21s %-21s "
>+				"%8lu %8lu %8lu %8lu %8lu%n",
>+				num,
>+				srcIP,
>+				destIP,
>+				SmoothedRttEstimate,
>+				AcksSent,
>+				DupAcksSent,
>+				PktsRecv,
>+				DupPktsRecv,
>+
>+				&len
>+			);
>+

Kernel has %pI4 to print IP addresses. 

	seq_printf(seq, "%d: %-21pI4 %-21pI4 "
			"%8lu %8lu %8lu %8lu %8lu\n",
			num,
			&inet->inet_rcv_saddr,
			&inet->inet_daddr,
			tp->srtt >> 3,
			tp->acks_sent,
			tp->dup_acks_sent,
			tp->pkts_recv,
			tp->dup_pkts_recv);
    
>+		seq_printf(seq, "%*s\n", LINESZ - 1 - len, "");

This padding of line is bogus, just print variable length line.
Are you trying to make it fixed length record file?


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/