| Message ID | 20140520233145.GA30260@verge.net.au |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, May 20, 2014 at 4:31 PM, Simon Horman <horms@verge.net.au> wrote: > On Tue, May 20, 2014 at 12:54:32PM -0700, Jesse Gross wrote: >> On Tue, May 20, 2014 at 3:41 AM, Simon Horman <horms@verge.net.au> wrote: >> > diff --git a/datapath/linux/compat/gso.h b/datapath/linux/compat/gso.h >> > index b83a4c3..f459c18 100644 >> > --- a/datapath/linux/compat/gso.h >> > +++ b/datapath/linux/compat/gso.h >> > #ifdef NET_SKBUFF_DATA_USES_OFFSET >> > -#define skb_inner_network_header rpl_skb_inner_network_header >> > -static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb) >> > -{ >> > - return skb->head + OVS_GSO_CB(skb)->inner_network_header; >> > -} >> > - >> > #define skb_inner_mac_header rpl_skb_inner_mac_header >> > static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb) >> > { >> > return skb->head + OVS_GSO_CB(skb)->inner_mac_header; >> > } >> > >> > +#define skb_inner_network_header rpl_skb_inner_network_header >> > +static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb) >> > +{ >> > + return skb_inner_mac_header(skb) + >> > + OVS_GSO_CB(skb)->inner_network_header; >> > +} >> > + >> > #else >> >> This looks mostly fine to me but doesn't this need to be outside the >> #ifdef NET_SKBUFF_DATA_USES_OFFSET block? Otherwise we will treat the >> offset as a pointer on 32 bit kernels. > > Thanks, Somehow I completely missed that. > How about this? > > > From: Simon Horman <horms@verge.net.au> > > [PATCH v1.1] datapath: 16bit inner_network_header field in struct ovs_gso_cb > > The motivation for this is to create a 16bit hole in struct ovs_gso_cb > which may be used for the inner_protocol field which is needed > for the proposed implementation of compatibility for MPLS GSO segmentation. > > This should be safe as inner_network_header is now an offset to > the inner_mac_header rather than skb->head. > > As pointed out by Thomas Graf simply making both inner offsets 16bis is not > safe as there have been cases of overflow with "with collapsed TCP frames > on IB when the headroom grew beyond 64K. See commit 50bceae9bd ``tcp: > Reallocate headroom if it would overflow csum_start'' for additional > details." > > This patch is based on suggestions by Thomas Graf and Jesse Gross. > > Cc: Thomas Graf <tgraf@suug.ch> > Cc: Jesse Gross <jesse@nicira.com> > Signed-off-by: Simon Horman <horms@verge.net.au> Applied, 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
On Tue, May 20, 2014 at 05:14:25PM -0700, Jesse Gross wrote: > On Tue, May 20, 2014 at 4:31 PM, Simon Horman <horms@verge.net.au> wrote: > > On Tue, May 20, 2014 at 12:54:32PM -0700, Jesse Gross wrote: > >> On Tue, May 20, 2014 at 3:41 AM, Simon Horman <horms@verge.net.au> wrote: > >> > diff --git a/datapath/linux/compat/gso.h b/datapath/linux/compat/gso.h > >> > index b83a4c3..f459c18 100644 > >> > --- a/datapath/linux/compat/gso.h > >> > +++ b/datapath/linux/compat/gso.h > >> > #ifdef NET_SKBUFF_DATA_USES_OFFSET > >> > -#define skb_inner_network_header rpl_skb_inner_network_header > >> > -static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb) > >> > -{ > >> > - return skb->head + OVS_GSO_CB(skb)->inner_network_header; > >> > -} > >> > - > >> > #define skb_inner_mac_header rpl_skb_inner_mac_header > >> > static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb) > >> > { > >> > return skb->head + OVS_GSO_CB(skb)->inner_mac_header; > >> > } > >> > > >> > +#define skb_inner_network_header rpl_skb_inner_network_header > >> > +static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb) > >> > +{ > >> > + return skb_inner_mac_header(skb) + > >> > + OVS_GSO_CB(skb)->inner_network_header; > >> > +} > >> > + > >> > #else > >> > >> This looks mostly fine to me but doesn't this need to be outside the > >> #ifdef NET_SKBUFF_DATA_USES_OFFSET block? Otherwise we will treat the > >> offset as a pointer on 32 bit kernels. > > > > Thanks, Somehow I completely missed that. > > How about this? > > > > > > From: Simon Horman <horms@verge.net.au> > > > > [PATCH v1.1] datapath: 16bit inner_network_header field in struct ovs_gso_cb > > > > The motivation for this is to create a 16bit hole in struct ovs_gso_cb > > which may be used for the inner_protocol field which is needed > > for the proposed implementation of compatibility for MPLS GSO segmentation. > > > > This should be safe as inner_network_header is now an offset to > > the inner_mac_header rather than skb->head. > > > > As pointed out by Thomas Graf simply making both inner offsets 16bis is not > > safe as there have been cases of overflow with "with collapsed TCP frames > > on IB when the headroom grew beyond 64K. See commit 50bceae9bd ``tcp: > > Reallocate headroom if it would overflow csum_start'' for additional > > details." > > > > This patch is based on suggestions by Thomas Graf and Jesse Gross. > > > > Cc: Thomas Graf <tgraf@suug.ch> > > Cc: Jesse Gross <jesse@nicira.com> > > Signed-off-by: Simon Horman <horms@verge.net.au> > > Applied, thanks. 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/datapath/linux/compat/gso.h b/datapath/linux/compat/gso.h index b83a4c3..3041e88 100644 --- a/datapath/linux/compat/gso.h +++ b/datapath/linux/compat/gso.h @@ -11,8 +11,10 @@ struct ovs_gso_cb { struct ovs_skb_cb dp_cb; - sk_buff_data_t inner_network_header; - sk_buff_data_t inner_mac_header; + u16 inner_network_header; /* Offset from + * inner_mac_header */ + /* 16bit hole */ + sk_buff_data_t inner_mac_header; /* Offset from skb->head */ void (*fix_segment)(struct sk_buff *); }; #define OVS_GSO_CB(skb) ((struct ovs_gso_cb *)(skb)->cb) @@ -20,12 +22,6 @@ struct ovs_gso_cb { #define skb_inner_network_header rpl_skb_inner_network_header #ifdef NET_SKBUFF_DATA_USES_OFFSET -#define skb_inner_network_header rpl_skb_inner_network_header -static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb) -{ - return skb->head + OVS_GSO_CB(skb)->inner_network_header; -} - #define skb_inner_mac_header rpl_skb_inner_mac_header static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb) { @@ -34,12 +30,6 @@ static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb) #else -#define skb_inner_network_header rpl_skb_inner_network_header -static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb) -{ - return OVS_GSO_CB(skb)->inner_network_header; -} - #define skb_inner_mac_header rpl_skb_inner_mac_header static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb) { @@ -48,6 +38,13 @@ static inline unsigned char *skb_inner_mac_header(const struct sk_buff *skb) #endif +#define skb_inner_network_header rpl_skb_inner_network_header +static inline unsigned char *skb_inner_network_header(const struct sk_buff *skb) +{ + return skb_inner_mac_header(skb) + + OVS_GSO_CB(skb)->inner_network_header; +} + #define skb_inner_network_offset rpl_skb_inner_network_offset static inline int skb_inner_network_offset(const struct sk_buff *skb) { @@ -64,7 +61,8 @@ static inline int skb_inner_mac_offset(const struct sk_buff *skb) static inline void skb_reset_inner_headers(struct sk_buff *skb) { BUILD_BUG_ON(sizeof(struct ovs_gso_cb) > FIELD_SIZEOF(struct sk_buff, cb)); - OVS_GSO_CB(skb)->inner_network_header = skb->network_header; + OVS_GSO_CB(skb)->inner_network_header = skb->network_header - + skb->mac_header; OVS_GSO_CB(skb)->inner_mac_header = skb->mac_header; OVS_GSO_CB(skb)->fix_segment = NULL;