TcpOutSegs way too optimistic (netstat -s)

TcpOutSegs way too optimistic (netstat -s)

[Marc Herbert]

TcpOutSegs is currently incremented even when
"tp->af_specific->queue_xmit(skb, 0)" fails (think about a full qdisc
for instance).

See fix below.



===== net/ipv4/tcp_output.c 1.19 vs edited =====
--- 1.19/net/ipv4/tcp_output.c	Fri May 14 22:32:18 2004
+++ edited/net/ipv4/tcp_output.c	Fri May 14 23:19:37 2004
@@ -276,13 +276,7 @@
 		if (skb->len != tcp_header_size)
 			tcp_event_data_sent(tp, skb, sk);

-		TCP_INC_STATS(TcpOutSegs);
-
 		err = tp->af_specific->queue_xmit(skb, 0);
-		if (err <= 0)
-			return err;
-
-		tcp_enter_cwr(tp);

 		/* NET_XMIT_CN is special. It does not guarantee,
 		 * that this packet is lost. It tells that device
@@ -290,7 +284,15 @@
 		 * drops some packets of the same priority and
 		 * invokes us to send less aggressively.
 		 */
-		return err == NET_XMIT_CN ? 0 : err;
+		if(err == NET_XMIT_CN)
+			err = 0;
+		if (!err)
+			TCP_INC_STATS(TcpOutSegs);
+
+		if (err > 0)
+			tcp_enter_cwr(tp);
+
+		return err;
 	}
 	return -ENOBUFS;
 #undef SYSCTL_FLAG_TSTAMPS

Re: TcpOutSegs way too optimistic (netstat -s)

[Marc Herbert]

On Sat, 15 May 2004, Marc Herbert wrote:

>
> TcpOutSegs is currently incremented even when
> "tp->af_specific->queue_xmit(skb, 0)" fails (think about a full qdisc
> for instance).
>
> See fix below.

Oups, forget the first patch, it did not tcp_enter_cwr() when
NET_XMIT_CN.

See hopefully correct patch below. Sorry for the noise.


===== net/ipv4/tcp_output.c 1.19 vs edited =====
--- 1.19/net/ipv4/tcp_output.c	Fri May 14 22:32:18 2004
+++ edited/net/ipv4/tcp_output.c	Sat May 15 00:16:21 2004
@@ -276,13 +276,10 @@
 		if (skb->len != tcp_header_size)
 			tcp_event_data_sent(tp, skb, sk);

-		TCP_INC_STATS(TcpOutSegs);
-
 		err = tp->af_specific->queue_xmit(skb, 0);
-		if (err <= 0)
-			return err;

-		tcp_enter_cwr(tp);
+		if (err > 0)
+			tcp_enter_cwr(tp);

 		/* NET_XMIT_CN is special. It does not guarantee,
 		 * that this packet is lost. It tells that device
@@ -290,7 +287,12 @@
 		 * drops some packets of the same priority and
 		 * invokes us to send less aggressively.
 		 */
-		return err == NET_XMIT_CN ? 0 : err;
+		if(err == NET_XMIT_CN)
+			err = 0;
+		if (!err)
+			TCP_INC_STATS(TcpOutSegs);
+
+		return err;
 	}
 	return -ENOBUFS;
 #undef SYSCTL_FLAG_TSTAMPS

Re: TcpOutSegs way too optimistic (netstat -s)

[Nivedita Singhvi]

Marc Herbert wrote:

>Oups, forget the first patch, it did not tcp_enter_cwr() when
>NET_XMIT_CN.
>
>See hopefully correct patch below. Sorry for the noise.
>
>  
>

I'm not sure I agree with this patch.  Ideally, every layer should count
what it sent out, and what it drops. If a lower layer drops the segment/
packet for any reason, that is the responsibility of the lower layer to
count.  This includes the qdisc layer (a topic I will come back to).
 
If a queing discipline were counting all incoming packets, regardless
of whether they later dropped them or not, then there would be an
inconsistency between how many TCP sent out and how many
the  layer below received.

Currently, the default queing discipline statistics (dropped) are not
displayed anywhere. I have a patch that displays them in /proc/net/
along with the dev stats, which at best is a hack.

They are available if you have tc and qd support compiled, but not
via a generic statistics gathering utility  like netstat -s, a fact which
a lot of people complain about.  From previous suggestions and
conversations it was proposed that netstat -s be at least enhanced
to show qdisc drops (using netlink sockets, not /proc, although it
would be manyfold convenience to have them in /proc in some
suitable place).

I'd appreciate any thoughts/feedback on the issue :). What's the
best way to go about doing this?

thanks,
Nivedita

Re: TcpOutSegs way too optimistic (netstat -s)

[Marc Herbert]

On Fri, 14 May 2004, Nivedita Singhvi wrote:

> I'm not sure I agree with this patch.  Ideally, every layer should count
> what it sent out, and what it drops. If a lower layer drops the segment/
> packet for any reason, that is the responsibility of the lower layer to
> count.  This includes the qdisc layer (a topic I will come back to).

The interface here between these two layers is not really "dropping"
but rather "blocking".

Let's have a look at the main sending loop "tcp_write_xmit()". When
the qdisc and thus tcp_transmit_skb() returns a error (most common
cause:  qdisc full), the loop just breaks and exits WITHOUT advancing
the "send_head".

In brief, what happened ? TCP tried to give a packet to the qdisc,
which refused it. So TCP keeps it until a better time.  No packet has
disappeared, no packet has moved. Please tell me what has been "sent"
or "dropped" in this case. This TCP/IP interface really looks much
more like a "blocking" pipe rather than like some "dropping" IP
router.

The retransmission code is very similar: when the qdisc errors, the
retransmission loop is broken, the status of the tentatively
retransmitted packet is NOT updated to "retransmitted", so it will be
the first packet to be retransmitted on next pass (please confirm
this).
And BTW, not any "TcpRetransWhatever" counter is incremented in this
case (so there is at least some inconsistency here).

If the IP stack was silently dropping packets just like a external
IP router, things would be different. TCP would then not be able to
tell the difference between its local IP stack and a further router, a
packet would have disappeared indeed, TCP would skip to the next
packet in the flow and finally receive a SACK / a timeout, etc. But
this is not what happens.


The point of view of the qdisc is quite different, because the qdisc
has not only TCP as a client. But my patch is for TCP.