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

login
register
mail settings
Submitter Eric Dumazet
Date Sept. 27, 2011, 6 a.m.
Message ID <1317103249.2796.25.camel@edumazet-laptop>
Download mbox | patch
Permalink /patch/116544/
State Accepted
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - Sept. 27, 2011, 6 a.m.
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(-)



--
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
David Miller - Sept. 27, 2011, 6:20 a.m.
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 - Sept. 27, 2011, 7:37 a.m.
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
--
--
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
Eric Dumazet - Sept. 27, 2011, 8:01 a.m.
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 !


--
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
David Miller - Sept. 27, 2011, 8:08 a.m.
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. :-)

--
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 - Sept. 27, 2011, 8:38 a.m.
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
--
--
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
Eric Dumazet - Sept. 27, 2011, 9:23 a.m.
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


--
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 - Sept. 27, 2011, 9:28 a.m.
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
--
--
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
Eric Dumazet - Sept. 27, 2011, 9:40 a.m.
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/



--
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

Patch

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);