[PATCH] tcp: ECN blackhole should not force quickack mode

[PATCH] tcp: ECN blackhole should not force quickack mode

[Eric Dumazet]

While playing with a new ADSL box at home, I discovered that ECN
blackhole can trigger suboptimal quickack mode on linux : We send one
ACK for each incoming data frame, without any delay and eventual
piggyback.

This is because TCP_ECN_check_ce() considers that if no ECT is seen on a
segment, this is because this segment was a retransmit.

Refine this heuristic and apply it only if we seen ECT in a previous
segment, to detect ECN blackhole at IP level.

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Jamal Hadi Salim <jhs@mojatatu.com>
CC: Jerry Chu <hkchu@google.com>
CC: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
CC: Jim Gettys <jg@freedesktop.org>
CC: Dave Taht <dave.taht@gmail.com>
---
Another possibility is to remove this (not in RFC 3168) heuristic, what
do you think ?

 include/net/tcp.h    |    1 +
 net/ipv4/tcp_input.c |   23 ++++++++++++++++-------
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index f357bef..702aefc 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -356,6 +356,7 @@ static inline void tcp_dec_quickack_mode(struct sock *sk,
 #define	TCP_ECN_OK		1
 #define	TCP_ECN_QUEUE_CWR	2
 #define	TCP_ECN_DEMAND_CWR	4
+#define	TCP_ECN_SEEN		8
 
 static __inline__ void
 TCP_ECN_create_request(struct request_sock *req, struct tcphdr *th)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index a5d01b1..5a4408c 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -217,16 +217,25 @@ static inline void TCP_ECN_withdraw_cwr(struct tcp_sock *tp)
 	tp->ecn_flags &= ~TCP_ECN_DEMAND_CWR;
 }
 
