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.
>
