On Thu, Apr 28, 2016 at 5:35 PM, Joe Stringer <j...@ovn.org> wrote:
> 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:
To be honest, now that I understand what is going on, I think the
original version is probably clearer.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