-static inline void TCP_ECN_check_ce(struct tcp_sock *tp, struct sk_buff *skb)
+static inline void TCP_ECN_check_ce(struct tcp_sock *tp, const struct sk_buff *skb)
 {
-	if (tp->ecn_flags & TCP_ECN_OK) {
-		if (INET_ECN_is_ce(TCP_SKB_CB(skb)->flags))
-			tp->ecn_flags |= TCP_ECN_DEMAND_CWR;
+	if (!(tp->ecn_flags & TCP_ECN_OK))
+		return;
+
+	switch (TCP_SKB_CB(skb)->flags & INET_ECN_MASK) {
+	case INET_ECN_NOT_ECT:
 		/* Funny extension: if ECT is not set on a segment,
-		 * it is surely retransmit. It is not in ECN RFC,
-		 * but Linux follows this rule. */
-		else if (INET_ECN_is_not_ect((TCP_SKB_CB(skb)->flags)))
+		 * and we already seen ECT on a previous segment,
+		 * it is probably a retransmit.
+		 */
+		if (tp->ecn_flags & TCP_ECN_SEEN)
 			tcp_enter_quickack_mode((struct sock *)tp);
+		break;
+	case INET_ECN_CE:
+		tp->ecn_flags |= TCP_ECN_DEMAND_CWR;
+		/* fallinto */
+	default:
+		tp->ecn_flags |= TCP_ECN_SEEN;
 	}
 }
 

Re: [PATCH] tcp: ECN blackhole should not force quickack mode

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 23 Sep 2011 08:02:19 +0200

> While playing with a new ADSL box at home, I discovered that ECN
> blackhole can trigger suboptimal quickack mode on linux : We send one
> ACK for each incoming data frame, without any delay and eventual
> piggyback.
> 
> This is because TCP_ECN_check_ce() considers that if no ECT is seen on a
> segment, this is because this segment was a retransmit.
> 
> Refine this heuristic and apply it only if we seen ECT in a previous
> segment, to detect ECN blackhole at IP level.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> CC: Jamal Hadi Salim <jhs@mojatatu.com>
> CC: Jerry Chu <hkchu@google.com>
> CC: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> CC: Jim Gettys <jg@freedesktop.org>
> CC: Dave Taht <dave.taht@gmail.com>
> ---
> Another possibility is to remove this (not in RFC 3168) heuristic, what
> do you think ?

Jamal, please give this a look over.  Thanks!

Re: [PATCH] tcp: ECN blackhole should not force quickack mode

[Ilpo Järvinen]

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

On Fri, 23 Sep 2011, Eric Dumazet wrote:

> While playing with a new ADSL box at home, I discovered that ECN
> blackhole can trigger suboptimal quickack mode on linux : We send one
> ACK for each incoming data frame, without any delay and eventual
> piggyback.
> 
> This is because TCP_ECN_check_ce() considers that if no ECT is seen on a
> segment, this is because this segment was a retransmit.
> 
> Refine this heuristic and apply it only if we seen ECT in a previous
> segment, to detect ECN blackhole at IP level.

This seems sensible thing to do.

Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>


-- 
 i.

Re: [PATCH] tcp: ECN blackhole should not force quickack mode

[Jamal Hadi Salim]

On Fri, 2011-09-23 at 08:02 +0200, Eric Dumazet wrote:
> While playing with a new ADSL box at home, I discovered that ECN
> blackhole can trigger suboptimal quickack mode on linux : We send one
> ACK for each incoming data frame, without any delay and eventual
> piggyback.
>
> This is because TCP_ECN_check_ce() considers that if no ECT is seen on a
> segment, this is because this segment was a retransmit.

If you have Linux as the sender, that assumption is legit (i.e we never
set ECT on retransmits).

> Refine this heuristic and apply it only if we seen ECT in a previous
> segment, to detect ECN blackhole at IP level.

So  small confusion for me, Eric:
Is theres some middlebox clearing the ECT but allowing the TCP header
ECN flags to be set during SYN echange?  If yes, why would not allowing
quickack help?

cheers,
jamal

Re: [PATCH] tcp: ECN blackhole should not force quickack mode

[Eric Dumazet]

Le vendredi 23 septembre 2011 à 20:55 -0400, Jamal Hadi Salim a écrit :

> So  small confusion for me, Eric:
> Is theres some middlebox clearing the ECT but allowing the TCP header
> ECN flags to be set during SYN echange?  If yes, why would not allowing
> quickack help?

As you may know, the SYN and SYNACK packets dont have ECT bits, as
mandated by RFC. So there is no way to detect a TOS blackhole in TCP
session init.

On following trace taken from my laptop connecting to a ssh server
(192.168.2.1), you can see that my laptop sends ECT messages, but never
receive ECT bits. Each sides believe the session is ECN enabled, as the
SYN and SYNACK messages have the appropriate ECN markers.

So we have somewhere a box that mask the TOS bits. This is a well known
effect on some networks, unfortunately.

The 'linux funny extension' is wrong in this case, since I have no
retransmits, no losses, yet my laptop is in quickack mode (it sends an
ACK for every incoming data packet), because you assumed receiving a
packet with no ECT bits means this packet was a retransmit.



21:14:44.665405 IP (tos 0x0, ttl 64, id 63689, offset 0, flags [DF], proto TCP (6), length 60)
    192.168.2.32.39529 > 192.168.2.1.22: Flags [SEW], cksum 0xf2ea (incorrect -> 0x2151), seq 1589453915, win 14600, options [mss 1460,sackOK,TS val 56466 ecr 0,nop,wscale 7], length 0

21:14:44.773643 IP (tos 0x0, ttl 46, id 0, offset 0, flags [DF], proto TCP (6), length 60)
    192.168.2.1.22 > 192.168.2.32.39529: Flags [S.E], cksum 0xbce1 (correct), seq 1422101564, ack 1589453916, win 14480, options [mss 1452,sackOK,TS val 176781781 ecr 56466,nop,wscale 7], length 0

21:14:44.773691 IP (tos 0x0, ttl 64, id 63690, offset 0, flags [DF], proto TCP (6), length 52)
    192.168.2.32.39529 > 192.168.2.1.22: Flags [.], cksum 0xf2e2 (incorrect -> 0x23f8), ack 1, win 115, options [nop,nop,TS val 56477 ecr 176781781], length 0

21:14:44.888804 IP (tos 0x0, ttl 46, id 62941, offset 0, flags [DF], proto TCP (6), length 91)
    192.168.2.1.22 > 192.168.2.32.39529: Flags [P.], cksum 0x64d1 (correct), seq 1:40, ack 1, win 114, options [nop,nop,TS val 176781792 ecr 56477], length 39

21:14:44.888842 IP (tos 0x0, ttl 64, id 63691, offset 0, flags [DF], proto TCP (6), length 52)
    192.168.2.32.39529 > 192.168.2.1.22: Flags [.], cksum 0xf2e2 (incorrect -> 0x23ba), ack 40, win 115, options [nop,nop,TS val 56489 ecr 176781792], length 0

21:14:44.888929 IP (tos 0x2,ECT(0), ttl 64, id 63692, offset 0, flags [DF], proto TCP (6), length 91)
    192.168.2.32.39529 > 192.168.2.1.22: Flags [P.], cksum 0x649d (correct), seq 1:40, ack 40, win 115, options [nop,nop,TS val 56489 ecr 176781792], length 39

21:14:44.993714 IP (tos 0x0, ttl 46, id 62942, offset 0, flags [DF], proto TCP (6), length 52)
    192.168.2.1.22 > 192.168.2.32.39529: Flags [.], cksum 0x2389 (correct), ack 40, win 114, options [nop,nop,TS val 176781803 ecr 56489], length 0

21:14:44.993758 IP (tos 0x2,ECT(0), ttl 64, id 63693, offset 0, flags [DF], proto TCP (6), length 1196)
    192.168.2.32.39529 > 192.168.2.1.22: Flags [P.], cksum 0x1e1b (correct), seq 40:1184, ack 40, win 115, options [nop,nop,TS val 56499 ecr 176781803], length 1144

21:14:44.994447 IP (tos 0x0, ttl 46, id 62943, offset 0, flags [DF], proto TCP (6), length 900)
    192.168.2.1.22 > 192.168.2.32.39529: Flags [P.], cksum 0xd3d4 (correct), seq 40:888, ack 40, win 114, options [nop,nop,TS val 176781803 ecr 56489], length 848

21:14:44.994463 IP (tos 0x0, ttl 64, id 63694, offset 0, flags [DF], proto TCP (6), length 52)
    192.168.2.32.39529 > 192.168.2.1.22: Flags [.], cksum 0xf2e2 (incorrect -> 0x1ba9), ack 888, win 128, options [nop,nop,TS val 56499 ecr 176781803], length 0

21:14:45.546332 IP (tos 0x0, ttl 64, id 63695, offset 0, flags [DF], proto TCP (6), length 1196)
    192.168.2.32.39529 > 192.168.2.1.22: Flags [P.], cksum 0x1a8c (correct), seq 40:1184, ack 888, win 128, options [nop,nop,TS val 56549 ecr 176781803], length 1144

Re: [PATCH] tcp: ECN blackhole should not force quickack mode

[jamal]

On Sun, 2011-09-25 at 21:24 +0200, Eric Dumazet wrote:

> 
> So we have somewhere a box that mask the TOS bits. This is a well known
> effect on some networks, unfortunately.

ok

> The 'linux funny extension' is wrong in this case, since I have no
> retransmits, no losses, yet my laptop is in quickack mode (it sends an
> ACK for every incoming data packet), because you assumed receiving a
> packet with no ECT bits means this packet was a retransmit.

No objection from me - only one corner case i can think of is if
expected behavior happens and the first data packet was a retransmit
(i.e you wouldnt have seen an ECT which you need for your heuristic),
but that is probably not a big a deal. 

In regards to the 'linux funny extension' - this was a brilliant idea
in my opinion back then from Alexey; lots of discussions happened but
I cant remember if it made it in some RFC or not (I will try to search
some old archives).

cheers,
jamal

Re: [PATCH] tcp: ECN blackhole should not force quickack mode

[jamal]

On Sun, 2011-09-25 at 21:07 -0400, jamal wrote:

> In regards to the 'linux funny extension' - this was a brilliant idea
> in my opinion back then from Alexey; lots of discussions happened but
> I cant remember if it made it in some RFC or not (I will try to search
> some old archives).

Wasnt hard:
https://tools.ietf.org/id/draft-ietf-tsvwg-tcp-ecn-00.txt
It doesnt seem to have made it to RFC and i cant remember why.
[We dont wanna bring Sally out of retirement but we can ask KK if
it bugs you;->]

cheers,
jamal

Re: [PATCH] tcp: ECN blackhole should not force quickack mode

[Eric Dumazet]

Le dimanche 25 septembre 2011 à 21:13 -0400, jamal a écrit :
> On Sun, 2011-09-25 at 21:07 -0400, jamal wrote:
> 
> > In regards to the 'linux funny extension' - this was a brilliant idea
> > in my opinion back then from Alexey; lots of discussions happened but
> > I cant remember if it made it in some RFC or not (I will try to search
> > some old archives).
> 
> Wasnt hard:
> https://tools.ietf.org/id/draft-ietf-tsvwg-tcp-ecn-00.txt
> It doesnt seem to have made it to RFC and i cant remember why.
> [We dont wanna bring Sally out of retirement but we can ask KK if
> it bugs you;->]

Thanks !

This refers to additions to RFC 2481 : This was refined by RFC 3168, and
the retransmitted TCP packets requirement is part of the final RFC :

6.1.5.  Retransmitted TCP packets

   This document specifies ECN-capable TCP implementations MUST NOT set
   either ECT codepoint (ECT(0) or ECT(1)) in the IP header for
   retransmitted data packets, ...

Followed by a very long description :)

