Re: Suspicious fackets_out handling
From: Ilpo_Järvinen [EMAIL PROTECTED] Date: Fri, 1 Jun 2007 15:17:00 +0300 (EEST) On Thu, 31 May 2007, David Miller wrote: Is it possible to update fastpath_cnt_hint properly perhaps? I think that would be valid and even accurate as it can checks skb's seq against fastpath_skb_hint-seq. I'm in a hurry and will be a week out of reach of internet connectivity but here's quick attempt to deal with this cleanly. Compile tested (be extra careful with this one, it's done i a hurry :-)), consider to net-2.6. Considering the marginality of this issue, stable might really be an overkill for this one, only a remotely valid concern comes to my mind in this case: possibility of fackets_out packet_out might not be dealt that cleanly everywhere (but I'm sure that you can come to a good decicion about it): [PATCH] [TCP]: SACK fastpath did override adjusted fackets_out Do same adjustment to SACK fastpath counters provided that they're valid. Signed-off-by: Ilpo Järvinen [EMAIL PROTECTED] Since the case is marginal and the patch does need some testing I'm putting this into net-2.6.23 for now. - 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: Suspicious fackets_out handling
On Thu, 31 May 2007, David Miller wrote:
From: Ilpo_Järvinen [EMAIL PROTECTED]
Date: Mon, 23 Apr 2007 14:28:21 +0300 (EEST)
There are IMHO two problems in it. First of all, nothing ensures that the
skb TCP is fragmenting is actually below the forwardmost sack block (and
thus is included to the fackets_out)...
...this is so old thing already that I had to refresh my memory,
haven't been expecting any answer for ages... :-)
Good catch, I agree with your analysis completely.
...I later on understood (from a comment I found elsewhere) that
fackets_out is occasionally estimated, rather than exact value (which
could be completely fixed in tcp-2.6 due to presence of highest_sack but
I'm also considering complete removal of it too, like you probably
noticed).
What I'm not sure of though, is how to fix this in net-2.6(.22), it
is due to the fact that there is no pointer/seq which can be used in
testing for it like in tcp-2.6 which has the highest_sack.
We can add highest_sack to 2.6.22 in order to fix a bug like this,
if necessary.
...I think we can leave it as estimate for now, it has been like that
for years and nobody has complained... :-) This problem covers really
marginal number of cases anyway I think (isn't the diff usually
negative: old - new after fragment should be negative unless mss_now
bites).
After I found the comments and analyzed some sacked_out code with NewReno,
I think that usually these estimates are indicated in the code with
tcp_dec_pcount_approx(...) but it wasn't used in this specific case
because it inputs skb instead of int, I'll add clarification of this into
my tcp-2.6 todo list...
Second problem is even more obvious: if adjustment here is being
done and the sacktag code then uses fastpath at the arrival of the
next ACK, the sacktag code will use a stale value from
fastpath_cnt_hint and fails to notice all that math TCP did here
unless we start clearing fastpath_skb_hint.
Let's try not to clear fastpath_skb_hint here if possible :-)
Np, however, whatever we do, it wouldn't really execute that
often... :-)
Is it possible to update fastpath_cnt_hint properly perhaps?
I think that would be valid and even accurate as it can checks skb's
seq against fastpath_skb_hint-seq. I'm in a hurry and will be a week
out of reach of internet connectivity but here's quick attempt to deal
with this cleanly. Compile tested (be extra careful with this one, it's
done i a hurry :-)), consider to net-2.6. Considering the marginality of
this issue, stable might really be an overkill for this one, only a
remotely valid concern comes to my mind in this case: possibility
of fackets_out packet_out might not be dealt that cleanly everywhere
(but I'm sure that you can come to a good decicion about it):
[PATCH] [TCP]: SACK fastpath did override adjusted fackets_out
Do same adjustment to SACK fastpath counters provided that
they're valid.
Signed-off-by: Ilpo Järvinen [EMAIL PROTECTED]
---
net/ipv4/tcp_output.c |9 +
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 53232dd..f24dd36 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -699,6 +699,15 @@ int tcp_fragment(struct sock *sk, struct sk_buff *skb, u32
len, unsigned int mss
tp-fackets_out -= diff;
if ((int)tp-fackets_out 0)
tp-fackets_out = 0;
+ /* SACK fastpath might overwrite it unless dealt with */
+ if (tp-fastpath_skb_hint != NULL
+ after(TCP_SKB_CB(tp-fastpath_skb_hint)-seq,
+ TCP_SKB_CB(skb)-seq)) {
+ tp-fastpath_cnt_hint -= diff;
+ if ((int)tp-fastpath_cnt_hint 0)
+ tp-fastpath_cnt_hint = 0;
+
+ }
}
}
--
1.5.0.6
Re: Suspicious fackets_out handling
From: Ilpo_Järvinen [EMAIL PROTECTED] Date: Mon, 23 Apr 2007 14:28:21 +0300 (EEST) There are IMHO two problems in it. First of all, nothing ensures that the skb TCP is fragmenting is actually below the forwardmost sack block (and thus is included to the fackets_out)... Good catch, I agree with your analysis completely. What I'm not sure of though, is how to fix this in net-2.6(.22), it is due to the fact that there is no pointer/seq which can be used in testing for it like in tcp-2.6 which has the highest_sack. We can add highest_sack to 2.6.22 in order to fix a bug like this, if necessary. Second problem is even more obvious: if adjustment here is being done and the sacktag code then uses fastpath at the arrival of the next ACK, the sacktag code will use a stale value from fastpath_cnt_hint and fails to notice all that math TCP did here unless we start clearing fastpath_skb_hint. Let's try not to clear fastpath_skb_hint here if possible :-) Is it possible to update fastpath_cnt_hint properly perhaps? - 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
Suspicious fackets_out handling
Hi,
While looking through the users of fackets_out, i found this from
tcp_fragment(...):
/* If this packet has been sent out already, we must
* adjust the various packet counters.
*/
if (!before(tp-snd_nxt, TCP_SKB_CB(buff)-end_seq)) {
int diff = old_factor - tcp_skb_pcount(skb) -
tcp_skb_pcount(buff);
[...snip...]
if (diff 0) {
/* Adjust Reno SACK estimate. */
if (!tp-rx_opt.sack_ok) {
tp-sacked_out -= diff;
if ((int)tp-sacked_out 0)
tp-sacked_out = 0;
tcp_sync_left_out(tp);
}
tp-fackets_out -= diff;
if ((int)tp-fackets_out 0)
tp-fackets_out = 0;
}
}
[...]
There are IMHO two problems in it. First of all, nothing ensures that the
skb TCP is fragmenting is actually below the forwardmost sack block (and
thus is included to the fackets_out)... What I'm not sure of though, is
how to fix this in net-2.6(.22), it is due to the fact that there is no
pointer/seq which can be used in testing for it like in tcp-2.6 which has
the highest_sack. Second problem is even more obvious: if adjustment here
is being done and the sacktag code then uses fastpath at the arrival of
the next ACK, the sacktag code will use a stale value from
fastpath_cnt_hint and fails to notice all that math TCP did here unless we
start clearing fastpath_skb_hint.
--
i.
-
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
