Re: [PATCH] IPv6: Refine IPv6 Address Validation Timer

[YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@xxxxxxxxxxxxxx>]

In article <20020927.022515.78074730.davem@xxxxxxxxxx> (at Fri, 27 Sep 2002 
02:25:15 -0700 (PDT)), "David S. Miller" <davem@xxxxxxxxxx> says:

>    @@ -1626,24 +1635,32 @@
>               for (ifp=inet6_addr_lst[i]; ifp; ifp=ifp->lst_next) {
>                       unsigned long age;
>     
>    -                  if (ifp->flags & IFA_F_PERMANENT)
>    +                  spin_lock(&ifp->lock);
>    +                  if (ifp->flags & IFA_F_PERMANENT) {
>    +                          spin_unlock(&ifp->lock);
>                               continue;
>    +                  }
> 
> This is completely unnecessary.  Nobody modifies the
> IFA_F_PERMANENT flag after the addr entry has been added
> to the hash table and this runs under the addrconf hash
> lock already.

Thanks for comment.
So, is this reasonable?

Index: net/ipv6/addrconf.c
===================================================================
RCS file: /cvsroot/usagi/usagi-backport/linux24/net/ipv6/addrconf.c,v
retrieving revision 1.1.1.1
retrieving revision 1.1.1.1.4.4
diff -u -r1.1.1.1 -r1.1.1.1.4.4
--- net/ipv6/addrconf.c 2002/08/20 09:47:02     1.1.1.1
+++ net/ipv6/addrconf.c 2002/09/27 15:02:57     1.1.1.1.4.4
@@ -26,6 +26,8 @@
  *                                             packets.
  *     yoshfuji@USAGI                  :       Fixed interval between DAD
  *                                             packets.
+ *     YOSHIFUJI Hideaki @USAGI        :       improved accuracy of
+ *                                             address validation timer.
  */
 
 #include <linux/config.h>
@@ -93,6 +95,7 @@
 void addrconf_verify(unsigned long);
 
 static struct timer_list addr_chk_timer = { function: addrconf_verify };
+static spinlock_t addrconf_verify_lock = SPIN_LOCK_UNLOCKED;
 
 static int addrconf_ifdown(struct net_device *dev, int how);
 
@@ -1616,9 +1619,15 @@
 void addrconf_verify(unsigned long foo)
 {
        struct inet6_ifaddr *ifp;
-       unsigned long now = jiffies;
+       unsigned long now, next;
        int i;
 
+       spin_lock_bh(&addrconf_verify_lock);
+       now = jiffies;
+       next = now + ADDR_CHECK_FREQUENCY;
+
+       del_timer(&addr_chk_timer);
+
        for (i=0; i < IN6_ADDR_HSIZE; i++) {
 
 restart:
@@ -1629,21 +1638,27 @@
                        if (ifp->flags & IFA_F_PERMANENT)
                                continue;
 
+                       spin_lock(&ifp->lock);
                        age = (now - ifp->tstamp) / HZ;
 
-                       if (age > ifp->valid_lft) {
+                       if (age >= ifp->valid_lft) {
+                               spin_unlock(&ifp->lock);
                                in6_ifa_hold(ifp);
                                write_unlock(&addrconf_hash_lock);
                                ipv6_del_addr(ifp);
                                goto restart;
-                       } else if (age > ifp->prefered_lft) {
+                       } else if (age >= ifp->prefered_lft) {
+                               /* jiffies - ifp->tsamp > age >= 
ifp->prefered_lft */
                                int deprecate = 0;
 
-                               spin_lock(&ifp->lock);
                                if (!(ifp->flags&IFA_F_DEPRECATED)) {
                                        deprecate = 1;
                                        ifp->flags |= IFA_F_DEPRECATED;
                                }
+
+                               if (time_before(ifp->tstamp + ifp->valid_lft * 
HZ, next))
+                                       next = ifp->tstamp + ifp->valid_lft * 
HZ;
+
                                spin_unlock(&ifp->lock);
 
                                if (deprecate) {
@@ -1654,12 +1669,24 @@
                                        in6_ifa_put(ifp);
                                        goto restart;
                                }
+                       } else {
+                               /* ifp->prefered_lft <= ifp->valid_lft */
+                               if (time_before(ifp->tstamp + ifp->prefered_lft 
* HZ, next))
+                                       next = ifp->tstamp + ifp->prefered_lft 
* HZ;
+                               spin_unlock(&ifp->lock);
                        }
                }
                write_unlock(&addrconf_hash_lock);
        }
 
-       mod_timer(&addr_chk_timer, jiffies + ADDR_CHECK_FREQUENCY);
+       if (time_before(now + HZ/2, jiffies)) {
+               ADBG((KERN_WARNING 
+                     "addrconf_verify(): too slow; jiffies - now = %ld\n",
+                     (long)jiffies - (long)now));
+       }
+       addr_chk_timer.expires = time_before(next, jiffies + HZ) ? jiffies + HZ 
: next;
+       add_timer(&addr_chk_timer);
+       spin_unlock_bh(&addrconf_verify_lock);
 }
 
 static int
@@ -2033,8 +2060,7 @@
        proc_net_create("if_inet6", 0, iface_proc_info);
 #endif
        
-       addr_chk_timer.expires = jiffies + ADDR_CHECK_FREQUENCY;
-       add_timer(&addr_chk_timer);
+       addrconf_verify(0);
        rtnetlink_links[PF_INET6] = inet6_rtnetlink_table;
 #ifdef CONFIG_SYSCTL
        addrconf_sysctl.sysctl_header =