| Message ID | 1317103249.2796.25.camel@edumazet-laptop |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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
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
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
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);
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