On Thu, Apr 28, 2016 at 5:17 PM, 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().
OK, thanks for the explanation. This is intricate but it makes sense now. >> More minor but it also seems odd that rpl_nf_ct_frag6_gather() is >> declared in the first #ifdef but then actually defined under >> OVS_NF_DEFRAG6_BACKPORT like all of the other functions. What's the >> reason for separating it out this way? > > This patch shifts the #define directly below the declaration.. I think > this patch fixes what you're describing here? My comment was more related to the fact that the declaration for nf_ct_frag6_gather() was in the first check (where OVS_NF_DEFRAG6_BACKPORT is #defined) but it's actual definition is under #ifdef OVS_NF_DEFRAG6_BACKPORT. I realized that this is minor and can't cause a problem in practice but it made me wonder why the declarations were separated this way. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev