On 17 June 2016 at 18:20, Jarno Rajahalme <[email protected]> wrote: > >> On Jun 17, 2016, at 10:47 AM, Jesse Gross <[email protected] >> <mailto:[email protected]>> wrote: >> >> On Tue, Jun 14, 2016 at 3:25 PM, Jarno Rajahalme <[email protected] >> <mailto:[email protected]>> wrote: >>> This series adds the conntrack NAT integration upstreamed in Linux 4.6 >>> to the OVS tree kernel module. Main code is the same as upstream >>> net-next, backports are provided for Linux kernels 3.10 - 4.6. Code >>> compiles on each Linux version on this range, except for Linux 4.4 - >>> 4.6, which fail to compile due to reasons unrelated to NAT and/or >>> conntrack. >>> >>> The backports are tested on linux-stable versions 4.3 and 4.1, and >>> Ubuntu 14.04 with kernels 3.16.0-71-generic and 3.19.0-59-generic. >>> >>> While testing I observed kernel crashes from 'expiry' tests in >>> tests/system-traffic.at <http://system-traffic.at/>. I was able to >>> reproduce these crashes on OVS >>> master with various Linux kernel versions, and did not experience any >>> crashes when running only the NAT test cases with the backports in >>> this series. This tells me that the problem is not related to the NAT >>> backports. >>> >>> The patch that adds GCC 5 support for older kernels was used for >>> compile-only testing. >> >> I don't have any further comments on this series besides the ones I >> already responded to - the rest of the patches look like pretty >> straightforward backports. >> > > I'll do a v3 addressing your comments later today. > >> I diff'ed current net-next against OVS master plus this patch series >> for conntrack.c. Mostly it was the same but I noticed a few things >> that didn't appear to be related to backporting necessities. Are we >> missing some patches? This is what I got (I edited the diff to remove >> irrelevant stuff): >> > > I guess Joe could have an idea about these. I recall him mentioning that the > NF_INET_PRE_ROUTING change was a backport to avoid dependency on skb->dev in > some cases, but I don't know the detail. I guess this change could be done > upstream as well, but maybe it is not relevant for the current upstream > kernel. > > Jarno > >> --- /home/jgross/openvswitch/datapath/conntrack.c 2016-06-17 >> 10:25:09.967245333 -0700 >> +++ net/openvswitch/conntrack.c 2016-05-18 12:18:50.597639085 -0700 >> @@ -357,14 +351,9 @@ >> static int handle_fragments(struct net *net, struct sw_flow_key *key, >> u16 zone, struct sk_buff *skb) >> [...] >> - if (!skb->dev) { >> - OVS_NLERR(true, "%s: skb has no dev; dropping", __func__); >> - return -EINVAL; >> - } >> - >> >> @@ -745,7 +734,7 @@ >> /* Repeat if requested, see nf_iterate(). */ >> do { >> err = nf_conntrack_in(net, info->family, >> - NF_INET_FORWARD, skb); >> + NF_INET_PRE_ROUTING, skb); >> } while (err == NF_REPEAT); >> >> if (err != NF_ACCEPT) >> @@ -1353,7 +1342,7 @@ >> if (ct_info->helper) >> module_put(ct_info->helper->me); >> if (ct_info->ct) >> - nf_ct_tmpl_free(ct_info->ct); >> + nf_ct_put(ct_info->ct); >> }
I think these patch fragments were accumulated into the backport during testing and I ended up merging them without separately addressing them upstream. The first should be dropped; the second fixes an issue with older kernels but the issue doesn't crop up on newer kernels; third I think is just cosmetic. The latter two should probably be upstreamed. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
