LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
To: David Miller <davem@davemloft.net>
Cc: Guillaume Chazarain <guichaz@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Ray Lee <ray-lk@madrabbit.org>, Chris Wedgwood <cw@f00f.org>,
	Giangiacomo Mariotti <giangiacomo_mariotti@yahoo.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Netdev <netdev@vger.kernel.org>
Subject: [PATCH net-2.6] [TCP]: Must count fack_count also when skipping
Date: Mon, 3 Mar 2008 15:53:12 +0200 (EET)	[thread overview]
Message-ID: <Pine.LNX.4.64.0803031541230.28251@kivilampi-30.cs.helsinki.fi> (raw)
In-Reply-To: <Pine.LNX.4.64.0803031525390.27064@kivilampi-30.cs.helsinki.fi>

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4225 bytes --]

On Mon, 3 Mar 2008, Ilpo Järvinen wrote:

> On Mon, 3 Mar 2008, Ilpo Järvinen wrote:
> 
> > On Sun, 2 Mar 2008, Ilpo Järvinen wrote:
> > 
> > > On Sun, 2 Mar 2008, Guillaume Chazarain wrote:
> > > 
> > > >> On Sun, Mar 2, 2008 at 1:38 PM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote:
> > > >>> It does not contain any "KERNEL: assertion (packets <=
> > > >> > tp->packets_out) failed at" line, so I'm afraid it's just noise.
> > > 
> > > Doh, you were right in this one...
> 
> I did some filtering among those and found out that some still point out 
> bug (there might be indication of tcp_mark_head_lost inconsistency as 
> well but it's nearly impossible to track with all the incorporated noise
> which is causing which). ...I'll post the patch separately right after 
> this.

Dave, at least this is needed for TCP packet counter correctness, there 
could be other one hiding still because this only makes fackets_out too 
small (there could be second order effect though), while people are seeing 
a result of too large fackets_out in tcp_mark_head_lost.

It's a bit shame that I didn't notice this when I verified time-seqno 
graphs after that sacktag rewrite (it mostly results just a substle 
difference in the slope).

With this & the debug patch I didn't get anything into my logs but others 
might be more successful if there are other bugs still to solve that 
require more sophisticated network conditions to occur.


-- 
 i.

[PATCH net-2.6] [TCP]: Must count fack_count also when skipping

It makes fackets_out to grow too slowly compared with the
real write queue.

This shouldn't cause those BUG_TRAP(packets <= tp->packets_out)
to trigger but how knows how such inconsistent fackets_out
affects here and there around TCP when everything is nowadays
assuming accurate fackets_out. So lets see if this silences
them all.

Reported by Guillaume Chazarain <guichaz@gmail.com>.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
 net/ipv4/tcp_input.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 19c449f..7facdb0 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1367,7 +1367,7 @@ static struct sk_buff *tcp_sacktag_walk(struct sk_buff *skb, struct sock *sk,
  * a normal way
  */
 static struct sk_buff *tcp_sacktag_skip(struct sk_buff *skb, struct sock *sk,
-					u32 skip_to_seq)
+					u32 skip_to_seq, int *fack_count)
 {
 	tcp_for_write_queue_from(skb, sk) {
 		if (skb == tcp_send_head(sk))
@@ -1375,6 +1375,8 @@ static struct sk_buff *tcp_sacktag_skip(struct sk_buff *skb, struct sock *sk,
 
 		if (!before(TCP_SKB_CB(skb)->end_seq, skip_to_seq))
 			break;
+
+		*fack_count += tcp_skb_pcount(skb);
 	}
 	return skb;
 }
@@ -1390,7 +1392,7 @@ static struct sk_buff *tcp_maybe_skipping_dsack(struct sk_buff *skb,
 		return skb;
 
 	if (before(next_dup->start_seq, skip_to_seq)) {
-		skb = tcp_sacktag_skip(skb, sk, next_dup->start_seq);
+		skb = tcp_sacktag_skip(skb, sk, next_dup->start_seq, fack_count);
 		tcp_sacktag_walk(skb, sk, NULL,
 				 next_dup->start_seq, next_dup->end_seq,
 				 1, fack_count, reord, flag);
@@ -1537,7 +1539,8 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb,
 
 			/* Head todo? */
 			if (before(start_seq, cache->start_seq)) {
-				skb = tcp_sacktag_skip(skb, sk, start_seq);
+				skb = tcp_sacktag_skip(skb, sk, start_seq,
+						       &fack_count);
 				skb = tcp_sacktag_walk(skb, sk, next_dup,
 						       start_seq,
 						       cache->start_seq,
@@ -1565,7 +1568,8 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb,
 				goto walk;
 			}
 
-			skb = tcp_sacktag_skip(skb, sk, cache->end_seq);
+			skb = tcp_sacktag_skip(skb, sk, cache->end_seq,
+					       &fack_count);
 			/* Check overlap against next cached too (past this one already) */
 			cache++;
 			continue;
@@ -1577,7 +1581,7 @@ tcp_sacktag_write_queue(struct sock *sk, struct sk_buff *ack_skb,
 				break;
 			fack_count = tp->fackets_out;
 		}
-		skb = tcp_sacktag_skip(skb, sk, start_seq);
+		skb = tcp_sacktag_skip(skb, sk, start_seq, &fack_count);
 
 walk:
 		skb = tcp_sacktag_walk(skb, sk, next_dup, start_seq, end_seq,
-- 
1.5.2.2

  reply	other threads:[~2008-03-03 13:53 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-21  4:04 WARNING: at net/ipv4/tcp_input.c:2054 tcp_mark_head_lost() Giangiacomo Mariotti
2008-02-23  8:03 ` Andrew Morton
2008-02-27  9:29   ` Ilpo Järvinen
2008-02-27 18:56     ` Guillaume Chazarain
2008-02-27 19:29       ` Ilpo Järvinen
2008-02-28  8:22         ` Ilpo Järvinen
2008-02-29  1:10           ` Andrew Morton
2008-02-29  4:35             ` Bill Fink
2008-02-29 12:24               ` Ilpo Järvinen
2008-03-02 12:20           ` Guillaume Chazarain
2008-03-02 12:38             ` Ilpo Järvinen
2008-03-02 13:15               ` Guillaume Chazarain
2008-03-02 19:51                 ` Ilpo Järvinen
2008-03-03  8:32                   ` Ilpo Järvinen
2008-03-03 13:40                     ` Ilpo Järvinen
2008-03-03 13:53                       ` Ilpo Järvinen [this message]
2008-03-03 20:11                         ` [PATCH net-2.6] [TCP]: Must count fack_count also when skipping David Miller
2008-03-24 20:36                         ` Frederik Himpe
2008-03-24 21:41                           ` David Miller
2008-03-25 21:24                             ` Ilpo Järvinen
2008-03-26  3:05                               ` David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Pine.LNX.4.64.0803031541230.28251@kivilampi-30.cs.helsinki.fi \
    --to=ilpo.jarvinen@helsinki.fi \
    --cc=akpm@linux-foundation.org \
    --cc=cw@f00f.org \
    --cc=davem@davemloft.net \
    --cc=giangiacomo_mariotti@yahoo.com \
    --cc=guichaz@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=ray-lk@madrabbit.org \
    --subject='Re: [PATCH net-2.6] [TCP]: Must count fack_count also when skipping' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).