Re: [PATCH net-next v2 3/4] ipv6: use net->rt_genid to check dst validity

[Vlad Yasevich <vyasevich@gmail.com>]

On 09/10/2012 10:34 AM, Nicolas Dichtel wrote:
> Le 10/09/2012 16:29, Vlad Yasevich a écrit :
>> On 09/10/2012 09:22 AM, Nicolas Dichtel wrote:
>>> IPv6 dst should take care of rt_genid too. When a xfrm policy is
>>> inserted or
>>> deleted, all dst should be invalidated.
>>> To force the validation, dst entries should be created with
>>> ->obsolete set to
>>> DST_OBSOLETE_FORCE_CHK. This was already the case for all functions
>>> calling
>>> ip6_dst_alloc(), except for ip6_rt_copy().
>>>
>>> As a consequence, we can remove the specific code in
>>> inet6_connection_sock.
>>>
>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>>> ---
>>>   include/net/ip6_fib.h            |  2 +-
>>>   net/ipv6/inet6_connection_sock.c | 23 +----------------------
>>>   net/ipv6/route.c                 | 17 +++++++++++++----
>>>   3 files changed, 15 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
>>> index cd64cf3..5eb93f4 100644
>>> --- a/include/net/ip6_fib.h
>>> +++ b/include/net/ip6_fib.h
>>> @@ -113,7 +113,7 @@ struct rt6_info {
>>>       unsigned long            _rt6i_peer;
>>>
>>>   #ifdef CONFIG_XFRM
>>> -    u32                rt6i_flow_cache_genid;
>>> +    u32                rt6i_genid;
>>>   #endif
>>>       /* more non-fragment space at head required */
>>>       unsigned short            rt6i_nfheader_len;
>>> diff --git a/net/ipv6/inet6_connection_sock.c
>>> b/net/ipv6/inet6_connection_sock.c
>>> index 0251a60..c4f9341 100644
>>> --- a/net/ipv6/inet6_connection_sock.c
>>> +++ b/net/ipv6/inet6_connection_sock.c
>>> @@ -175,33 +175,12 @@ void __inet6_csk_dst_store(struct sock *sk, struct
>>> dst_entry *dst,
>>>                  const struct in6_addr *saddr)
>>>   {
>>>       __ip6_dst_store(sk, dst, daddr, saddr);
>>> -
>>> -#ifdef CONFIG_XFRM
>>> -    {
>>> -        struct rt6_info *rt = (struct rt6_info  *)dst;
>>> -        rt->rt6i_flow_cache_genid = atomic_read(&flow_cache_genid);
>>> -    }
>>> -#endif
>>>   }
>>>
>>>   static inline
>>>   struct dst_entry *__inet6_csk_dst_check(struct sock *sk, u32 cookie)
>>>   {
>>> -    struct dst_entry *dst;
>>> -
>>> -    dst = __sk_dst_check(sk, cookie);
>>> -
>>> -#ifdef CONFIG_XFRM
>>> -    if (dst) {
>>> -        struct rt6_info *rt = (struct rt6_info *)dst;
>>> -        if (rt->rt6i_flow_cache_genid !=
>>> atomic_read(&flow_cache_genid)) {
>>> -            __sk_dst_reset(sk);
>>> -            dst = NULL;
>>> -        }
>>> -    }
>>> -#endif
>>> -
>>> -    return dst;
>>> +    return __sk_dst_check(sk, cookie);
>>>   }
>>>
>>>   static struct dst_entry *inet6_csk_route_socket(struct sock *sk,
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index 339d921..db7b78f 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -281,13 +281,16 @@ static inline struct rt6_info
>>> *ip6_dst_alloc(struct net
>>> *net,
>>>                            struct fib6_table *table)
>>>   {
>>>       struct rt6_info *rt = dst_alloc(&net->ipv6.ip6_dst_ops, dev,
>>> -                    0, DST_OBSOLETE_NONE, flags);
>>> +                    0, DST_OBSOLETE_FORCE_CHK, flags);
>>>
>>>       if (rt) {
>>>           struct dst_entry *dst = &rt->dst;
>>>
>>>           memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst));
>>>           rt6_init_peer(rt, table ? &table->tb6_peers :
>>> net->ipv6.peers);
>>> +#ifdef CONFIG_XFRM
>>> +        rt->rt6i_genid = rt_genid(net);
>>> +#endif
>>
>> This isn't XFRM dependent any more, is it?
> Not dependent, but for IPv6, it's only usefull when xfrm is set. Goal of
> this ifdef was to avoid the test if xfrm is not used.

It's not the usage,  it's enable at build time and that's almost always 
on.  Now the cache behavior is different when XFRM is excluded from the 
kernel build.

Before the ifdef was needed since you were actually looking at xfrm 
variable.  Not anymore.   The ifdef doesn't make sense.

-vlad