Socket buffer sizes with autotuning
Subsystems (3)
I've been seeing the same problem here and am trying to fix it. My fix is to not count those pkts still in the host queue as "prior_in_flight" when feeding the latter to tcp_cong_avoid(). This should cause tcp_is_cwnd_limited() test to fail when the previous in_flight build-up is all due to the large host queue, and stop the cwnd to grow beyond what's really necessary.
"sysctl_tcp_tso_win_divisor causes cwnd" also unnecessarily inflates cwnd quite a bit when TSO is enabled.
Jerry
From: David Miller <[email protected]>
Quoted 45 lines
Date: Tue, Apr 22, 2008 at 8:59 PM
Subject: Re: Socket buffer sizes with autotuning
To: [email protected]
Cc: [email protected], [email protected]From: "John Heffner" <[email protected]>
Date: Tue, 22 Apr 2008 19:17:39 -0700Quoted 6 lines · Rick Jones
Quoted 5 lines
oslowest:~# netstat -an | grep ESTAB ... tcp 0 2760560 10.208.0.1:40500 10.208.0.45:42049 ESTABLISHED ...Quoted 6 lines
Quoted 1 line
Is this expected behaviour?
What is your interface txqueuelen and mtu? If you have a very large interface queue, TCP will happily fill it up unless you are using a delay-based congestion controller.
Yes, that's the fundamental problem with loss based congestion control. If there are any queues in the path, TCP will fill them up.
Vegas and other similar techniques are able to avoid this, but come with the fundamental flaw that it's easy to get them into situations where they do not respond to increases in pipe space adequately, and thus underperform compared to loss based algorithms.
--
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
Quoted 7 lines · Jerry Chu
I've been seeing the same problem here and am trying to fix it.
My fix is to not count those pkts still in the host queue as "prior_in_flight" when feeding the latter totcp_cong_avoid(). This should causetcp_is_cwnd_limited()test to fail when the previousin_flightbuild-up is all due to the large host queue, and stop the cwnd to grow beyond what's really necessary.
Sounds like a useful optimization. Do you have a patch?
-John
Quoted 11 lines · John Heffner
Quoted 7 lines · Jerry Chu
I've been seeing the same problem here and am trying to fix it.
My fix is to not count those pkts still in the host queue as "prior_in_flight" when feeding the latter totcp_cong_avoid(). This should causetcp_is_cwnd_limited()test to fail when the previousin_flightbuild-up is all due to the large host queue, and stop the cwnd to grow beyond what's really necessary.Sounds like a useful optimization. Do you have a patch?
Am working on one, but still need to completely rootcause the problem first, and do a lot more testing. I, like Rick Jones, have for a while thought either the autotuning, or the Congestion Window Validation (rfc2861) code should dampen the cwnd growth so the bug must be there, until last week when I decided to get to the bottom of this problem.
One question: I currently use skb_shinfo(skb)->dataref == 1 for skb's on the sk_write_queue list as the heuristic to determine if a packet has hit the wire. This seems a good solution for the normal cases without requiring changes to the driver to notify TCP in the xmit completion path. But I can imagine there may be cases where another below-IP consumer of skb, e.g., tcpdump, can nullify the above heuristic. If the below IP consumer causes the skb ref count to drop to 1 prematurally, well the inflated cwnd problem comes back but it's no worse than before. What if the below IP skb reader causes the skb ref count to remain > 1 while pkts have long hit the wire? This may cause the fix to prevent cwnd from growing when needed, hence hurting performance. Is there a better solution than checking against dataref to determine if a pkt has hit the wire?
Also the code to determine when/how much to defer in the TSO path seems too aggressive. It's currently based on a percentage (sysctl_tcp_tso_win_divisor)
of min(snd_wnd, snd_cwnd). Would it be too much if the value is large? E.g., when I disable sysctl_tcp_tso_win_divisor, the cwnd of my simple netperf run drops exactly 1/3 from 1037 (segments) to 695. It seems to me the TSO defer factor should be based on an absolute count, e.g., 64KB.
Jerry
Quoted 3 lines
-John
David Miller
maintainerFrom: "Jerry Chu" <[email protected]>
Date: Thu, 24 Apr 2008 17:49:33 -0700
Quoted 2 lines
One question: I currently use skb_shinfo(skb)->dataref == 1 for skb's on the
sk_write_queuelist as the heuristic to determine if a packet has hit the wire.
This doesn't work for the reasons that you mention in detail next :-)
Quoted 2 lines
Is there a better solution than checking against dataref to determine if a pkt has hit the wire?
Unfortunately, no there isn't.
Part of the issue is that the driver is only working with a clone, but if a packet gets resent before the driver gives up it's reference, we'll make a completely new copy.
But even assuming we could say that the driver gets a clone all the time, the "sent" state would need to be in the shared data area.
Quoted 7 lines
Also the code to determine when/how much to defer in the TSO path seems too aggressive. It's currently based on a percentage (
sysctl_tcp_tso_win_divisor)
ofmin(snd_wnd, snd_cwnd). Would it be too much if the value is large? E.g., when I disablesysctl_tcp_tso_win_divisor, the cwnd of my simple netperf run drops exactly 1/3 from 1037 (segments) to 695. It seems to me the TSO defer factor should be based on an absolute count, e.g., 64KB.
This is one of the most difficult knobs to get right in the TSO code.
If the percentage is too low, you'll notice that cpu utilization increases because you aren't accumulating enough data to send down the largest possible TSO frames.
But yes you are absolutely right that we should have a hard limit of 64K here, since we can't build a larger TSO frame anyways.
In fact I thought we had something like that here already :-/
Wait, in fact we do, it's just hidden behind a variable now:
/* If a full-sized TSO skb can be sent, do it. */ if (limit >= sk->sk_gso_max_size) goto send_now;
:-)
Quoted 10 lines · David Miller
From: "Jerry Chu" <[email protected]>
Date: Thu, 24 Apr 2008 17:49:33 -0700Quoted 2 lines
One question: I currently use skb_shinfo(skb)->dataref == 1 for skb's on the
sk_write_queuelist as the heuristic to determine if a packet has hit the wire.This doesn't work for the reasons that you mention in detail next :-)
Quoted 8 lines
Quoted 2 lines
Is there a better solution than checking against dataref to determine if a pkt has hit the wire?
Unfortunately, no there isn't.
Part of the issue is that the driver is only working with a clone, but if a packet gets resent before the driver gives up it's reference, we'll make a completely new copy.
I think we can ignore this case if it happens rarely.
Quoted 3 lines
But even assuming we could say that the driver gets a clone all the time, the "sent" state would need to be in the shared data area.
Ok.
Quoted 10 lines
Quoted 9 lines
Also the code to determine when/how much to defer in the TSO path seems too aggressive. It's currently based on a percentage (
sysctl_tcp_tso_win_divisor)
ofmin(snd_wnd, snd_cwnd). Would it be too much if the value is large? E.g., when I disablesysctl_tcp_tso_win_divisor, the cwnd of my simple netperf run drops exactly 1/3 from 1037 (segments) to 695. It seems to me the TSO defer factor should be based on an absolute count, e.g., 64KB.
Quoted 5 lines
This is one of the most difficult knobs to get right in the TSO code.
If the percentage is too low, you'll notice that cpu utilization increases because you aren't accumulating enough data to send down the largest possible TSO frames.
Well, there is a fine line to walk before CPU efficiency and traffic burstiness. The TSO defer code causes a few hundred KB of bursts that quickly blow away our small switch buffers. The matter may get even worse for 10GE.
Quoted 11 lines
But yes you are absolutely right that we should have a hard limit of 64K here, since we can't build a larger TSO frame anyways.
In fact I thought we had something like that here already :-/
Wait, in fact we do, it's just hidden behind a variable now:
/* If a full-sized TSO skb can be sent, do it. */ if (limit >= sk->sk_gso_max_size) goto send_now;
Oh, just realized I've been working on a very "old" (2.6.18 :-) version of kernel.
Will get the latest 2.6.25 and take a look. I can't find "skb_release_all()" function you pointed in a later mail either. Guess the Linux kernel code is rewritten every few month :-(.
Jerry
Quoted 3 lines
:-)
David Miller
maintainerFrom: "Jerry Chu" <[email protected]>
Date: Fri, 25 Apr 2008 14:29:25 -0700
Quoted 3 lines
Will get the latest 2.6.25 and take a look. I can't find "
skb_release_all()" function you pointed in a later mail either. Guess the Linux kernel code is rewritten every few month :-(.
You want bugs fixed, like the one you reported here, don't you?
Quoted 42 lines · David Miller
From: "Jerry Chu" <[email protected]>
Date: Thu, 24 Apr 2008 17:49:33 -0700Quoted 2 lines
One question: I currently use skb_shinfo(skb)->dataref == 1 for skb's on the
sk_write_queuelist as the heuristic to determine if a packet has hit the wire.This doesn't work for the reasons that you mention in detail next :-)
Quoted 2 lines
Is there a better solution than checking against dataref to determine if a pkt has hit the wire?
Unfortunately, no there isn't.
Part of the issue is that the driver is only working with a clone, but if a packet gets resent before the driver gives up it's reference, we'll make a completely new copy.
But even assuming we could say that the driver gets a clone all the time, the "sent" state would need to be in the shared data area.
Quoted 7 lines
Also the code to determine when/how much to defer in the TSO path seems too aggressive. It's currently based on a percentage (
sysctl_tcp_tso_win_divisor)
ofmin(snd_wnd, snd_cwnd). Would it be too much if the value is large? E.g., when I disablesysctl_tcp_tso_win_divisor, the cwnd of my simple netperf run drops exactly 1/3 from 1037 (segments) to 695. It seems to me the TSO defer factor should be based on an absolute count, e.g., 64KB.This is one of the most difficult knobs to get right in the TSO code.
If the percentage is too low, you'll notice that cpu utilization increases because you aren't accumulating enough data to send down the largest possible TSO frames.
But yes you are absolutely right that we should have a hard limit of 64K here, since we can't build a larger TSO frame anyways.
In fact I thought we had something like that here already :-/
Quoted 7 lines
Wait, in fact we do, it's just hidden behind a variable now:
/* If a full-sized TSO skb can be sent, do it. */ if (limit >= sk->sk_gso_max_size) goto send_now;:-)
Correct, but its counterpart doesn't exist in tcp_is_cwnd_limited(). So cwnd will continue to grow when left < cwnd/sysctl_tcp_tso_win_divisor, which can be very large when cwnd is large.
If I change tcp_tso_win_divisor to 0, cwnd max out at 695 rather than 1037, exactly off by 1/3. I tried to add the same check to tcp_is_cwnd_limited():
diff -c /tmp/tcp.h.old tcp.h
*** /tmp/tcp.h.old Mon Apr 28 11:00:44 2008
--- tcp.h Mon Apr 28 10:54:10 2008
***************
*** 828,833 ****
--- 828,835 ----
return 0;
left = tp->snd_cwnd - in_flight;
+ if (left >= 65536)
+ return 0;
if (sysctl_tcp_tso_win_divisor)
return left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd;
elseQuoted 1 line
But it doesn't seem to help (cwnd still grows to 1037).
Jerry
Quoted 24 lines · Jerry Chu
Correct, but its counterpart doesn't exist in
tcp_is_cwnd_limited(). So cwnd will continue to grow when left < cwnd/sysctl_tcp_tso_win_divisor, which can be very large when cwnd is large.If I change
tcp_tso_win_divisorto 0, cwnd max out at 695 rather than 1037, exactly off by 1/3. I tried to add the same check totcp_is_cwnd_limited():diff -c
/tmp/tcp.h.oldtcp.h
***/tmp/tcp.h.oldMon Apr 28 11:00:44 2008
---tcp.hMon Apr 28 10:54:10 2008
***************
*** 828,833 ****
--- 828,835 ----return 0; left = tp->snd_cwnd - in_flight; + if (left >= 65536) + return 0; if (sysctl_tcp_tso_win_divisor) return left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd; elseQuoted 1 line
Quoted 1 line
But it doesn't seem to help (cwnd still grows to 1037).
Cwnd is in packets, not bytes.
Try this series of patches, against net-next.
-John
From 57039cfc486ae4deac3b1fecaa238b5483c034df Mon Sep 17 00:00:00 2001
From: John Heffner <[email protected]>
Date: Mon, 28 Apr 2008 12:17:49 -0700
Subject: [PATCH 1/2] [NET]: Allow send-limited cwnd to grow up to max_burst when gso disabled
This changes the logic in tcp_is_cwnd_limited() so that cwnd may grow up to tcp_max_burst() even when sk_can_gso() is false, or when sysctl_tcp_tso_win_divisor != 0.
--- net/ipv4/tcp_cong.c | 11 ++++------- 1 files changed, 4 insertions(+), 7 deletions(-)
net/ipv4/tcp_cong.c +4 -7
@@ -285,14 +285,11 @@ int tcp_is_cwnd_limited(const struct sock *sk, u32 in_flight)if (in_flight >= tp->snd_cwnd)return 1 ; -if (!sk_can_gso (sk)) -return 0 ; - left = tp->snd_cwnd - in_flight; -if (sysctl_tcp_tso_win_divisor) -return left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd; -else -return left <=tcp_max_burst (tp); +if (sk_can_gso (sk) && + left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd) +return 1 ; +return left <=tcp_max_burst (tp); }EXPORT_SYMBOL_GPL (tcp_is_cwnd_limited);
Signature · 1 line
1.5.2.5
From fc1d9fe99748f3811a0a5b9d93463cf0e4788f56 Mon Sep 17 00:00:00 2001
From: John Heffner <[email protected]>
Date: Mon, 28 Apr 2008 12:19:22 -0700
Subject: [PATCH 2/2] [NET]: Limit cwnd growth when deferring for GSO
This fixes inappropriately large cwnd growth on sender-limited flows when GSO is enabled, limiting cwnd growth to 64k.
--- net/ipv4/tcp_cong.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
net/ipv4/tcp_cong.c +2 -1
@@ -287,7 +287,8 @@ int tcp_is_cwnd_limited(const struct sock *sk, u32 in_flight) left = tp->snd_cwnd - in_flight;if (sk_can_gso (sk) && - left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd) + left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd && + left * tp->mss_cache < sk->sk_gso_max_size)return 1 ;return left <=tcp_max_burst (tp); }
Signature · 1 line
1.5.2.5
Quoted 26 lines · John Heffner
Quoted 24 lines · Jerry Chu
Correct, but its counterpart doesn't exist in
tcp_is_cwnd_limited(). So cwnd will continue to grow when left < cwnd/sysctl_tcp_tso_win_divisor, which can be very large when cwnd is large.If I change
tcp_tso_win_divisorto 0, cwnd max out at 695 rather than 1037, exactly off by 1/3. I tried to add the same check totcp_is_cwnd_limited():diff -c
/tmp/tcp.h.oldtcp.h
***/tmp/tcp.h.oldMon Apr 28 11:00:44 2008
---tcp.hMon Apr 28 10:54:10 2008
***************
*** 828,833 ****
--- 828,835 ----return 0; left = tp->snd_cwnd - in_flight; + if (left >= 65536) + return 0; if (sysctl_tcp_tso_win_divisor) return left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd; elseQuoted 1 line
Quoted 3 lines
Quoted 1 line
But it doesn't seem to help (cwnd still grows to 1037).
Cwnd is in packets, not bytes.
Yes, of course.
Quoted 2 lines
Try this series of patches, against net-next.
Ah, you already know about this problem. Yes it does the trick.
Jerry
Quoted 3 lines
-John
PATCH 1/2 : Allow send-limited cwnd to grow up to max_burst when gso disabled
This changes the logic in tcp_is_cwnd_limited() so that cwnd may grow up to tcp_max_burst() even when sk_can_gso() is false, or when sysctl_tcp_tso_win_divisor != 0.
--- net/ipv4/tcp_cong.c | 11 ++++------- 1 files changed, 4 insertions(+), 7 deletions(-)
net/ipv4/tcp_cong.c +4 -7
@@ -285,14 +285,11 @@ int tcp_is_cwnd_limited(const struct sock *sk, u32 in_flight)if (in_flight >= tp->snd_cwnd)return 1 ; -if (!sk_can_gso (sk)) -return 0 ; - left = tp->snd_cwnd - in_flight; -if (sysctl_tcp_tso_win_divisor) -return left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd; -else -return left <=tcp_max_burst (tp); +if (sk_can_gso (sk) && + left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd) +return 1 ; +return left <=tcp_max_burst (tp); }EXPORT_SYMBOL_GPL (tcp_is_cwnd_limited);
Signature · 7 lines
1.5.2.5 ______________________________________________________________________ This email has been scanned by the MessageLabs Email Security System. For more information please visit http://www.messagelabs.com/email ______________________________________________________________________
PATCH 2/2 : Limit cwnd growth when deferring for GSO
This fixes inappropriately large cwnd growth on sender-limited flows when GSO is enabled, limiting cwnd growth to 64k.
--- net/ipv4/tcp_cong.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
net/ipv4/tcp_cong.c +2 -1
@@ -287,7 +287,8 @@ int tcp_is_cwnd_limited(const struct sock *sk, u32 in_flight) left = tp->snd_cwnd - in_flight;if (sk_can_gso (sk) && - left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd) + left * sysctl_tcp_tso_win_divisor < tp->snd_cwnd && + left * tp->mss_cache < sk->sk_gso_max_size)return 1 ;return left <=tcp_max_burst (tp); }
Signature · 7 lines
1.5.2.5 ______________________________________________________________________ This email has been scanned by the MessageLabs Email Security System. For more information please visit http://www.messagelabs.com/email ______________________________________________________________________
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 3 lines · Jerry Chu
Quoted 1 line
Try this series of patches, against net-next.
Ah, you already know about this problem. Yes it does the trick.
I had not actually known about this. (Just cooked up the patches after I saw your mail.) This looks to me like a clear bug. After looking carefully at the code there, it seems there was another small problem, too, though not likely to have much effect.
Dave, can you apply the patches I just sent?
Thanks,
-John
David Miller
maintainerFrom: "John Heffner" <[email protected]>
Date: Mon, 28 Apr 2008 16:28:16 -0700
Quoted 1 line
Dave, can you apply the patches I just sent?
Sure thing.
Just for the record, in my netperf RR (1MB request/20B reply) over 1GE/tg3 test on top of 2.6.18, changing tcp_tso_win_divisor to 0 from the default of 3 gets the cwnd down to 695 from 1037. Applying the bound check (left * tp->mss_cache < 65536) will get cwnd down to 737, not 695. Still big improvement though. (Anyway this may be moot after I fix the real culprit.)
Jerry
Quoted 16 lines · John Heffner
Quoted 3 lines · Jerry Chu
Quoted 1 line
Try this series of patches, against net-next.
Ah, you already know about this problem. Yes it does the trick.
I had not actually known about this. (Just cooked up the patches after I saw your mail.) This looks to me like a clear bug. After looking carefully at the code there, it seems there was another small problem, too, though not likely to have much effect.
Dave, can you apply the patches I just sent?
Thanks,
-John
David Miller
maintainerFrom: "Jerry Chu" <[email protected]>
Date: Wed, 23 Apr 2008 16:29:58 -0700
Quoted 6 lines
I've been seeing the same problem here and am trying to fix it. My fix is to not count those pkts still in the host queue as "
prior_in_flight" when feeding the latter totcp_cong_avoid(). This should causetcp_is_cwnd_limited()test to fail when the previousin_flightbuild-up is all due to the large host queue, and stop the cwnd to grow beyond what's really necessary.
Does something like the following suit your needs?
include/linux/skbuff.h +1 -0
@@ -140,6 +140,7 @@ struct skb_frag_struct {*/ struct skb_shared_info {atomic_t dataref; +atomic_t *in_flight;unsigned short nr_frags;unsigned short gso_size;/* Warning: this field is not always filled in (UFO)! */
include/linux/tcp.h +2 -0
@@ -271,6 +271,8 @@ struct tcp_sock {u32 rcv_tstamp;/* timestamp of last received ACK (for keepalives) */ u32 lsndtime;/* timestamp of last sent data packet (for restart window) */ +atomic_t host_inflight;/* packets queued in transmit path */ +/* Data for direct copy to user */ struct {struct sk_buff_head prequeue;
net/core/skbuff.c +6 -0
@@ -212,6 +212,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,/* make sure we initialize shinfo sequentially */ shinfo =skb_shinfo (skb);atomic_set (&shinfo->dataref,1 ); + shinfo->in_flight =NULL ; shinfo->nr_frags =0 ; shinfo->gso_size =0 ; shinfo->gso_segs =0 ;
@@ -403,6 +404,8 @@ static void skb_release_all(struct sk_buff *skb)void __kfree_skb (struct sk_buff *skb) {skb_release_all (skb); +if (skb_shinfo (skb)->in_flight) +atomic_dec (skb_shinfo (skb)->in_flight);kfree_skbmem (skb); }
@@ -486,6 +489,8 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)atomic_set (&n->users,1 );atomic_inc (&(skb_shinfo (skb)->dataref)); +if (skb_shinfo (skb)->in_flight) +atomic_inc (skb_shinfo (skb)->in_flight); skb->cloned =1 ;return n;
@@ -743,6 +748,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, skb->hdr_len =0 ; skb->nohdr =0 ;atomic_set (&skb_shinfo (skb)->dataref,1 ); +skb_shinfo (skb)->in_flight =NULL ;return 0 ; nodata:
net/ipv4/tcp.c +1 -0
@@ -479,6 +479,7 @@ static inline void skb_entail(struct sock *sk, struct sk_buff *skb)struct tcp_sock *tp =tcp_sk (sk);struct tcp_skb_cb *tcb =TCP_SKB_CB (skb); +skb_shinfo (skb)->in_flight = &tp->host_inflight; skb->csum =0 ; tcb->seq = tcb->end_seq = tp->write_seq; tcb->flags =TCPCB_FLAG_ACK ;
I fail to see how adding shinfo->in_flight to count how many outstanding clones are there can help accounting for how many "host_inflight" pkts. Part of the problems,
as you've mentioned before, is that the driver may not always get a clone. It may
be getting a copy (e.g., when GSO is on?) hence losing all its connection to the original tp and any chance to have the pkt properly accounted for as host_infligh
by TCP. The skb may also be cloned more than once (e.g., due to tcpdump)...
That said, I also fail to come up with a more bullet-proof solution after studying
much of the TSO/GSO code without requring driver and more skb changes. So I'm currently leaning toward my original fix of checking
if (1 == (atomic_read(&skb_shinfo(skb1)->dataref) & SKB_DATAREF_MASK))
My current prototype scans either sk_send_head or sk_write_queue backwards until the above condition is true. I'm thinking about adding and maintaining a new "tp->host_queue_head" field to avoid most of the scanning. Also it seems much
less costly to add a new field to tcp_sock than to skb/skb_shared_info. If you have
a better idea please let me know.
Jerry
Quoted 91 lines · David Miller
From: "Jerry Chu" <[email protected]>
Date: Wed, 23 Apr 2008 16:29:58 -0700Quoted 6 lines
I've been seeing the same problem here and am trying to fix it. My fix is to not count those pkts still in the host queue as "
prior_in_flight" when feeding the latter totcp_cong_avoid(). This should causetcp_is_cwnd_limited()test to fail when the previousin_flightbuild-up is all due to the large host queue, and stop the cwnd to grow beyond what's really necessary.Does something like the following suit your needs?
include/linux/skbuff.h +1
-0@@ -140,6 +140,7 @@ struct skb_frag_struct {*/ struct skb_shared_info {atomic_t dataref; +atomic_t *in_flight;unsigned short nr_frags;unsigned short gso_size;/* Warning: this field is not always filled in (UFO)! */ include/linux/tcp.h +2
-0@@ -271,6 +271,8 @@ struct tcp_sock {u32 rcv_tstamp;/* timestamp of last received ACK (for keepalives) */ u32 lsndtime;/* timestamp of last sent data packet (for restart window) */ +atomic_t host_inflight;/* packets queued in transmit path */ +/* Data for direct copy to user */ struct {struct sk_buff_head prequeue;net/core/skbuff.c +6
-0@@ -212,6 +212,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,/* make sure we initialize shinfo sequentially */ shinfo =skb_shinfo (skb);atomic_set (&shinfo->dataref,1 ); + shinfo->in_flight =NULL ; shinfo->nr_frags =0 ; shinfo->gso_size =0 ; shinfo->gso_segs =0 ;@@ -403,6 +404,8 @@ static void skb_release_all(struct sk_buff *skb)void __kfree_skb (struct sk_buff *skb) {skb_release_all (skb); +if (skb_shinfo (skb)->in_flight) +atomic_dec (skb_shinfo (skb)->in_flight);kfree_skbmem (skb); }@@ -486,6 +489,8 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)atomic_set (&n->users,1 );atomic_inc (&(skb_shinfo (skb)->dataref)); +if (skb_shinfo (skb)->in_flight) +atomic_inc (skb_shinfo (skb)->in_flight); skb->cloned =1 ;return n;@@ -743,6 +748,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail, skb->hdr_len =0 ; skb->nohdr =0 ;atomic_set (&skb_shinfo (skb)->dataref,1 ); +skb_shinfo (skb)->in_flight =NULL ;return 0 ; nodata:net/ipv4/tcp.c +1
-0@@ -479,6 +479,7 @@ static inline void skb_entail(struct sock *sk, struct sk_buff *skb)struct tcp_sock *tp =tcp_sk (sk);struct tcp_skb_cb *tcb =TCP_SKB_CB (skb); +skb_shinfo (skb)->in_flight = &tp->host_inflight; skb->csum =0 ; tcb->seq = tcb->end_seq = tp->write_seq; tcb->flags =TCPCB_FLAG_ACK ;
David Miller
maintainerFrom: "Jerry Chu" <[email protected]>
Date: Tue, 6 May 2008 20:57:46 -0700
Quoted 4 lines
I fail to see how adding
shinfo->in_flightto count how many outstanding clones are there can help accounting for how many "host_inflight" pkts. Part of the problems, as you've mentioned before, is that the driver may not always get a clone.
Sure but it will get one %99.9999 of the time.
With TCP, as long as that clone is alive the driver has it. And the counter only counts the clones.
Anyways, did you even test my patch and try to use it for your needs or is this analysis purely from your inspection of it? :-/
Quoted 11 lines · David Miller
From: "Jerry Chu" <[email protected]>
Date: Tue, 6 May 2008 20:57:46 -0700Quoted 4 lines
I fail to see how adding
shinfo->in_flightto count how many outstanding clones are there can help accounting for how many "host_inflight" pkts. Part of the problems, as you've mentioned before, is that the driver may not always get a clone.Sure but it will get one %99.9999 of the time.
I couldn't care less about some rare boundary case either. What I'm concerned about is if there is some popular below-IP module/feature that, when turned on, will render my host_inflight check (using dataref==1 as an indication that the pkt has left the host) completely bogus. Turning on pkt tapping (e.g., tcpdump) seems to be one such case, although one may argue that belongs to those edge usage conditions that we don't need to worry about. (OTOH it may be undesirable for tcpdump to have significant performance side effect for those who try to use tcpdump to debug performance problem.) Turning on GSO but not TSO seems to be another case where the driver gets a copy all the time (kernel version 2.6.18).
If there is no other popular below-IP module/feature that will break the dataref==1 then perhaps my initial concern was invalid. In either case, I don't see your patch any better than my dataref==1 check. Neither one address the below-IP module/feature concern described before.
Quoted 7 lines
With TCP, as long as that clone is alive the driver has it. And the counter only counts the clones.
Anyways, did you even test my patch and try to use it for your needs or is this analysis purely from your inspection of it? :-/
No I haven't tested your patch. I tried to understand skb better before applying your patch. After I studied bunch of code, I come to the conclusion that your patch won't work for me. First it tracks # of clones, which is not what I need. E.g., tcpdump will cause host_inflight to be grossly wrong. Ok, maybe we can ignore tcpdump, your patch counts # of cloned skb whereas I need a count of # of pkts. Perhaps this can be fixed also, but it then dawned on me that wouldn't it be more desirable to add the space overhead per tcp_sock than per skb?
Jerry
Quoted 2 lines
David Miller
maintainerFrom: "Jerry Chu" <[email protected]>
Date: Wed, 7 May 2008 11:36:59 -0700
Quoted 4 lines
No I haven't tested your patch. I tried to understand skb better before applying your patch. After I studied bunch of code, I come to the conclusion that your patch won't work for me. First it tracks # of clones, which is not what I need. E.g., tcpdump will cause
host_inflightto be grossly wrong.
We can make sub-clones not count.
Also, we already can distinguish this case, because all SKB clones made by TCP are fast-clones. So we could only bump the counter for fast clones. If tcpdump clones it again, it won't be a fast clone and therefore we can avoid bumping the counter in that case. Similarly for other features that want to clone.
Please try to get your idea working with my infrastructure. We can modify it to behave however you need it to, but at the core it's the idea that tracks the state most directly and properly.
Quoted 9 lines · David Miller
From: "Jerry Chu" <[email protected]>
Date: Wed, 7 May 2008 11:36:59 -0700Quoted 4 lines
No I haven't tested your patch. I tried to understand skb better before applying your patch. After I studied bunch of code, I come to the conclusion that your patch won't work for me. First it tracks # of clones, which is not what I need. E.g., tcpdump will cause
host_inflightto be grossly wrong.
Quoted 7 lines
We can make sub-clones not count.
Also, we already can distinguish this case, because all SKB clones made by TCP are fast-clones. So we could only bump the counter for fast clones. If tcpdump clones it again, it won't be a fast clone and therefore we can avoid bumping the counter in that case. Similarly for other features that want to clone.
Ok, just Google search "skb fast clone" and found some posting from you. Will take a look.
Quoted 5 lines
Please try to get your idea working with my infrastructure. We can modify it to behave however you need it to, but at the core it's the idea that tracks the state most directly and properly.
Ok, will give it a try. First i'll fix your patch to atomic_add()/atomic_sub() by
skb_shinfo(skb)->gso_segs rather than always 1, in order for GSO/TSO to work.
One problem came up to my mind - it seems possible for __kfree_skb() to access skb_shinfo(skb)->in_flight whose tp has been freed up since only the original skb's on TCP's rexmit list have the owner set and socket held. One solution
is for TCP to zap skb_shinfo(skb)->in_flight field when it's ready to free up skb.
I can hack sock_wfree() to do this, but I don't know how to do it right.
Jerry
David Miller
maintainerFrom: "Jerry Chu" <[email protected]>
Date: Wed, 7 May 2008 18:37:01 -0700
Quoted 3 lines
Ok, will give it a try. First i'll fix your patch to atomic_add()/atomic_sub() by
skb_shinfo(skb)->gso_segs rather than always 1, in order for GSO/TSO to work.
That might not work. gso_segs can change over time as retransmit packets get split up due to SACKs etc. it needs to be audited, at the very least.
Quoted 7 lines
One problem came up to my mind - it seems possible for
__kfree_skb()to access skb_shinfo(skb)->in_flight whose tp has been freed up since only the original skb's on TCP's rexmit list have the owner set and socket held. One solution
is for TCP to zap skb_shinfo(skb)->in_flight field when it's ready to free up skb.
I can hacksock_wfree()to do this, but I don't know how to do it right.
There will be references to the socket, so this should be ok.
If it isn't we can adjust the count and zap the pointer in skb_orphan().
There seems to be quite a bit of complexity plus one additional pointer field per skb_shared_info to make skb better track when a pkt leaves the host. Now I wonder if it's really a better solution than my original, simply checking dataref==1 approach which, although not bullet proof, may be "good enough" for all practical purposes?
Jerry
Quoted 26 lines · David Miller
From: "Jerry Chu" <[email protected]>
Date: Wed, 7 May 2008 18:37:01 -0700Quoted 3 lines
Ok, will give it a try. First i'll fix your patch to atomic_add()/atomic_sub() by
skb_shinfo(skb)->gso_segs rather than always 1, in order for GSO/TSO to work.That might not work.
gso_segscan change over time as retransmit packets get split up due to SACKs etc. it needs to be audited, at the very least.Quoted 7 lines
One problem came up to my mind - it seems possible for
__kfree_skb()to access skb_shinfo(skb)->in_flight whose tp has been freed up since only the original skb's on TCP's rexmit list have the owner set and socket held. One solution
is for TCP to zap skb_shinfo(skb)->in_flight field when it's ready to free up skb.
I can hacksock_wfree()to do this, but I don't know how to do it right.There will be references to the socket, so this should be ok.
If it isn't we can adjust the count and zap the pointer in
skb_orphan().
I did a quick prototype based on your idea of adding an "in_flight" field to skb_shared_info to track how many in-flight clones in the host. I tested
it quickly and it doesn't work. After some thought it was obvious why it won't work. It's because what the TCP stack needs is to track how many in-flight pkts are in the host, but your proposed patch increments "in_flight" once on the 1st __skb_clone() to be sent to the driver, but decrements "in_flight" TWICE, one for each of the clones to be freed. I did a quick hack to make it work for my limited test case but I haven't figured out an acceptable (non-hack) solution.
Continued testing, I discovered the problem I described below where "in_flight" may point to a tp that has already been freed can not be addressed by zapping skb_shinfo(skb)->in_flight in sock_wfree(). The reason is that pkts may be acked and freed by TCP before driver freeing up its clone copy (e.g., due to driver lazy reclaim...) When that happens the "host_inflight" accounting will get messed up.
Jerry
Quoted 38 lines · Jerry Chu
There seems to be quite a bit of complexity plus one additional pointer
field perskb_shared_infoto make skb better track when a pkt leaves the host. Now I wonder if it's really a better solution than my original, simply checking dataref==1 approach which, although not bullet proof, may be "good enough" for all practical purposes?Jerry
Quoted 26 lines · David Miller
From: "Jerry Chu" <[email protected]>
Date: Wed, 7 May 2008 18:37:01 -0700Quoted 3 lines
Ok, will give it a try. First i'll fix your patch to atomic_add()/atomic_sub() by
skb_shinfo(skb)->gso_segs rather than always 1, in order for GSO/TSO to work.That might not work.
gso_segscan change over time as retransmit packets get split up due to SACKs etc. it needs to be audited, at the very least.Quoted 7 lines
One problem came up to my mind - it seems possible for
__kfree_skb()to access skb_shinfo(skb)->in_flight whose tp has been freed up since only the original skb's on TCP's rexmit list have the owner set and socket held. One solution
is for TCP to zap skb_shinfo(skb)->in_flight field when it's ready to free up skb.
I can hacksock_wfree()to do this, but I don't know how to do it right.There will be references to the socket, so this should be ok.
If it isn't we can adjust the count and zap the pointer in
skb_orphan().
David Miller
maintainerFrom: "Jerry Chu" <[email protected]>
Date: Mon, 12 May 2008 15:22:55 -0700
Quoted 10 lines
I did a quick prototype based on your idea of adding an "
in_flight" field toskb_shared_infoto track how many in-flight clones in the host. I tested
it quickly and it doesn't work. After some thought it was obvious why it won't work. It's because what the TCP stack needs is to track how many in-flight pkts are in the host, but your proposed patch increments "in_flight" once on the 1st__skb_clone()to be sent to the driver, but decrements "in_flight" TWICE, one for each of the clones to be freed. I did a quick hack to make it work for my limited test case but I haven't figured out an acceptable (non-hack) solution.
That's easy to fix, only set the in_flight pointer in the child clone skb. Thanks for figuring that out.
Quoted 6 lines
Continued testing, I discovered the problem I described below where "
in_flight" may point to a tp that has already been freed can not be addressed by zapping skb_shinfo(skb)->in_flight insock_wfree(). The reason is that pkts may be acked and freed by TCP before driver freeing up its clone copy (e.g., due to driver lazy reclaim...) When that happens the "host_inflight" accounting will get messed up.
Simply notice, when we're about to decrement in_flight, that the data reference is one. You can take appropriate actions if so.
David Miller
maintainerFrom: David Miller <[email protected]>
Date: Mon, 12 May 2008 15:29:00 -0700 (PDT)
Quoted 2 lines
Simply notice, when we're about to decrement
in_flight, that the data reference is one. You can take appropriate actions if so.
Actually, there is an even simpler solution. Since this is a fast clone, when the parent SKB is freed we can see if the fast-clone child is active and has the in_flight atomic_t pointer. If so, we decrement in_flight and zap it to NULL.
Quoted 11 lines · David Miller
From: David Miller <[email protected]>
Date: Mon, 12 May 2008 15:29:00 -0700 (PDT)Quoted 2 lines
Simply notice, when we're about to decrement
in_flight, that the data reference is one. You can take appropriate actions if so.Actually, there is an even simpler solution. Since this is a fast clone, when the parent SKB is freed we can see if the fast-clone child is active and has the
in_flightatomic_tpointer. If so, we decrementin_flightand zap it to NULL.
I thought about that too. But what happens if the clone is being freed by the driver at the same time when TCP is freeing the parent? Isn't there an intrinsic race condition here? If true, can we use some atomic operation to zap in_flight, but also return to the caller if in_flight has been nullified already so that the accounting can be handled correctly? (Although tp->host_inflight doesn't need to be super accurate, if there is no simple solution above I'll have to reset tp->host_inflight periodically and convince myself the pathological case of host_inflight drifting forever won't ever happen...)
Jerry
David Miller
maintainerFrom: "Jerry Chu" <[email protected]>
Date: Mon, 12 May 2008 20:56:16 -0700
Quoted 3 lines
If true, can we use some atomic operation to zap
in_flight, but also return to the caller ifin_flighthas been nullified already so that the accounting can be handled correctly?
xchg() should work for that purpose.
Does xchg() return the old value? (Can't find any comments on it.)
Jerry
Quoted 10 lines · David Miller
From: "Jerry Chu" <[email protected]>
Date: Mon, 12 May 2008 20:56:16 -0700Quoted 3 lines
If true, can we use some atomic operation to zap
in_flight, but also return to the caller ifin_flighthas been nullified already so that the accounting can be handled correctly?
xchg()should work for that purpose.
David Miller
maintainerFrom: "Jerry Chu" <[email protected]>
Date: Mon, 12 May 2008 21:00:08 -0700
Quoted 1 line
Does
xchg()return the old value? (Can't find any comments on it.)
Yes, otherwise it wouldn't be very useful.
Ok, now the code seems to work when TSO is disabled, i.e., cwnd remains low (40-60pkts) and only grows slowly for extended period on a back2back 1GbE link. It also works when tcpdump was running after I fixed the code, per your suggestion, to only bump the in_flight count for fast clones.
But it doesn't work when TSO is enabled. I've fixed tso_fragment() to correctly set skb_shinfo(buff)->in_flight after the split. But cwnd still grows rapidly to a few hundred pkts (although smaller than w/o the fix). The host inflight accounting gets screwed up. It looks like pskb_expand_head() called by tcp_tso_acked()->tcp_trim_head() messes up the accounting but I don't know how to fix it (still trying to understand this complex piece of code). There could be other reason as well.
Jerry
Quoted 7 lines · David Miller
From: "Jerry Chu" <[email protected]>
Date: Mon, 12 May 2008 21:00:08 -0700Quoted 1 line
Does
xchg()return the old value? (Can't find any comments on it.)Yes, otherwise it wouldn't be very useful.
David Miller
maintainerFrom: "Jerry Chu" <[email protected]>
Date: Fri, 16 May 2008 18:13:20 -0700
Quoted 5 lines
The host inflight accounting gets screwed up. It looks like
pskb_expand_head()called by tcp_tso_acked()->tcp_trim_head() messes up the accounting but I don't know how to fix it (still trying to understand this complex piece of code). There could be other reason as well.
This is just like freeing up a normal SKB, so decrementing the in_flight value the appropriate number of packets should do the right thing.
tcp_tso_acked() calculates this adjustment for you, in packets_acked.
Quoted 9 lines · David Miller
From: "Jerry Chu" <[email protected]>
Date: Fri, 16 May 2008 18:13:20 -0700Quoted 5 lines
The host inflight accounting gets screwed up. It looks like
pskb_expand_head()called by tcp_tso_acked()->tcp_trim_head() messes up the accounting but I don't know how to fix it (still trying to understand this complex piece of code). There could be other reason as well.
Quoted 6 lines
This is just like freeing up a normal SKB, so decrementing the
in_flightvalue the appropriate number of packets should do the right thing.
tcp_tso_acked()calculates this adjustment for you, inpackets_acked.
The current pskb_expand_head() code sets skb_shinfo(skb)->in_flight to NULL to leave the in_flight accounting solely to the other clone to handle. To calculate in_flight more accurately, it seems to require splitting the gso_segs and in_flight accounting bewteen the clone and the skb (with new header), right? Plus any race condition to take care... Hard to think clearly in a Friday afternoon :-(
Jerry
Quoted 15 lines · David Miller
From: "Jerry Chu" <[email protected]>
Date: Mon, 12 May 2008 15:22:55 -0700Quoted 10 lines
I did a quick prototype based on your idea of adding an "
in_flight" field toskb_shared_infoto track how many in-flight clones in the host. I tested
it quickly and it doesn't work. After some thought it was obvious why it won't work. It's because what the TCP stack needs is to track how many in-flight pkts are in the host, but your proposed patch increments "in_flight" once on the 1st__skb_clone()to be sent to the driver, but decrements "in_flight" TWICE, one for each of the clones to be freed. I did a quick hack to make it work for my limited test case but I haven't figured out an acceptable (non-hack) solution.
Quoted 2 lines
That's easy to fix, only set the
in_flightpointer in the child clone skb. Thanks for figuring that out.
I must be missing something. All the clones share the same "skb_shared_info", how does one only set in_flight in one clone but not the other?
Quoted 9 lines
Quoted 8 lines
Continued testing, I discovered the problem I described below where "
in_flight" may point to a tp that has already been freed can not be addressed by zapping skb_shinfo(skb)->in_flight insock_wfree(). The reason is that pkts may be acked and freed by TCP before driver freeing up its clone copy (e.g., due to driver lazy reclaim...) When that happens the "host_inflight" accounting will get messed up.
Quoted 3 lines
Simply notice, when we're about to decrement
in_flight, that the data reference is one. You can take appropriate actions if so.
Jerry
David Miller
maintainerFrom: "Jerry Chu" <[email protected]>
Date: Mon, 12 May 2008 15:58:58 -0700
Quoted 3 lines
I must be missing something. All the clones share the same "
skb_shared_info", how does one only setin_flightin one clone but not the other?
These are fast clones, we "know" which is the parent and which is the child.
David Miller
maintainerFrom: "Jerry Chu" <[email protected]>
Date: Tue, 6 May 2008 20:57:46 -0700
Quoted 4 lines
It may be getting a copy (e.g., when GSO is on?) hence losing all its connection to the original tp and any chance to have the pkt properly accounted for as
host_inflighby TCP. The skb may also be cloned more than once (e.g., due to tcpdump)...
It only gets a copy if the SKB is already cloned and that clone is alive somewhere (f.e. stuck in the device, a rare occurance by the time we retransmit).
It is a clone %99.999999 of the time.
Quoted 5 lines · David Miller
From: "Jerry Chu" <[email protected]>
Date: Tue, 6 May 2008 20:57:46 -0700
Quoted 8 lines
Quoted 4 lines
It may be getting a copy (e.g., when GSO is on?) hence losing all its connection to the original tp and any chance to have the pkt properly accounted for as
host_inflighby TCP. The skb may also be cloned more than once (e.g., due to tcpdump)...It only gets a copy if the SKB is already cloned and that clone is alive somewhere (f.e. stuck in the device, a rare occurance by the time we retransmit).
This is one of many things about skb that I still don't completely understand. Why in tcp_transmit_skb() we'll have to pskb_copy() if skb_cloned()? Can't we clone a skb mulitple times? Is it due to some special optimization from skb->fclone stuff... that imposes this restriction?
Quoted 2 lines
It is a clone %99.999999 of the time.
When I turned on GSO but not TSO, I believe it's a copy %99.999999 of the time. (I must confess I don't understand GSO code yet. I temporarily ran out of stream after studying bunch of other code.)
Jerry
Quoted 1 line
David Miller
maintainerFrom: "Jerry Chu" <[email protected]>
Date: Wed, 7 May 2008 11:54:01 -0700
Quoted 2 lines
This is one of many things about skb that I still don't completely understand. Why in
tcp_transmit_skb()we'll have topskb_copy()ifskb_cloned()?
The other clone holder owns the packet header area. All packets on the retransmit queue of TCP are headerless. The call sites down into the device add the headers.
Therefore we cannot have two paths modifying the headers at the same time.
Quoted 2 lines
Can't we clone a skb mulitple times? Is it due to some special optimization from
skb->fclonestuff... that imposes this restriction?
No, it has nothing to do with fclone. It has to do with what instance owns the packet header area in front of the user's TCP data.
Quoted 7 lines · David Miller
From: "Jerry Chu" <[email protected]>
Date: Wed, 7 May 2008 11:54:01 -0700Quoted 2 lines
This is one of many things about skb that I still don't completely understand. Why in
tcp_transmit_skb()we'll have topskb_copy()ifskb_cloned()?
Quoted 6 lines
The other clone holder owns the packet header area. All packets on the retransmit queue of TCP are headerless. The call sites down into the device add the headers.
Therefore we cannot have two paths modifying the headers at the same time.
Oh I see. I think I confused pskb_copy() with skb_copy() and kept thinking why do we need to copy the whole thing...
Quoted 7 lines
Quoted 4 lines
Can't we clone a skb mulitple times? Is it due to some special optimization from
skb->fclonestuff... that imposes this restriction?No, it has nothing to do with fclone. It has to do with what instance owns the packet header area in front of the user's TCP data.
Thanks for explaining.
Jerry