| Message ID | 1469571849-116091-2-git-send-email-pshelar@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Jul 26, 2016 at 3:24 PM, Pravin B Shelar <pshelar@ovn.org> wrote: > diff --git a/datapath/linux/compat/include/net/udp.h b/datapath/linux/compat/include/net/udp.h > index fa49fa5..266e70a 100644 > --- a/datapath/linux/compat/include/net/udp.h > +++ b/datapath/linux/compat/include/net/udp.h > @@ -54,7 +54,7 @@ static inline __sum16 udp_v4_check(int len, __be32 saddr, > } > #endif > > -#ifndef HAVE_UDP_SET_CSUM > +#if LINUX_VERSION_CODE < KERNEL_VERSION(3,18,0) I'm a little nervous about these version checks being hard to maintain - especially since they don't correspond to anything obvious in this function upstream. Maybe we could just declare a #define with a name that would make it clearer. That might actually be useful in any case since I suspect that we will start seeing some backports in distributions that will allow us to avoid doing OVS segmentation even on older kernels.
On Tue, Jul 26, 2016 at 3:53 PM, Jesse Gross <jesse@kernel.org> wrote: > On Tue, Jul 26, 2016 at 3:24 PM, Pravin B Shelar <pshelar@ovn.org> wrote: >> diff --git a/datapath/linux/compat/include/net/udp.h b/datapath/linux/compat/include/net/udp.h >> index fa49fa5..266e70a 100644 >> --- a/datapath/linux/compat/include/net/udp.h >> +++ b/datapath/linux/compat/include/net/udp.h >> @@ -54,7 +54,7 @@ static inline __sum16 udp_v4_check(int len, __be32 saddr, >> } >> #endif >> >> -#ifndef HAVE_UDP_SET_CSUM >> +#if LINUX_VERSION_CODE < KERNEL_VERSION(3,18,0) > > I'm a little nervous about these version checks being hard to maintain > - especially since they don't correspond to anything obvious in this > function upstream. Maybe we could just declare a #define with a name > that would make it clearer. That might actually be useful in any case > since I suspect that we will start seeing some backports in > distributions that will allow us to avoid doing OVS segmentation even > on older kernels. Is it fine if I do it as part of separate patch? This patch is about fixing the UDP checksum issue. And the requested change is about general code improvement.
On Tue, Jul 26, 2016 at 3:59 PM, pravin shelar <pshelar@ovn.org> wrote: > On Tue, Jul 26, 2016 at 3:53 PM, Jesse Gross <jesse@kernel.org> wrote: >> On Tue, Jul 26, 2016 at 3:24 PM, Pravin B Shelar <pshelar@ovn.org> wrote: >>> diff --git a/datapath/linux/compat/include/net/udp.h b/datapath/linux/compat/include/net/udp.h >>> index fa49fa5..266e70a 100644 >>> --- a/datapath/linux/compat/include/net/udp.h >>> +++ b/datapath/linux/compat/include/net/udp.h >>> @@ -54,7 +54,7 @@ static inline __sum16 udp_v4_check(int len, __be32 saddr, >>> } >>> #endif >>> >>> -#ifndef HAVE_UDP_SET_CSUM >>> +#if LINUX_VERSION_CODE < KERNEL_VERSION(3,18,0) >> >> I'm a little nervous about these version checks being hard to maintain >> - especially since they don't correspond to anything obvious in this >> function upstream. Maybe we could just declare a #define with a name >> that would make it clearer. That might actually be useful in any case >> since I suspect that we will start seeing some backports in >> distributions that will allow us to avoid doing OVS segmentation even >> on older kernels. > > Is it fine if I do it as part of separate patch? This patch is about > fixing the UDP checksum issue. And the requested change is about > general code improvement. Yes, that's fine. I think we'll want to convert all of the GSO related 3.18 version checks to use this symbol, so that's mostly not related to checksums anyways. Acked-by: Jesse Gross <jesse@kernel.org>
On Tue, Jul 26, 2016 at 4:03 PM, Jesse Gross <jesse@kernel.org> wrote: > On Tue, Jul 26, 2016 at 3:59 PM, pravin shelar <pshelar@ovn.org> wrote: >> On Tue, Jul 26, 2016 at 3:53 PM, Jesse Gross <jesse@kernel.org> wrote: >>> On Tue, Jul 26, 2016 at 3:24 PM, Pravin B Shelar <pshelar@ovn.org> wrote: >>>> diff --git a/datapath/linux/compat/include/net/udp.h b/datapath/linux/compat/include/net/udp.h >>>> index fa49fa5..266e70a 100644 >>>> --- a/datapath/linux/compat/include/net/udp.h >>>> +++ b/datapath/linux/compat/include/net/udp.h >>>> @@ -54,7 +54,7 @@ static inline __sum16 udp_v4_check(int len, __be32 saddr, >>>> } >>>> #endif >>>> >>>> -#ifndef HAVE_UDP_SET_CSUM >>>> +#if LINUX_VERSION_CODE < KERNEL_VERSION(3,18,0) >>> >>> I'm a little nervous about these version checks being hard to maintain >>> - especially since they don't correspond to anything obvious in this >>> function upstream. Maybe we could just declare a #define with a name >>> that would make it clearer. That might actually be useful in any case >>> since I suspect that we will start seeing some backports in >>> distributions that will allow us to avoid doing OVS segmentation even >>> on older kernels. >> >> Is it fine if I do it as part of separate patch? This patch is about >> fixing the UDP checksum issue. And the requested change is about >> general code improvement. > > Yes, that's fine. I think we'll want to convert all of the GSO related > 3.18 version checks to use this symbol, so that's mostly not related > to checksums anyways. > > Acked-by: Jesse Gross <jesse@kernel.org> Thanks for reviews, I pushed this series to master. I also pushed first two patches to branch 2.5.
diff --git a/acinclude.m4 b/acinclude.m4 index 5f38539..3f31e5f 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -639,7 +639,6 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ [OVS_GREP_IFELSE([$KSRC/include/net/udp.h], [inet_get_local_port_range(net], [OVS_DEFINE([HAVE_UDP_FLOW_SRC_PORT])])]) OVS_GREP_IFELSE([$KSRC/include/net/udp.h], [udp_v4_check]) - OVS_GREP_IFELSE([$KSRC/include/net/udp.h], [udp_set_csum]) OVS_GREP_IFELSE([$KSRC/include/net/udp_tunnel.h], [udp_tunnel_gro_complete]) OVS_FIND_FIELD_IFELSE([$KSRC/include/net/udp_tunnel.h], [udp_tunnel_sock_cfg], [gro_receive]) diff --git a/datapath/linux/compat/include/net/udp.h b/datapath/linux/compat/include/net/udp.h index fa49fa5..266e70a 100644 --- a/datapath/linux/compat/include/net/udp.h +++ b/datapath/linux/compat/include/net/udp.h @@ -54,7 +54,7 @@ static inline __sum16 udp_v4_check(int len, __be32 saddr, } #endif -#ifndef HAVE_UDP_SET_CSUM +#if LINUX_VERSION_CODE < KERNEL_VERSION(3,18,0) #define udp_set_csum rpl_udp_set_csum void rpl_udp_set_csum(bool nocheck, struct sk_buff *skb, __be32 saddr, __be32 daddr, int len); diff --git a/datapath/linux/compat/udp.c b/datapath/linux/compat/udp.c index f0362b6..4cd22fa 100644 --- a/datapath/linux/compat/udp.c +++ b/datapath/linux/compat/udp.c @@ -1,6 +1,6 @@ #include <linux/version.h> -#ifndef HAVE_UDP_SET_CSUM +#if LINUX_VERSION_CODE < KERNEL_VERSION(3,18,0) #include <net/udp.h> @@ -26,12 +26,13 @@ void rpl_udp_set_csum(bool nocheck, struct sk_buff *skb, skb->csum_offset = offsetof(struct udphdr, check); uh->check = ~udp_v4_check(len, saddr, daddr, 0); } else { + int l4_offset = skb_transport_offset(skb); __wsum csum; BUG_ON(skb->ip_summed == CHECKSUM_PARTIAL); uh->check = 0; - csum = skb_checksum(skb, 0, len, 0); + csum = skb_checksum(skb, l4_offset, len, 0); uh->check = udp_v4_check(len, saddr, daddr, csum); if (uh->check == 0) uh->check = CSUM_MANGLED_0; diff --git a/datapath/linux/compat/udp_tunnel.c b/datapath/linux/compat/udp_tunnel.c index 9265c8a..9cf7286 100644 --- a/datapath/linux/compat/udp_tunnel.c +++ b/datapath/linux/compat/udp_tunnel.c @@ -203,12 +203,13 @@ static void udp6_set_csum(bool nocheck, struct sk_buff *skb, skb->csum_offset = offsetof(struct udphdr, check); uh->check = ~udp_v6_check(len, saddr, daddr, 0); } else { + int l4_offset = skb_transport_offset(skb); __wsum csum; BUG_ON(skb->ip_summed == CHECKSUM_PARTIAL); uh->check = 0; - csum = skb_checksum(skb, 0, len, 0); + csum = skb_checksum(skb, l4_offset, len, 0); uh->check = udp_v6_check(len, saddr, daddr, csum); if (uh->check == 0) uh->check = CSUM_MANGLED_0;
In upstream linux kernel networking stack udp_set_csum() is called with only udp header applied but in case of compat layer it can be called with IP header. So following patch take the offset into account. Signed-off-by: Pravin B Shelar <pshelar@ovn.org> --- acinclude.m4 | 1 - datapath/linux/compat/include/net/udp.h | 2 +- datapath/linux/compat/udp.c | 5 +++-- datapath/linux/compat/udp_tunnel.c | 3 ++- 4 files changed, 6 insertions(+), 5 deletions(-)