| Message ID | CAPWQB7HiDLv80o3v4Eob=jdRWg4m+0Ti32HA5mrV608UE8h7pA@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, Apr 28, 2016 at 5:35 PM, Joe Stringer <joe@ovn.org> wrote: > On 28 April 2016 at 17:17, Joe Stringer <joe@ovn.org> wrote: >> On 26 April 2016 at 19:44, Jesse Gross <jesse@kernel.org> wrote: >>> On Thu, Apr 21, 2016 at 2:07 PM, Joe Stringer <joe@ovn.org> wrote: >>>> diff --git a/datapath/linux/compat/include/net/netfilter/ipv6/nf_defrag_ipv6.h b/datapath/linux/compat/include/net/netfilter/ipv6/nf_defrag_ipv6.h >>>> index fe99ced37227..a3b86dab2c9c 100644 >>>> --- a/datapath/linux/compat/include/net/netfilter/ipv6/nf_defrag_ipv6.h >>>> +++ b/datapath/linux/compat/include/net/netfilter/ipv6/nf_defrag_ipv6.h >>>> @@ -16,17 +16,17 @@ >>>> #define OVS_NF_DEFRAG6_BACKPORT 1 >>>> struct sk_buff *rpl_nf_ct_frag6_gather(struct net *net, struct sk_buff *skb, >>>> u32 user); >>>> +#define nf_ct_frag6_gather rpl_nf_ct_frag6_gather >>>> +#endif /* HAVE_NF_CT_FRAG6_CONSUME_ORIG */ >>>> + >>>> +#ifdef OVS_NF_DEFRAG6_BACKPORT >>>> int __init rpl_nf_ct_frag6_init(void); >>>> void rpl_nf_ct_frag6_cleanup(void); >>>> -void rpl_nf_ct_frag6_consume_orig(struct sk_buff *skb); >>>> -#define nf_ct_frag6_gather rpl_nf_ct_frag6_gather >>>> -#else /* HAVE_NF_CT_FRAG6_CONSUME_ORIG */ >>>> +#else /* !OVS_NF_DEFRAG6_BACKPORT */ >>>> static inline int __init rpl_nf_ct_frag6_init(void) { return 0; } >>>> static inline void rpl_nf_ct_frag6_cleanup(void) { } >>>> -static inline void rpl_nf_ct_frag6_consume_orig(struct sk_buff *skb) { } >>>> -#endif /* HAVE_NF_CT_FRAG6_CONSUME_ORIG */ >>>> +#endif /* OVS_NF_DEFRAG6_BACKPORT */ >>>> #define nf_ct_frag6_init rpl_nf_ct_frag6_init >>>> #define nf_ct_frag6_cleanup rpl_nf_ct_frag6_cleanup >>>> -#define nf_ct_frag6_consume_orig rpl_nf_ct_frag6_consume_orig >>> >>> I'm not sure that I totally understand what is going on in this file. >>> In particular, although not directly related to this patch, I'm not >>> sure why we define rpl_nf_ct_frag6_init()/cleanup() to be no-ops in >>> cases where we don't have OVS_NF_DEFRAG6_BACKPORT. On new kernels, >>> shouldn't we be using the upstream definitions of these functions? The >>> values initialized seem to be used by functions which aren't >>> backported. >> >> In upstream, we depend on the exported symbol nf_ct_frag6_gather() >> which ensures that nf_defrag_ipv6 module is loaded when loading >> openvswitch. During nf_defrag_ipv6 module load, in nf_defrag_init() it >> will invoke nf_ct_frag6_init(), initializing the IPv6 fragmentation >> handlers and cache. >> >> In out-of-tree, depending on the upstream kernel, we may be able to >> rely on the same behaviour from upstream, however if >> nf_ct_frag6_gather() is not available then we will backport ipv6 >> defrag so we need to do this initialization ourselves. In >> datapath/compat.h, compat_init() is invoked during openvswitch module >> load which invokes nf_ct_frag6_init() to perform this initialization >> if necessary. >> >> When depending on a kernel which exports nf_ct_frag6_gather(), we >> don't need to do any initialization, so we define this function as a >> no-op and rely on the upstream ipv6_defrag. >> >> When nf_ct_frag6_gather() is not exported, we are unable to depend on >> the upstream module so we rely on our own backport. In this case, we >> provide the implementation of nf_ct_frag6_init(). > > Perhaps the following incremental would simplify this logic: To be honest, now that I understand what is going on, I think the original version is probably clearer.
diff --git a/datapath/compat.h b/datapath/compat.h index 816f754c64e2..6aadded64ab4 100644 --- a/datapath/compat.h +++ b/datapath/compat.h @@ -51,9 +51,11 @@ static inline int __init compat_init(void) if (err) return err; +#ifdef OVS_NF_DEFRAG6_BACKPORT err = nf_ct_frag6_init(); if (err) goto error_ipfrag_exit; +#endif err = ip6_output_init(); if (err) diff --git a/datapath/linux/compat/include/net/netfilter/ipv6/nf_defrag_ipv6.h b/datapath/linux/compat/include/net/netfilter/ipv6/nf_defrag_ipv6.h index dc440db99924..7f596903de5b 100644 --- a/datapath/linux/compat/include/net/netfilter/ipv6/nf_defrag_ipv6.h +++ b/datapath/linux/compat/include/net/netfilter/ipv6/nf_defrag_ipv6.h @@ -16,16 +16,10 @@ #define OVS_NF_DEFRAG6_BACKPORT 1 int rpl_nf_ct_frag6_gather(struct net *net, struct sk_buff *skb, u32 user); #define nf_ct_frag6_gather rpl_nf_ct_frag6_gather -#endif /* HAVE_NF_CT_FRAG6_CONSUME_ORIG */ - -#ifdef OVS_NF_DEFRAG6_BACKPORT int __init rpl_nf_ct_frag6_init(void); -void rpl_nf_ct_frag6_cleanup(void); -#else /* !OVS_NF_DEFRAG6_BACKPORT */ -static inline int __init rpl_nf_ct_frag6_init(void) { return 0; } -static inline void rpl_nf_ct_frag6_cleanup(void) { } -#endif /* OVS_NF_DEFRAG6_BACKPORT */ #define nf_ct_frag6_init rpl_nf_ct_frag6_init +void rpl_nf_ct_frag6_cleanup(void); #define nf_ct_frag6_cleanup rpl_nf_ct_frag6_cleanup +#endif /* OVS_NF_DEFRAG6_BACKPORT */ #endif /* __NF_DEFRAG_IPV6_WRAPPER_H */