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 <[email protected]>
Credit: YOSHIFUJI Hideaki / USAGI Project <[email protected]>
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 =
From: YOSHIFUJI Hideaki / 吉藤英明 <[email protected]>
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.
In article <[email protected]> (at Fri, 27 Sep 2002 02:25:15 -0700 (PDT)), "David S. Miller" <[email protected]> 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 =
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
In article <[email protected]> (at Fri, 27 Sep 2002 19:30:14 +0400 (MSD)), [email protected] 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 <[email protected]>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF 80D8 4807 F894 E062 0EEA
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
From: [email protected]
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.