| Message ID | 1369205836-9407-2-git-send-email-horms@verge.net.au |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 22 May 2013, Simon Horman wrote: > include/linux/skbuff.h | 63 +++++++------------------------------------------- > 1 file changed, 8 insertions(+), 55 deletions(-) > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > index 2e0ced1..3791615 100644 > --- a/include/linux/skbuff.h > +++ b/include/linux/skbuff.h > @@ -509,9 +509,9 @@ struct sk_buff { > __u32 reserved_tailroom; > }; > > - sk_buff_data_t inner_transport_header; > - sk_buff_data_t inner_network_header; > - sk_buff_data_t inner_mac_header; > + __u16 inner_transport_header; > + __u16 inner_network_header; > + __u16 inner_mac_header; > sk_buff_data_t transport_header; > sk_buff_data_t network_header; > sk_buff_data_t mac_header; > Why not take one more step and use 16-bit fields for the other header fields too: transport_header, network_header and mac_header? Pekka -- 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 Wed, 2013-05-22 at 09:25 +0200, Pekka Riikonen wrote: > On Wed, 22 May 2013, Simon Horman wrote: > > > include/linux/skbuff.h | 63 +++++++------------------------------------------- > > 1 file changed, 8 insertions(+), 55 deletions(-) > > > > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h > > index 2e0ced1..3791615 100644 > > --- a/include/linux/skbuff.h > > +++ b/include/linux/skbuff.h > > @@ -509,9 +509,9 @@ struct sk_buff { > > __u32 reserved_tailroom; > > }; > > > > - sk_buff_data_t inner_transport_header; > > - sk_buff_data_t inner_network_header; > > - sk_buff_data_t inner_mac_header; > > + __u16 inner_transport_header; > > + __u16 inner_network_header; > > + __u16 inner_mac_header; > > sk_buff_data_t transport_header; > > sk_buff_data_t network_header; > > sk_buff_data_t mac_header; > > > Why not take one more step and use 16-bit fields for the other header > fields too: transport_header, network_header and mac_header? Thats more risky [1], but definitely worth doing [1] commit 50bceae9bd3569d56744882f3012734d48a1d413 Author: Thomas Graf <tgraf@suug.ch> Date: Thu Apr 11 10:57:18 2013 +0000 tcp: Reallocate headroom if it would overflow csum_start If a TCP retransmission gets partially ACKed and collapsed multiple times it is possible for the headroom to grow beyond 64K which will overflow the 16bit skb->csum_start which is based on the start of the headroom. It has been observed rarely in the wild with IPoIB due to the 64K MTU. Verify if the acking and collapsing resulted in a headroom exceeding what csum_start can cover and reallocate the headroom if so. A big thank you to Jim Foraker <foraker1@llnl.gov> and the team at LLNL for helping out with the investigation and testing. Reported-by: Jim Foraker <foraker1@llnl.gov> Signed-off-by: Thomas Graf <tgraf@suug.ch> Acked-by: Eric Dumazet <edumazet@google.com> Signed-off-by: David S. Miller <davem@davemloft.net> -- 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: Simon Horman <horms@verge.net.au> Date: Wed, 22 May 2013 15:57:15 +0900 > @@ -509,9 +509,9 @@ struct sk_buff { > __u32 reserved_tailroom; > }; > > - sk_buff_data_t inner_transport_header; > - sk_buff_data_t inner_network_header; > - sk_buff_data_t inner_mac_header; > + __u16 inner_transport_header; > + __u16 inner_network_header; > + __u16 inner_mac_header; > sk_buff_data_t transport_header; > sk_buff_data_t network_header; > sk_buff_data_t mac_header; As Pekka suggested, let's try to convert them allto __u16. 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
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 2e0ced1..3791615 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -509,9 +509,9 @@ struct sk_buff { __u32 reserved_tailroom; }; - sk_buff_data_t inner_transport_header; - sk_buff_data_t inner_network_header; - sk_buff_data_t inner_mac_header; + __u16 inner_transport_header; + __u16 inner_network_header; + __u16 inner_mac_header; sk_buff_data_t transport_header; sk_buff_data_t network_header; sk_buff_data_t mac_header; @@ -1517,9 +1517,9 @@ static inline void skb_reserve(struct sk_buff *skb, int len) static inline void skb_reset_inner_headers(struct sk_buff *skb) { - skb->inner_mac_header = skb->mac_header; - skb->inner_network_header = skb->network_header; - skb->inner_transport_header = skb->transport_header; + skb->inner_mac_header = 0; + skb->inner_network_header = 0; + skb->inner_transport_header = 0; } static inline void skb_reset_mac_len(struct sk_buff *skb) @@ -1527,7 +1527,6 @@ static inline void skb_reset_mac_len(struct sk_buff *skb) skb->mac_len = skb->network_header - skb->mac_header; } -#ifdef NET_SKBUFF_DATA_USES_OFFSET static inline unsigned char *skb_inner_transport_header(const struct sk_buff *skb) { @@ -1579,6 +1578,8 @@ static inline void skb_set_inner_mac_header(struct sk_buff *skb, skb_reset_inner_mac_header(skb); skb->inner_mac_header += offset; } + +#ifdef NET_SKBUFF_DATA_USES_OFFSET static inline bool skb_transport_header_was_set(const struct sk_buff *skb) { return skb->transport_header != ~0U; @@ -1639,54 +1640,6 @@ static inline void skb_set_mac_header(struct sk_buff *skb, const int offset) } #else /* NET_SKBUFF_DATA_USES_OFFSET */ -static inline unsigned char *skb_inner_transport_header(const struct sk_buff - *skb) -{ - return skb->inner_transport_header; -} - -static inline void skb_reset_inner_transport_header(struct sk_buff *skb) -{ - skb->inner_transport_header = skb->data; -} - -static inline void skb_set_inner_transport_header(struct sk_buff *skb, - const int offset) -{ - skb->inner_transport_header = skb->data + offset; -} - -static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb) -{ - return skb->inner_network_header; -} - -static inline void skb_reset_inner_network_header(struct sk_buff *skb) -{ - skb->inner_network_header = skb->data; -} - -static inline void skb_set_inner_network_header(struct sk_buff *skb, - const int offset) -{ - skb->inner_network_header = skb->data + offset; -} - -static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb) -{ - return skb->inner_mac_header; -} - -static inline void skb_reset_inner_mac_header(struct sk_buff *skb) -{ - skb->inner_mac_header = skb->data; -} - -static inline void skb_set_inner_mac_header(struct sk_buff *skb, - const int offset) -{ - skb->inner_mac_header = skb->data + offset; -} static inline bool skb_transport_header_was_set(const struct sk_buff *skb) { return skb->transport_header != NULL;
In order to reduce the size of struct skbuff use 16 bit integer offsets rather than pointers for inner_*_headers. The implication of this is that the behaviour of is that of other _headers when NET_SKBUFF_DATA_USES_OFFSET is set. This appears to reduce the size of struct skbuff from 0xd0 to 0xc8 bytes on x86_64 with the following all unset. CONFIG_XFRM CONFIG_NF_CONNTRACK CONFIG_NF_CONNTRACK_MODULE NET_SKBUFF_NF_DEFRAG_NEEDED CONFIG_BRIDGE_NETFILTER CONFIG_NET_SCHED CONFIG_IPV6_NDISC_NODETYPE CONFIG_NET_DMA CONFIG_NETWORK_SECMARK Signed-off-by: Simon Horman <horms@verge.net.au> --- include/linux/skbuff.h | 63 +++++++------------------------------------------- 1 file changed, 8 insertions(+), 55 deletions(-)