BTW, the ECN+ proposal (RFC 5562 :  Adding Explicit Congestion
Notification (ECN) Capability to TCP's SYN/ACK Packets) would allow the
client (receiving SYNACK message with ECT flags) to set the TCP_ECN_SEEN
I added in my patch, allowing even the first (retransmitted) data packet
to trigger quickack mode.

Re: [PATCH] tcp: ECN blackhole should not force quickack mode

[jamal]

On Mon, 2011-09-26 at 10:26 +0200, Eric Dumazet wrote:

> This refers to additions to RFC 2481 : This was refined by RFC 3168, and
> the retransmitted TCP packets requirement is part of the final RFC :

Ah, yes - thats where it went.

> 
> BTW, the ECN+ proposal (RFC 5562 :  Adding Explicit Congestion
> Notification (ECN) Capability to TCP's SYN/ACK Packets) would allow the
> client (receiving SYNACK message with ECT flags) to set the TCP_ECN_SEEN
> I added in my patch, allowing even the first (retransmitted) data packet
> to trigger quickack mode.

Excellent ;->
Are you going to add 5562 support?

cheers,
jamal

Re: [PATCH] tcp: ECN blackhole should not force quickack mode

[Eric Dumazet]

Le lundi 26 septembre 2011 à 08:00 -0400, jamal a écrit :
> On Mon, 2011-09-26 at 10:26 +0200, Eric Dumazet wrote:
> 
> > This refers to additions to RFC 2481 : This was refined by RFC 3168, and
> > the retransmitted TCP packets requirement is part of the final RFC :
> 
> Ah, yes - thats where it went.
> 
> > 
> > BTW, the ECN+ proposal (RFC 5562 :  Adding Explicit Congestion
> > Notification (ECN) Capability to TCP's SYN/ACK Packets) would allow the
> > client (receiving SYNACK message with ECT flags) to set the TCP_ECN_SEEN
> > I added in my patch, allowing even the first (retransmitted) data packet
> > to trigger quickack mode.
> 
> Excellent ;->
> Are you going to add 5562 support?

I sent a private mail to Adam Langley this morning asking him if he
planned an official submission of a prior patch :  
http://www.ietf.org/mail-archive/web/tcpm/current/msg03988.html

If Adam is too busy or not anymore interested, I would like to spend
some cycles on this.

Thanks

Re: [PATCH] tcp: ECN blackhole should not force quickack mode

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 23 Sep 2011 08:02:19 +0200

> While playing with a new ADSL box at home, I discovered that ECN
> blackhole can trigger suboptimal quickack mode on linux : We send one
> ACK for each incoming data frame, without any delay and eventual
> piggyback.
> 
> This is because TCP_ECN_check_ce() considers that if no ECT is seen on a
> segment, this is because this segment was a retransmit.
> 
> Refine this heuristic and apply it only if we seen ECT in a previous
> segment, to detect ECN blackhole at IP level.
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied to net-next, thanks Eric.

[PATCH net-next] tcp: unalias tcp_skb_cb flags and ip_dsfield

[Eric Dumazet]

struct tcp_skb_cb contains a "flags" field containing either tcp flags
or IP dsfield depending on context (input or output path)

Introduce ip_dsfield to make the difference clear and ease maintenance.
If later we want to save space, we can union flags/ip_dsfield

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---

If there is no objection, I plan to rename "flags" to "tcp_flags" in a
following patch.

 include/net/tcp.h    |    3 ++-
 net/ipv4/tcp_input.c |    2 +-
 net/ipv4/tcp_ipv4.c  |    2 +-
 net/ipv6/tcp_ipv6.c  |    2 +-
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 702aefc..28a9997 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -642,7 +642,8 @@ struct tcp_skb_cb {
 #define TCPCB_SACKED_RETRANS	0x02	/* SKB retransmitted		*/
 #define TCPCB_LOST		0x04	/* SKB is lost			*/
 #define TCPCB_TAGBITS		0x07	/* All tag bits			*/
-
+	__u8		ip_dsfield;	/* IPv4 tos or IPv6 dsfield	*/
+	/* 1 byte hole */
 #define TCPCB_EVER_RETRANS	0x80	/* Ever retransmitted frame	*/
 #define TCPCB_RETRANS		(TCPCB_SACKED_RETRANS|TCPCB_EVER_RETRANS)
 
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 5a4408c..7008fcc 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -222,7 +222,7 @@ static inline void TCP_ECN_check_ce(struct tcp_sock *tp, const struct sk_buff *s
 	if (!(tp->ecn_flags & TCP_ECN_OK))
 		return;
 
-	switch (TCP_SKB_CB(skb)->flags & INET_ECN_MASK) {
+	switch (TCP_SKB_CB(skb)->ip_dsfield & INET_ECN_MASK) {
 	case INET_ECN_NOT_ECT:
 		/* Funny extension: if ECT is not set on a segment,
 		 * and we already seen ECT on a previous segment,
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index c29912c..dd3fad9 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1677,7 +1677,7 @@ int tcp_v4_rcv(struct sk_buff *skb)
 				    skb->len - th->doff * 4);
 	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
 	TCP_SKB_CB(skb)->when	 = 0;
-	TCP_SKB_CB(skb)->flags	 = iph->tos;
+	TCP_SKB_CB(skb)->ip_dsfield = ipv4_get_dsfield(iph);
 	TCP_SKB_CB(skb)->sacked	 = 0;
 
 	sk = __inet_lookup_skb(&tcp_hashinfo, skb, th->source, th->dest);
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 12bdb9a..00797d8 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1717,7 +1717,7 @@ static int tcp_v6_rcv(struct sk_buff *skb)
 				    skb->len - th->doff*4);
 	TCP_SKB_CB(skb)->ack_seq = ntohl(th->ack_seq);
 	TCP_SKB_CB(skb)->when = 0;
-	TCP_SKB_CB(skb)->flags = ipv6_get_dsfield(hdr);
+	TCP_SKB_CB(skb)->ip_dsfield = ipv6_get_dsfield(hdr);
 	TCP_SKB_CB(skb)->sacked = 0;
 
 	sk = __inet6_lookup_skb(&tcp_hashinfo, skb, th->source, th->dest);

Re: [PATCH net-next] tcp: unalias tcp_skb_cb flags and ip_dsfield

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 27 Sep 2011 08:00:49 +0200

> struct tcp_skb_cb contains a "flags" field containing either tcp flags
> or IP dsfield depending on context (input or output path)
> 
> Introduce ip_dsfield to make the difference clear and ease maintenance.
> If later we want to save space, we can union flags/ip_dsfield
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks Eric.

> If there is no objection, I plan to rename "flags" to "tcp_flags" in a
> following patch.

No objections.

Re: [PATCH net-next] tcp: unalias tcp_skb_cb flags and ip_dsfield

[Christoph Paasch]

Hi,

I realize that before "struct tcp_skb_cb" is a comment saying that it's 44 
bytes long.

However, (as far as I can see) before Eric's change it was 42 bytes long and 
now it became 43 bytes.

Wouldn't it make sense to change the comment, so that it's consistent again 
with the real size of tcp_skb_cb ?

Cheers,
Christoph



On Tuesday 27 September 2011 wrote David Miller:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 27 Sep 2011 08:00:49 +0200
> 
> > struct tcp_skb_cb contains a "flags" field containing either tcp flags
> > or IP dsfield depending on context (input or output path)
> > 
> > Introduce ip_dsfield to make the difference clear and ease maintenance.
> > If later we want to save space, we can union flags/ip_dsfield
> > 
> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> Applied, thanks Eric.
> 
> > If there is no objection, I plan to rename "flags" to "tcp_flags" in a
> > following patch.
> 
> No objections.
> --
> 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

--
Christoph Paasch
PhD Student

IP Networking Lab --- http://inl.info.ucl.ac.be
MultiPath TCP in the Linux Kernel --- http://inl.info.ucl.ac.be/mptcp
Université Catholique de Louvain

www.rollerbulls.be
--

Re: [PATCH net-next] tcp: unalias tcp_skb_cb flags and ip_dsfield

[Eric Dumazet]

Le mardi 27 septembre 2011 à 10:37 +0300, Christoph Paasch a écrit :
> Hi,
> 
> I realize that before "struct tcp_skb_cb" is a comment saying that it's 44 
> bytes long.
> 
> However, (as far as I can see) before Eric's change it was 42 bytes long and 
> now it became 43 bytes.
> 
> Wouldn't it make sense to change the comment, so that it's consistent again 
> with the real size of tcp_skb_cb ?

You are mistaken :)

There was a two bytes hole.

I took one byte for dsfield, and left one byte hole,

I added a comment to remind there is this hole

Thanks !

Re: [PATCH net-next] tcp: unalias tcp_skb_cb flags and ip_dsfield

[David Miller]

From: Christoph Paasch <christoph.paasch@uclouvain.be>
Date: Tue, 27 Sep 2011 10:37:38 +0300

> However, (as far as I can see) before Eric's change it was 42 bytes long and 
> now it became 43 bytes.

Then, I suggest that you go buy a pair of glasses. :-)

Re: [PATCH net-next] tcp: unalias tcp_skb_cb flags and ip_dsfield

[Christoph Paasch]

On Tuesday 27 September 2011 wrote David Miller:
> From: Christoph Paasch <christoph.paasch@uclouvain.be>
> Date: Tue, 27 Sep 2011 10:37:38 +0300
> 
> > However, (as far as I can see) before Eric's change it was 42 bytes long
> > and now it became 43 bytes.
> 
> Then, I suggest that you go buy a pair of glasses. :-)

Yeah, maybe it's time to... ;-)

This two-byte hole, is it because "flags" and "sacked" are both __u8 and 
fields are aligned to 32-bit ?

Thanks,
Christoph

--
Christoph Paasch
PhD Student

IP Networking Lab --- http://inl.info.ucl.ac.be
MultiPath TCP in the Linux Kernel --- http://inl.info.ucl.ac.be/mptcp
Université Catholique de Louvain

www.rollerbulls.be
--

Re: [PATCH net-next] tcp: unalias tcp_skb_cb flags and ip_dsfield

[Eric Dumazet]

Le mardi 27 septembre 2011 à 11:38 +0300, Christoph Paasch a écrit :
> On Tuesday 27 September 2011 wrote David Miller:
> > From: Christoph Paasch <christoph.paasch@uclouvain.be>
> > Date: Tue, 27 Sep 2011 10:37:38 +0300
> > 
> > > However, (as far as I can see) before Eric's change it was 42 bytes long
> > > and now it became 43 bytes.
> > 
> > Then, I suggest that you go buy a pair of glasses. :-)
> 
> Yeah, maybe it's time to... ;-)
> 
> This two-byte hole, is it because "flags" and "sacked" are both __u8 and 
> fields are aligned to 32-bit ?

Not exactly.

Reason for the hole is the following :

struct whatever {
	u8   first;
	u8   second;
	u32  third;
};

"third" field has an alignement requirement of 4 bytes on most arches.

Compiler inserts a 2 bytes hole before "third" to respect this
requirement.

There is no hole between "first" and "second", since second has no
alignment requirement.

sizeof(struct whatever) = 8

Re: [PATCH net-next] tcp: unalias tcp_skb_cb flags and ip_dsfield

[Christoph Paasch]

Eric,

On Tuesday 27 September 2011 wrote Eric Dumazet:
> Le mardi 27 septembre 2011 à 11:38 +0300, Christoph Paasch a écrit :
> > On Tuesday 27 September 2011 wrote David Miller:
> > This two-byte hole, is it because "flags" and "sacked" are both __u8 and 
> > fields are aligned to 32-bit ?
> 
> Not exactly.
> 
> Reason for the hole is the following :
> 
> struct whatever {
>         u8   first;
>         u8   second;
>         u32  third;
> };
> 
> "third" field has an alignement requirement of 4 bytes on most arches.
> 
> Compiler inserts a 2 bytes hole before "third" to respect this
> requirement.
> 
> There is no hole between "first" and "second", since second has no
> alignment requirement.
> 
> sizeof(struct whatever) = 8


Ok, I understand.
Thanks for the info. I didn't knew this...

It's good news...
Maybe we win now a little bit of space in MultiPath TCP by rearranging our 
fields... :-)


Cheers,
Christoph


--
Christoph Paasch
PhD Student

IP Networking Lab --- http://inl.info.ucl.ac.be
MultiPath TCP in the Linux Kernel --- http://inl.info.ucl.ac.be/mptcp
Université Catholique de Louvain

www.rollerbulls.be
--

Re: [PATCH net-next] tcp: unalias tcp_skb_cb flags and ip_dsfield

[Eric Dumazet]

Le mardi 27 septembre 2011 à 12:28 +0300, Christoph Paasch a écrit :

> 
> Ok, I understand.
> Thanks for the info. I didn't knew this...
> 
> It's good news...
> Maybe we win now a little bit of space in MultiPath TCP by rearranging our 
> fields... :-)

To ease your task, I suggest you take a look at pahole

http://lwn.net/Articles/365844/

[PATCH net-next] tcp: rename tcp_skb_cb flags

[Eric Dumazet]

Rename struct tcp_skb_cb "flags" to "tcp_flags" to ease code review and
maintenance.

Its content is a combination of FIN/SYN/RST/PSH/ACK/URG/ECE/CWR flags

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 include/net/tcp.h     |    2 -
 net/ipv4/tcp.c        |    8 ++---
 net/ipv4/tcp_input.c  |    4 +-
 net/ipv4/tcp_output.c |   63 ++++++++++++++++++++--------------------
 4 files changed, 39 insertions(+), 38 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 28a9997..0113d30 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -636,7 +636,7 @@ struct tcp_skb_cb {
 	__u32		seq;		/* Starting sequence number	*/
 	__u32		end_seq;	/* SEQ + FIN + SYN + datalen	*/
 	__u32		when;		/* used to compute rtt's	*/
-	__u8		flags;		/* TCP header flags.		*/
+	__u8		tcp_flags;	/* TCP header flags. (tcp[13])	*/
 	__u8		sacked;		/* State flags for SACK/FACK.	*/
 #define TCPCB_SACKED_ACKED	0x01	/* SKB ACK'd by a SACK block	*/
 #define TCPCB_SACKED_RETRANS	0x02	/* SKB retransmitted		*/
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index cc0d5de..131c45f 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -524,7 +524,7 @@ EXPORT_SYMBOL(tcp_ioctl);
 
 static inline void tcp_mark_push(struct tcp_sock *tp, struct sk_buff *skb)
 {
-	TCP_SKB_CB(skb)->flags |= TCPHDR_PSH;
+	TCP_SKB_CB(skb)->tcp_flags |= TCPHDR_PSH;
 	tp->pushed_seq = tp->write_seq;
 }
 
@@ -540,7 +540,7 @@ static inline void skb_entail(struct sock *sk, struct sk_buff *skb)
 
 	skb->csum    = 0;
 	tcb->seq     = tcb->end_seq = tp->write_seq;
-	tcb->flags   = TCPHDR_ACK;
+	tcb->tcp_flags = TCPHDR_ACK;
 	tcb->sacked  = 0;
 	skb_header_release(skb);
 	tcp_add_write_queue_tail(sk, skb);
@@ -830,7 +830,7 @@ new_segment:
 		skb_shinfo(skb)->gso_segs = 0;
 
 		if (!copied)
-			TCP_SKB_CB(skb)->flags &= ~TCPHDR_PSH;
+			TCP_SKB_CB(skb)->tcp_flags &= ~TCPHDR_PSH;
 
 		copied += copy;
 		poffset += copy;
@@ -1074,7 +1074,7 @@ new_segment:
 			}
 
 			if (!copied)
-				TCP_SKB_CB(skb)->flags &= ~TCPHDR_PSH;
+				TCP_SKB_CB(skb)->tcp_flags &= ~TCPHDR_PSH;
 
 			tp->write_seq += copy;
 			TCP_SKB_CB(skb)->end_seq += copy;
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 7008fcc..143221e 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1449,7 +1449,7 @@ static int tcp_shifted_skb(struct sock *sk, struct sk_buff *skb,
 		tp->lost_cnt_hint -= tcp_skb_pcount(prev);
 	}
 
-	TCP_SKB_CB(skb)->flags |= TCP_SKB_CB(prev)->flags;
+	TCP_SKB_CB(skb)->tcp_flags |= TCP_SKB_CB(prev)->tcp_flags;
 	if (skb == tcp_highest_sack(sk))
 		tcp_advance_highest_sack(sk, skb);
 
@@ -3348,7 +3348,7 @@ static int tcp_clean_rtx_queue(struct sock *sk, int prior_fackets,
 		 * connection startup slow start one packet too
 		 * quickly.  This is severely frowned upon behavior.
 		 */
-		if (!(scb->flags & TCPHDR_SYN)) {
+		if (!(scb->tcp_flags & TCPHDR_SYN)) {
 			flag |= FLAG_DATA_ACKED;
 		} else {
 			flag |= FLAG_SYN_ACKED;
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 081dcd6..dde6b57 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -297,9 +297,9 @@ static u16 tcp_select_window(struct sock *sk)
 /* Packet ECN state for a SYN-ACK */
 static inline void TCP_ECN_send_synack(struct tcp_sock *tp, struct sk_buff *skb)
 {
-	TCP_SKB_CB(skb)->flags &= ~TCPHDR_CWR;
+	TCP_SKB_CB(skb)->tcp_flags &= ~TCPHDR_CWR;
 	if (!(tp->ecn_flags & TCP_ECN_OK))
-		TCP_SKB_CB(skb)->flags &= ~TCPHDR_ECE;
+		TCP_SKB_CB(skb)->tcp_flags &= ~TCPHDR_ECE;
 }
 
 /* Packet ECN state for a SYN.  */
@@ -309,7 +309,7 @@ static inline void TCP_ECN_send_syn(struct sock *sk, struct sk_buff *skb)
 
 	tp->ecn_flags = 0;
 	if (sysctl_tcp_ecn == 1) {
-		TCP_SKB_CB(skb)->flags |= TCPHDR_ECE | TCPHDR_CWR;
+		TCP_SKB_CB(skb)->tcp_flags |= TCPHDR_ECE | TCPHDR_CWR;
 		tp->ecn_flags = TCP_ECN_OK;
 	}
 }
@@ -356,7 +356,7 @@ static void tcp_init_nondata_skb(struct sk_buff *skb, u32 seq, u8 flags)
 	skb->ip_summed = CHECKSUM_PARTIAL;
 	skb->csum = 0;
 
-	TCP_SKB_CB(skb)->flags = flags;
+	TCP_SKB_CB(skb)->tcp_flags = flags;
 	TCP_SKB_CB(skb)->sacked = 0;
 
 	skb_shinfo(skb)->gso_segs = 1;
@@ -826,7 +826,7 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	tcb = TCP_SKB_CB(skb);
 	memset(&opts, 0, sizeof(opts));
 
-	if (unlikely(tcb->flags & TCPHDR_SYN))
+	if (unlikely(tcb->tcp_flags & TCPHDR_SYN))
 		tcp_options_size = tcp_syn_options(sk, skb, &opts, &md5);
 	else
 		tcp_options_size = tcp_established_options(sk, skb, &opts,
@@ -850,9 +850,9 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	th->seq			= htonl(tcb->seq);
 	th->ack_seq		= htonl(tp->rcv_nxt);
 	*(((__be16 *)th) + 6)	= htons(((tcp_header_size >> 2) << 12) |
-					tcb->flags);
+					tcb->tcp_flags);
 
-	if (unlikely(tcb->flags & TCPHDR_SYN)) {
+	if (unlikely(tcb->tcp_flags & TCPHDR_SYN)) {
 		/* RFC1323: The window in SYN & SYN/ACK segments
 		 * is never scaled.
 		 */
@@ -875,7 +875,7 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 	}
 
 	tcp_options_write((__be32 *)(th + 1), tp, &opts);
-	if (likely((tcb->flags & TCPHDR_SYN) == 0))
+	if (likely((tcb->tcp_flags & TCPHDR_SYN) == 0))
 		TCP_ECN_send(sk, skb, tcp_header_size);
 
 #ifdef CONFIG_TCP_MD5SIG
@@ -889,7 +889,7 @@ static int tcp_transmit_skb(struct sock *sk, struct sk_buff *skb, int clone_it,
 
 	icsk->icsk_af_ops->send_check(sk, skb);
 
-	if (likely(tcb->flags & TCPHDR_ACK))
+	if (likely(tcb->tcp_flags & TCPHDR_ACK))
 		tcp_event_ack_sent(sk, tcp_skb_pcount(skb));
 
 	if (skb->len != tcp_header_size)
@@ -1032,9 +1032,9 @@ int tcp_fragment(struct sock *sk, struct sk_buff *skb, u32 len,
 	TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(buff)->seq;
 
 	/* PSH and FIN should only be set in the second packet. */
-	flags = TCP_SKB_CB(skb)->flags;
-	TCP_SKB_CB(skb)->flags = flags & ~(TCPHDR_FIN | TCPHDR_PSH);
-	TCP_SKB_CB(buff)->flags = flags;
+	flags = TCP_SKB_CB(skb)->tcp_flags;
+	TCP_SKB_CB(skb)->tcp_flags = flags & ~(TCPHDR_FIN | TCPHDR_PSH);
+	TCP_SKB_CB(buff)->tcp_flags = flags;
 	TCP_SKB_CB(buff)->sacked = TCP_SKB_CB(skb)->sacked;
 
 	if (!skb_shinfo(skb)->nr_frags && skb->ip_summed != CHECKSUM_PARTIAL) {
@@ -1340,7 +1340,8 @@ static inline unsigned int tcp_cwnd_test(struct tcp_sock *tp,
 	u32 in_flight, cwnd;
 
 	/* Don't be strict about the congestion window for the final FIN.  */
-	if ((TCP_SKB_CB(skb)->flags & TCPHDR_FIN) && tcp_skb_pcount(skb) == 1)
+	if ((TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN) &&
+	    tcp_skb_pcount(skb) == 1)
 		return 1;
 
 	in_flight = tcp_packets_in_flight(tp);
@@ -1409,7 +1410,7 @@ static inline int tcp_nagle_test(struct tcp_sock *tp, struct sk_buff *skb,
 	 * Nagle can be ignored during F-RTO too (see RFC4138).
 	 */
 	if (tcp_urg_mode(tp) || (tp->frto_counter == 2) ||
-	    (TCP_SKB_CB(skb)->flags & TCPHDR_FIN))
+	    (TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN))
 		return 1;
 
 	if (!tcp_nagle_check(tp, skb, cur_mss, nonagle))
@@ -1497,9 +1498,9 @@ static int tso_fragment(struct sock *sk, struct sk_buff *skb, unsigned int len,
 	TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(buff)->seq;
 
 	/* PSH and FIN should only be set in the second packet. */
-	flags = TCP_SKB_CB(skb)->flags;
-	TCP_SKB_CB(skb)->flags = flags & ~(TCPHDR_FIN | TCPHDR_PSH);
-	TCP_SKB_CB(buff)->flags = flags;
+	flags = TCP_SKB_CB(skb)->tcp_flags;
+	TCP_SKB_CB(skb)->tcp_flags = flags & ~(TCPHDR_FIN | TCPHDR_PSH);
+	TCP_SKB_CB(buff)->tcp_flags = flags;
 
 	/* This packet was never sent out yet, so no SACK bits. */
 	TCP_SKB_CB(buff)->sacked = 0;
@@ -1530,7 +1531,7 @@ static int tcp_tso_should_defer(struct sock *sk, struct sk_buff *skb)
 	u32 send_win, cong_win, limit, in_flight;
 	int win_divisor;
 
-	if (TCP_SKB_CB(skb)->flags & TCPHDR_FIN)
+	if (TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN)
 		goto send_now;
 
 	if (icsk->icsk_ca_state != TCP_CA_Open)
@@ -1657,7 +1658,7 @@ static int tcp_mtu_probe(struct sock *sk)
 
 	TCP_SKB_CB(nskb)->seq = TCP_SKB_CB(skb)->seq;
 	TCP_SKB_CB(nskb)->end_seq = TCP_SKB_CB(skb)->seq + probe_size;
-	TCP_SKB_CB(nskb)->flags = TCPHDR_ACK;
+	TCP_SKB_CB(nskb)->tcp_flags = TCPHDR_ACK;
 	TCP_SKB_CB(nskb)->sacked = 0;
 	nskb->csum = 0;
 	nskb->ip_summed = skb->ip_summed;
@@ -1677,11 +1678,11 @@ static int tcp_mtu_probe(struct sock *sk)
 		if (skb->len <= copy) {
 			/* We've eaten all the data from this skb.
 			 * Throw it away. */
-			TCP_SKB_CB(nskb)->flags |= TCP_SKB_CB(skb)->flags;
+			TCP_SKB_CB(nskb)->tcp_flags |= TCP_SKB_CB(skb)->tcp_flags;
 			tcp_unlink_write_queue(skb, sk);
 			sk_wmem_free_skb(sk, skb);
 		} else {
-			TCP_SKB_CB(nskb)->flags |= TCP_SKB_CB(skb)->flags &
+			TCP_SKB_CB(nskb)->tcp_flags |= TCP_SKB_CB(skb)->tcp_flags &
 						   ~(TCPHDR_FIN|TCPHDR_PSH);
 			if (!skb_shinfo(skb)->nr_frags) {
 				skb_pull(skb, copy);
@@ -1987,7 +1988,7 @@ static void tcp_collapse_retrans(struct sock *sk, struct sk_buff *skb)
 	TCP_SKB_CB(skb)->end_seq = TCP_SKB_CB(next_skb)->end_seq;
 
 	/* Merge over control information. This moves PSH/FIN etc. over */
-	TCP_SKB_CB(skb)->flags |= TCP_SKB_CB(next_skb)->flags;
+	TCP_SKB_CB(skb)->tcp_flags |= TCP_SKB_CB(next_skb)->tcp_flags;
 
 	/* All done, get rid of second SKB and account for it so
 	 * packet counting does not break.
@@ -2035,7 +2036,7 @@ static void tcp_retrans_try_collapse(struct sock *sk, struct sk_buff *to,
 
 	if (!sysctl_tcp_retrans_collapse)
 		return;
-	if (TCP_SKB_CB(skb)->flags & TCPHDR_SYN)
+	if (TCP_SKB_CB(skb)->tcp_flags & TCPHDR_SYN)
 		return;
 
 	tcp_for_write_queue_from_safe(skb, tmp, sk) {
@@ -2127,12 +2128,12 @@ int tcp_retransmit_skb(struct sock *sk, struct sk_buff *skb)
 	 * since it is cheap to do so and saves bytes on the network.
 	 */
 	if (skb->len > 0 &&
-	    (TCP_SKB_CB(skb)->flags & TCPHDR_FIN) &&
+	    (TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN) &&
 	    tp->snd_una == (TCP_SKB_CB(skb)->end_seq - 1)) {
 		if (!pskb_trim(skb, 0)) {
 			/* Reuse, even though it does some unnecessary work */
 			tcp_init_nondata_skb(skb, TCP_SKB_CB(skb)->end_seq - 1,
-					     TCP_SKB_CB(skb)->flags);
+					     TCP_SKB_CB(skb)->tcp_flags);
 			skb->ip_summed = CHECKSUM_NONE;
 		}
 	}
@@ -2322,7 +2323,7 @@ void tcp_send_fin(struct sock *sk)
 	mss_now = tcp_current_mss(sk);
 
 	if (tcp_send_head(sk) != NULL) {
-		TCP_SKB_CB(skb)->flags |= TCPHDR_FIN;
+		TCP_SKB_CB(skb)->tcp_flags |= TCPHDR_FIN;
 		TCP_SKB_CB(skb)->end_seq++;
 		tp->write_seq++;
 	} else {
@@ -2384,11 +2385,11 @@ int tcp_send_synack(struct sock *sk)
 	struct sk_buff *skb;
 
 	skb = tcp_write_queue_head(sk);
-	if (skb == NULL || !(TCP_SKB_CB(skb)->flags & TCPHDR_SYN)) {
+	if (skb == NULL || !(TCP_SKB_CB(skb)->tcp_flags & TCPHDR_SYN)) {
 		printk(KERN_DEBUG "tcp_send_synack: wrong queue state\n");
 		return -EFAULT;
 	}
-	if (!(TCP_SKB_CB(skb)->flags & TCPHDR_ACK)) {
+	if (!(TCP_SKB_CB(skb)->tcp_flags & TCPHDR_ACK)) {
 		if (skb_cloned(skb)) {
 			struct sk_buff *nskb = skb_copy(skb, GFP_ATOMIC);
 			if (nskb == NULL)
@@ -2402,7 +2403,7 @@ int tcp_send_synack(struct sock *sk)
 			skb = nskb;
 		}
 
-		TCP_SKB_CB(skb)->flags |= TCPHDR_ACK;
+		TCP_SKB_CB(skb)->tcp_flags |= TCPHDR_ACK;
 		TCP_ECN_send_synack(tcp_sk(sk), skb);
 	}
 	TCP_SKB_CB(skb)->when = tcp_time_stamp;
@@ -2799,13 +2800,13 @@ int tcp_write_wakeup(struct sock *sk)
 		if (seg_size < TCP_SKB_CB(skb)->end_seq - TCP_SKB_CB(skb)->seq ||
 		    skb->len > mss) {
 			seg_size = min(seg_size, mss);
-			TCP_SKB_CB(skb)->flags |= TCPHDR_PSH;
+			TCP_SKB_CB(skb)->tcp_flags |= TCPHDR_PSH;
 			if (tcp_fragment(sk, skb, seg_size, mss))
 				return -1;
 		} else if (!tcp_skb_pcount(skb))
 			tcp_set_skb_tso_segs(sk, skb, mss);
 
-		TCP_SKB_CB(skb)->flags |= TCPHDR_PSH;
+		TCP_SKB_CB(skb)->tcp_flags |= TCPHDR_PSH;
 		TCP_SKB_CB(skb)->when = tcp_time_stamp;
 		err = tcp_transmit_skb(sk, skb, 1, GFP_ATOMIC);
 		if (!err)

Re: [PATCH net-next] tcp: rename tcp_skb_cb flags

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 27 Sep 2011 11:51:54 +0200

> Rename struct tcp_skb_cb "flags" to "tcp_flags" to ease code review and
> maintenance.
> 
> Its content is a combination of FIN/SYN/RST/PSH/ACK/URG/ECE/CWR flags
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied.