On 28 April 2016 at 17:17, Joe Stringer <j...@ovn.org> wrote:
> On 26 April 2016 at 19:44, Jesse Gross <je...@kernel.org> wrote:
>> On Thu, Apr 21, 2016 at 2:07 PM, Joe Stringer <j...@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:
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 */
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
