On 03/08/17(Thu) 09:13, Florian Obser wrote:
> 
> as mpi pointed out in "nd6 address expiration & NET_LOCK() contention"
> we run nd6_expire every second. That seems a bit silly considering
> that we normally have a pltime of a day.
> 
> With a bit of math we can work out when the timer should fire when we
> set pltime/vltime and when we walk the list in nd6_expire.
> 
> This intentionally only lowers the timeout in in6_update_ifa(). That
> means in the case of no privacy addresses and default pltime/vltime
> and no router advertisements getting lost the timer fires every week
> and notices that it has nothing to do. I think that is acceptable
> since it fires 600.000 times less...
> 
> Tests, comments, OKs?

I like it, comments below.

> diff --git sys/netinet6/in6.c sys/netinet6/in6.c
> index 45a28663d65..844781c0ff2 100644
> --- sys/netinet6/in6.c
> +++ sys/netinet6/in6.c
> @@ -497,6 +497,8 @@ in6_update_ifa(struct ifnet *ifp, struct in6_aliasreq 
> *ifra,
>       struct in6_addrlifetime *lt;
>       struct in6_multi_mship *imm;
>       struct rtentry *rt;
> +     time_t expire_time = -1;
> +     int secs;
>       char addr[INET6_ADDRSTRLEN];
>  
>       NET_ASSERT_LOCKED();
> @@ -686,6 +688,19 @@ in6_update_ifa(struct ifnet *ifp, struct in6_aliasreq 
> *ifra,
>        */
>       ia6->ia6_flags = ifra->ifra_flags;
>  
> +     nd6_next_expire_time(ia6, &expire_time);
> +
> +     if (expire_time != -1 && (nd6_expire_time == -1 || nd6_expire_time >
> +         expire_time)) {
> +             expire_time++; /* fire one second after expiry */
> +             timeout_del(&nd6_expire_timeout);
> +             secs = expire_time - time_uptime;
> +             if ( secs < 0)
> +                     secs = 0;
> +             timeout_add_sec(&nd6_expire_timeout, secs);
> +             nd6_expire_time = expire_time;
> +     }

I'd put the whole block in the function and call it nd6_expire_timer_update().

The whole logic needs to be protected by the KERNEL_LOCK() since it
touches a global variable, see below.

> diff --git sys/netinet6/nd6.c sys/netinet6/nd6.c
> index 08e0207eafa..585a062d4e3 100644
> --- sys/netinet6/nd6.c
> +++ sys/netinet6/nd6.c
> @@ -424,13 +439,12 @@ void
>  nd6_expire(void *unused)
>  {
>       struct ifnet *ifp;
> -     int s;
> +     time_t expire_time = -1;
> +     int s, secs;
>  
>       KERNEL_LOCK();
>       NET_LOCK(s);
>  
> -     timeout_add_sec(&nd6_expire_timeout, nd6_prune);
> -
>       TAILQ_FOREACH(ifp, &ifnet, if_list) {
>               struct ifaddr *ifa, *nifa;
>               struct in6_ifaddr *ia6;
> @@ -442,20 +456,27 @@ nd6_expire(void *unused)
>                       /* check address lifetime */
>                       if (IFA6_IS_INVALID(ia6)) {
>                               in6_purgeaddr(&ia6->ia_ifa);
> -                     } else if (IFA6_IS_DEPRECATED(ia6)) {
> -                             ia6->ia6_flags |= IN6_IFF_DEPRECATED;
>                       } else {
> -                             /*
> -                              * A new RA might have made a deprecated address
> -                              * preferred.
> -                              */
> -                             ia6->ia6_flags &= ~IN6_IFF_DEPRECATED;
> +                             if (IFA6_IS_DEPRECATED(ia6))
> +                                     ia6->ia6_flags |= IN6_IFF_DEPRECATED;
> +
> +                             nd6_next_expire_time(ia6, &expire_time);
>                       }
>               }
>       }
>  
>       NET_UNLOCK(s);
>       KERNEL_UNLOCK();
> +
> +     if (expire_time != -1) {
> +             expire_time++; /* fire one second after expiry */
> +             secs = expire_time - time_uptime;
> +             if ( secs < 0)
> +                     secs = 0;
> +             timeout_add_sec(&nd6_expire_timeout, secs);
> +     }
> +
> +     nd6_expire_time = expire_time;

This cannot be outside KERNEL_LOCK(), and it would simplify the code to
have it in the function call above.

> diff --git sys/netinet6/nd6.h sys/netinet6/nd6.h
> index 3b08e2c5dfe..f737219cd47 100644
> --- sys/netinet6/nd6.h
> +++ sys/netinet6/nd6.h
> @@ -138,7 +138,7 @@ struct    llinfo_nd6 {
>               (((MIN_RANDOM_FACTOR * (x >> 10)) + (arc4random() & \
>               ((MAX_RANDOM_FACTOR - MIN_RANDOM_FACTOR) * (x >> 10)))) /1000)
>  
> -extern int nd6_prune;
> +extern time_t nd6_expire_time;
>  extern int nd6_delay;
>  extern int nd6_umaxtries;
>  extern int nd6_mmaxtries;
> @@ -146,6 +146,8 @@ extern int nd6_maxnudhint;
>  extern int nd6_gctimer;
>  extern int nd6_debug;
>  
> +extern struct timeout nd6_expire_timeout;

This no longer need to be global if you move everything in a function.

> +
>  #define nd6log(x)    do { if (nd6_debug) log x; } while (0)
>  
>  union nd_opts {
> @@ -208,6 +210,7 @@ void nd6_rs_input(struct mbuf *, int, int);
>  int in6_ifdel(struct ifnet *, struct in6_addr *);
>  void rt6_flush(struct in6_addr *, struct ifnet *);
>  
> +void nd6_next_expire_time(struct in6_ifaddr *, time_t *);
>  #endif /* _KERNEL */
>  
>  #endif /* _NETINET6_ND6_H_ */
> 
> 
> -- 
> I'm not entirely sure you are real.
> 

Reply via email to