[PATCH] IPv6: Refine IPv6 Address Validation Timer

[PATCH] IPv6: Refine IPv6 Address Validation Timer

[YOSHIFUJI Hideaki / 吉藤英明]

Hello!

Current IPv6 address validation timer is rough and
timing of address validation is not precise.
This patch refines timing of address validation timer.

Following patch is against linux-2.4.19.

Thank you in advance.

-------------------------------------------------------------------
Patch-Name: Refine IPv6 Address Validation Timer
Patch-Id: FIX_2_4_19_ADDRCONF_TIMER-20020905
Patch-Author: YOSHIFUJI Hideaki / USAGI Project <yoshfuji@linux-ipv6.org>
Credit: YOSHIFUJI Hideaki / USAGI Project <yoshfuji@linux-ipv6.org>
Reference: RFC2462
-------------------------------------------------------------------
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.3
diff -u -r1.1.1.1 -r1.1.1.1.4.3
--- net/ipv6/addrconf.c	2002/08/20 09:47:02	1.1.1.1
+++ net/ipv6/addrconf.c	2002/09/26 06:58:20	1.1.1.1.4.3
@@ -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:
@@ -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;
+			}
 
 			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 +1671,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 +2062,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 =

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

[David S. Miller]

   From: YOSHIFUJI Hideaki / 吉藤英明 <yoshfuji@linux-ipv6.org>
   Date: Fri, 27 Sep 2002 18:12:56 +0900 (JST)

This patch has problems.
    
   @@ -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.

Alexey will have to comment on the rest.

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

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20020927.022515.78074730.davem@redhat.com> (at Fri, 27 Sep 2002 02:25:15 -0700 (PDT)), "David S. Miller" <davem@redhat.com> 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 =

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

[kuznet]

Hello!

> So, is this reasonable?

Yes, I like this.

Let's only delete this:

> +	if (time_before(now + HZ/2, jiffies)) {
> +		ADBG((KERN_WARNING 
> +		      "addrconf_verify(): too slow; jiffies - now = %ld\n",
> +		      (long)jiffies - (long)now));
> +	}

I do not understand how this survived. If you worry about infinite
spinning in loop you could make this check real, f.e. breaking loop
when jiffies >= now+2. In this form it is just mud.

Alexey

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

[YOSHIFUJI Hideaki / 吉藤英明]

In article <200209271530.TAA21206@sex.inr.ac.ru> (at Fri, 27 Sep 2002 19:30:14 +0400 (MSD)), kuznet@ms2.inr.ac.ru says:

> Let's only delete this:
> 
> > +	if (time_before(now + HZ/2, jiffies)) {
> > +		ADBG((KERN_WARNING 
> > +		      "addrconf_verify(): too slow; jiffies - now = %ld\n",
> > +		      (long)jiffies - (long)now));
> > +	}
> 
> I do not understand how this survived. If you worry about infinite
> spinning in loop you could make this check real, f.e. breaking loop
> when jiffies >= now+2. In this form it is just mud.

hmm, I supposed it finally exited the loop (and then we got above warn).
Code around end of patch (*) prevent continuous run of this function.
If it is (almost) meaningless, just delete it.

(*) Oops, I slipped at (almost) end of the patch when making patch... sorry;
I inteded to refine timing into ~0.5 sec at worst; so, please change
	addr_chk_timer.expires = time_before(next, jiffies + HZ) ? jiffies + HZ : next;
to
	addr_chk_timer.expires = time_before(next, jiffies + HZ/2) ? jiffies + HZ/2 : next;
.

Thanks.


(do I need to resend complete patch?)

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

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

[kuznet]

Hello!

> (*) Oops, I slipped at (almost) end of the patch when making patch... sorry;
> I inteded to refine timing into ~0.5 sec at worst;

Does this matter? What is special about 0.5sec comparing to 1?

> (do I need to resend complete patch?)

No, I think. Deletion of bad debugging is easier to make after patching.

Alexey

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

[David S. Miller]

   From: kuznet@ms2.inr.ac.ru
   Date: Fri, 27 Sep 2002 20:39:45 +0400 (MSD)
   
   > (do I need to resend complete patch?)
   
   No, I think. Deletion of bad debugging is easier to make after patching.

I've applied the patch with the time_after() debugging check
removed to both 2.4.x and 2.5.x

If, after discussion, the "HZ --> HZ/2" change needs to be made, just
send another patch on top of previous one, thanks.