[PATCH] SNMPv2 tcpOutSegs counter error

[PATCH] SNMPv2 tcpOutSegs counter error

[Wei Yongjun]

RFC2012 saied that tcpOutSegs should excluding those containing only
retransmitted octets. But in my test, linux kernel increased the
tcpOutSegs even if only retransmitted octets is send out.
So I think this is a bug of kernel.

Refer to RFC2012, tcpOutSegs is defined as following:

   tcpOutSegs OBJECT-TYPE
       SYNTAX      Counter32
       MAX-ACCESS  read-only
       STATUS      current
       DESCRIPTION
               "The total number of segments sent, including those on
               current connections but excluding those containing only
               retransmitted octets."
       ::= { tcp 11 }

Following is my patch:

--- a/net/ipv4/tcp_output.c	2006-06-30 13:37:38.000000000 -0400
+++ b/net/ipv4/tcp_output.c	2006-07-05 04:49:56.000000000 -0400
@@ -462,7 +462,8 @@ static int tcp_transmit_skb(struct sock 
 	if (skb->len != tcp_header_size)
 		tcp_event_data_sent(tp, skb, sk);
 
-	TCP_INC_STATS(TCP_MIB_OUTSEGS);
+	if (!(tcb->sacked & TCPCB_LOST))
+		TCP_INC_STATS(TCP_MIB_OUTSEGS);
 
 	err = icsk->icsk_af_ops->queue_xmit(skb, 0);
 	if (likely(err <= 0))

Signed-off-by: Wei Yongjun <yjwei@nanjing-fnst.com>

Re: [PATCH] SNMPv2 tcpOutSegs counter error

[David Miller]

From: Wei Yongjun <yjwei@nanjing-fnst.com>
Date: Thu, 06 Jul 2006 04:01:18 -0400

> -	TCP_INC_STATS(TCP_MIB_OUTSEGS);
> +	if (!(tcb->sacked & TCPCB_LOST))
> +		TCP_INC_STATS(TCP_MIB_OUTSEGS);

This test is not accurate enough.  For example, timer based
retransmits will not set the TCPCB_LOST bit.

I'm tempted to say to pass a flag to tcp_transmit_skb()
which says whether it is a retransmit or not, but that
function already takes way too many arguments.

Re: [PATCH] SNMPv2 tcpOutSegs counter error

[Wei Yongjun]

I modified my patch by add a flag to sacked when retransmit, and it work
well.


On Monday 24 July 2006 17:44, David Miller wrote:
> From: Wei Yongjun <yjwei@nanjing-fnst.com>
> Date: Thu, 06 Jul 2006 04:01:18 -0400
>
> This test is not accurate enough.  For example, timer based
> retransmits will not set the TCPCB_LOST bit.
>
> I'm tempted to say to pass a flag to tcp_transmit_skb()
> which says whether it is a retransmit or not, but that
> function already takes way too many arguments.

Signed-off-by: Wei Yongjun <yjwei@nanjing-fnst.com>

--- a/include/net/tcp.h	2006-07-28 16:19:14.000000000 -0400
+++ b/include/net/tcp.h	2006-08-03 17:56:35.686158424 -0400
@@ -550,6 +550,8 @@ struct tcp_skb_cb {
 
 #define TCPCB_URG		0x20	/* Urgent pointer advanced here	*/
 
+#define TCPCB_TRANS		0x40
+
 #define TCPCB_AT_TAIL		(TCPCB_URG)
 
 	__u16		urg_ptr;	/* Valid w/URG flags is set.	*/
 
--- a/net/ipv4/tcp_output.c	2006-08-03 18:05:22.425081936 -0400
+++ b/net/ipv4/tcp_output.c	2006-08-03 17:59:16.462716672 -0400
@@ -462,7 +462,8 @@ static int tcp_transmit_skb(struct sock 
 	if (skb->len != tcp_header_size)
 		tcp_event_data_sent(tp, skb, sk);
 
-	TCP_INC_STATS(TCP_MIB_OUTSEGS);
+	if(!(tcb->sacked & TCPCB_TRANS))
+		TCP_INC_STATS(TCP_MIB_OUTSEGS);
 
 	err = icsk->icsk_af_ops->queue_xmit(skb, 0);
 	if (likely(err <= 0))
@@ -1732,6 +1733,8 @@ int tcp_retransmit_skb(struct sock *sk, 
 	 */
 	TCP_SKB_CB(skb)->when = tcp_time_stamp;
 
+	TCP_SKB_CB(skb)->sacked |= TCPCB_TRANS;
+
 	err = tcp_transmit_skb(sk, skb, 1, GFP_ATOMIC);
 
 	if (err == 0) {

Re: [PATCH] SNMPv2 tcpOutSegs counter error

[David Miller]

From: Wei Yongjun <yjwei@nanjing-fnst.com>
Date: Thu, 03 Aug 2006 11:46:58 -0400

> I modified my patch by add a flag to sacked when retransmit, and it
> work well.

This is the most timing critical code path of the TCP stack output
packet processing, and you're adding a read-modify-write operation
just to get some statistics correct.

Let's look for a cheaper test, perhaps something like:

	if (!before(TCP_SKB_CB(skb)->seq, tp->snd_nxt))
		TCP_INC_STATS(TCP_MIB_OUTSEGS);

Would that work?

It should, because tp->snd_nxt always advances on new data
via update_send_head().

Re: [PATCH] SNMPv2 tcpOutSegs counter error

[David Miller]

From: Wei Yongjun <yjwei@nanjing-fnst.com>
Date: Fri, 04 Aug 2006 08:46:13 -0400

> This always correct except when do active open in tcp client, which will
> send a SYN, that segment will not be counted, even if it is not
> restrained. Maybe I can do following to fix this:
> 
> int tcp_connect(struct sock *sk)
> ...
> +	tp->snd_nxt = tp->write_seq;
> 	TCP_SKB_CB(buff)->seq = tp->write_seq++;
> 	TCP_SKB_CB(buff)->end_seq = tp->write_seq;
> -	tp->snd_nxt = tp->write_seq;
> -	tp->pushed_seq = tp->write_seq;
> 
> 	tcp_transmit_skb(sk, buff, 1, GFP_KERNEL);
> +	tp->snd_nxt = tp->write_seq;
> +	tp->pushed_seq = tp->write_seq;
> 	TCP_INC_STATS(TCP_MIB_ACTIVEOPENS);
> 
> Do you agree with me? If It is correctly, I will send this patch soon.

This looks fine and should work.

Please add some comments in tcp_connect() explaining the ordering of
assignments, so that a future developer does not break things by
accident.

Thank you.

Re: [PATCH] SNMPv2 tcpOutSegs counter error

[Wei Yongjun]

This always correct except when do active open in tcp client, which will
send a SYN, that segment will not be counted, even if it is not
restrained. Maybe I can do following to fix this:

int tcp_connect(struct sock *sk)
...
+	tp->snd_nxt = tp->write_seq;
	TCP_SKB_CB(buff)->seq = tp->write_seq++;
	TCP_SKB_CB(buff)->end_seq = tp->write_seq;
-	tp->snd_nxt = tp->write_seq;
-	tp->pushed_seq = tp->write_seq;

	tcp_transmit_skb(sk, buff, 1, GFP_KERNEL);
+	tp->snd_nxt = tp->write_seq;
+	tp->pushed_seq = tp->write_seq;
	TCP_INC_STATS(TCP_MIB_ACTIVEOPENS);

Do you agree with me? If It is correctly, I will send this patch soon.

On Thursday 03 August 2006 19:35, David Miller wrote:
> From: Wei Yongjun <yjwei@nanjing-fnst.com>
> Date: Thu, 03 Aug 2006 11:46:58 -0400
>
> > I modified my patch by add a flag to sacked when retransmit, and it
> > work well.
>
> This is the most timing critical code path of the TCP stack output
> packet processing, and you're adding a read-modify-write operation
> just to get some statistics correct.
>
> Let's look for a cheaper test, perhaps something like:
>
>  if (!before(TCP_SKB_CB(skb)->seq, tp->snd_nxt))
>   TCP_INC_STATS(TCP_MIB_OUTSEGS);
>
> Would that work?
>
> It should, because tp->snd_nxt always advances on new data
> via update_send_head().
> -
> 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

Re: [PATCH] SNMPv2 tcpOutSegs counter error

[Herbert Xu]

Wei Yongjun <yjwei@nanjing-fnst.com> wrote:
> 
> --- a/net/ipv4/tcp_output.c     2006-08-03 18:05:22.425081936 -0400
> +++ b/net/ipv4/tcp_output.c     2006-08-07 09:48:41.186372896 -0400
> @@ -462,7 +462,8 @@ static int tcp_transmit_skb(struct sock 
>        if (skb->len != tcp_header_size)
>                tcp_event_data_sent(tp, skb, sk);
> 
> -       TCP_INC_STATS(TCP_MIB_OUTSEGS);
> +       if(!before(tcb->seq, tp->snd_nxt))
> +               TCP_INC_STATS(TCP_MIB_OUTSEGS);

The general approach looks sound.  I have one esoteric question
though.  If a retransmitted packet is coalesced with one that is
yet to be transmitted (a fairly unlikely scenario, but possible I
think), should it count towards OUTSEGS?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] SNMPv2 tcpOutSegs counter error

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 07 Aug 2006 12:40:34 +1000

> The general approach looks sound.  I have one esoteric question
> though.  If a retransmitted packet is coalesced with one that is
> yet to be transmitted (a fairly unlikely scenario, but possible I
> think), should it count towards OUTSEGS?

Probably the packet should be counted to OUTSEGS if any of it contains
new data.

Re: [PATCH] SNMPv2 tcpOutSegs counter error

[Herbert Xu]

On Sun, Aug 06, 2006 at 07:44:47PM -0700, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Mon, 07 Aug 2006 12:40:34 +1000
> 
> > The general approach looks sound.  I have one esoteric question
> > though.  If a retransmitted packet is coalesced with one that is
> > yet to be transmitted (a fairly unlikely scenario, but possible I
> > think), should it count towards OUTSEGS?
> 
> Probably the packet should be counted to OUTSEGS if any of it contains
> new data.

OK, in that case Yongjun please update your patch to test against
tcb->end_seq instead of tcb->seq.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] SNMPv2 tcpOutSegs counter error

[Wei Yongjun]

Patch has been fixed.

On Friday 04 August 2006 07:23, David Miller wrote:
> From: Wei Yongjun <yjwei@nanjing-fnst.com>
> Date: Fri, 04 Aug 2006 08:46:13 -0400
>
> > This always correct except when do active open in tcp client, which
will
> > send a SYN, that segment will not be counted, even if it is not
> > restrained. Maybe I can do following to fix this:
> >
> >
> > Do you agree with me? If It is correctly, I will send this patch
soon.
>
> This looks fine and should work.
>
> Please add some comments in tcp_connect() explaining the ordering of
> assignments, so that a future developer does not break things by
> accident.

Signed-off-by: Wei Yongjun <yjwei@nanjing-fnst.com>

--- a/net/ipv4/tcp_output.c	2006-08-03 18:05:22.425081936 -0400
+++ b/net/ipv4/tcp_output.c	2006-08-07 09:48:41.186372896 -0400
@@ -462,7 +462,8 @@ static int tcp_transmit_skb(struct sock 
 	if (skb->len != tcp_header_size)
 		tcp_event_data_sent(tp, skb, sk);
 
-	TCP_INC_STATS(TCP_MIB_OUTSEGS);
+	if(!before(tcb->seq, tp->snd_nxt))
+		TCP_INC_STATS(TCP_MIB_OUTSEGS);
 
 	err = icsk->icsk_af_ops->queue_xmit(skb, 0);
 	if (likely(err <= 0))
@@ -2151,10 +2152,9 @@ int tcp_connect(struct sock *sk)
 	skb_shinfo(buff)->tso_segs = 1;
 	skb_shinfo(buff)->tso_size = 0;
 	buff->csum = 0;
+	tp->snd_nxt = tp->write_seq;
 	TCP_SKB_CB(buff)->seq = tp->write_seq++;
 	TCP_SKB_CB(buff)->end_seq = tp->write_seq;
-	tp->snd_nxt = tp->write_seq;
-	tp->pushed_seq = tp->write_seq;
 
 	/* Send it off. */
 	TCP_SKB_CB(buff)->when = tcp_time_stamp;
@@ -2164,6 +2164,11 @@ int tcp_connect(struct sock *sk)
 	sk_charge_skb(sk, buff);
 	tp->packets_out += tcp_skb_pcount(buff);
 	tcp_transmit_skb(sk, buff, 1, GFP_KERNEL);
+	/* change tp->snd_nxt after tcp_transmit_skb() to make this packet to be
+	 * counted to tcpOutSegs
+	 */
+	tp->snd_nxt = tp->write_seq;
+	tp->pushed_seq = tp->write_seq;
 	TCP_INC_STATS(TCP_MIB_ACTIVEOPENS);
 
 	/* Timer for repeating the SYN until an answer. */

Re: [PATCH] SNMPv2 tcpOutSegs counter error

[Wei Yongjun]

I used tcb->end_seq instead of tcb->seq. And add a new condition 'tcb-
seq == tcb->end_seq' to make ACK segment to be counted.

On Sunday 06 August 2006 22:48, Herbert Xu wrote:
> On Sun, Aug 06, 2006 at 07:44:47PM -0700, David Miller wrote:
> > From: Herbert Xu <herbert@gondor.apana.org.au>
> > Date: Mon, 07 Aug 2006 12:40:34 +1000
> >
> > > The general approach looks sound.  I have one esoteric question
> > > though.  If a retransmitted packet is coalesced with one that is
> > > yet to be transmitted (a fairly unlikely scenario, but possible I
> > > think), should it count towards OUTSEGS?
> >
> > Probably the packet should be counted to OUTSEGS if any of it
contains
> > new data.
>
> OK, in that case Yongjun please update your patch to test against
> tcb->end_seq instead of tcb->seq.
>
> Cheers,

Signed-off-by: Wei Yongjun <yjwei@nanjing-fnst.com>

--- a/net/ipv4/tcp_output.c	2006-08-03 18:05:22.425081936 -0400
+++ b/net/ipv4/tcp_output.c	2006-08-07 09:48:41.186372896 -0400
@@ -462,7 +462,8 @@ static int tcp_transmit_skb(struct sock 
 	if (skb->len != tcp_header_size)
 		tcp_event_data_sent(tp, skb, sk);
 
-	TCP_INC_STATS(TCP_MIB_OUTSEGS);
+	if(after(tcb->end_seq, tp->snd_nxt) || tcb->seq == tcb->end_seq)
+		TCP_INC_STATS(TCP_MIB_OUTSEGS);
 
 	err = icsk->icsk_af_ops->queue_xmit(skb, 0);
 	if (likely(err <= 0))
@@ -2151,10 +2152,9 @@ int tcp_connect(struct sock *sk)
 	skb_shinfo(buff)->tso_segs = 1;
 	skb_shinfo(buff)->tso_size = 0;
 	buff->csum = 0;
+	tp->snd_nxt = tp->write_seq;
 	TCP_SKB_CB(buff)->seq = tp->write_seq++;
 	TCP_SKB_CB(buff)->end_seq = tp->write_seq;
-	tp->snd_nxt = tp->write_seq;
-	tp->pushed_seq = tp->write_seq;
 
 	/* Send it off. */
 	TCP_SKB_CB(buff)->when = tcp_time_stamp;
@@ -2164,6 +2164,11 @@ int tcp_connect(struct sock *sk)
 	sk_charge_skb(sk, buff);
 	tp->packets_out += tcp_skb_pcount(buff);
 	tcp_transmit_skb(sk, buff, 1, GFP_KERNEL);
+	/* change tp->snd_nxt after tcp_transmit_skb() to make this packet to be
+	 * counted to tcpOutSegs
+	 */
+	tp->snd_nxt = tp->write_seq;
+	tp->pushed_seq = tp->write_seq;
 	TCP_INC_STATS(TCP_MIB_ACTIVEOPENS);
 
 	/* Timer for repeating the SYN until an answer. */

Re: [PATCH] SNMPv2 tcpOutSegs counter error

[David Miller]

From: Wei Yongjun <yjwei@nanjing-fnst.com>
Date: Mon, 07 Aug 2006 04:45:13 -0400

> I used tcb->end_seq instead of tcb->seq. And add a new condition 'tcb-
> seq == tcb->end_seq' to make ACK segment to be counted.
 ...
> Signed-off-by: Wei Yongjun <yjwei@nanjing-fnst.com>

Applied, thanks a lot.