Re: [PATCH] SNMPv2 tcpOutSegs counter error
From: Wei Yongjun <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> Applied, thanks a lot. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SNMPv2 tcpOutSegs counter error
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 <[EMAIL PROTECTED]> > > 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 <[EMAIL PROTECTED]> --- 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. */ - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SNMPv2 tcpOutSegs counter error
On Sun, Aug 06, 2006 at 07:44:47PM -0700, David Miller wrote:
> From: Herbert Xu <[EMAIL PROTECTED]>
> 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~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SNMPv2 tcpOutSegs counter error
From: Herbert Xu <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SNMPv2 tcpOutSegs counter error
Wei Yongjun <[EMAIL PROTECTED]> 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~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SNMPv2 tcpOutSegs counter error
Patch has been fixed. On Friday 04 August 2006 07:23, David Miller wrote: > From: Wei Yongjun <[EMAIL PROTECTED]> > 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 <[EMAIL PROTECTED]> --- 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. */ - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SNMPv2 tcpOutSegs counter error
From: Wei Yongjun <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SNMPv2 tcpOutSegs counter error
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 <[EMAIL PROTECTED]> > 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 [EMAIL PROTECTED] > More majordomo info at http://vger.kernel.org/majordomo-info.html - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SNMPv2 tcpOutSegs counter error
From: Wei Yongjun <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SNMPv2 tcpOutSegs counter error
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 <[EMAIL PROTECTED]>
> 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 <[EMAIL PROTECTED]>
--- a/include/net/tcp.h 2006-07-28 16:19:14.0 -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_TRANS0x40
+
#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) {
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] SNMPv2 tcpOutSegs counter error
From: Wei Yongjun <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] SNMPv2 tcpOutSegs counter error
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.0 -0400
+++ b/net/ipv4/tcp_output.c 2006-07-05 04:49:56.0 -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 <[EMAIL PROTECTED]>
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
