optimization updating timer in connection tracking

Hi Patrick,

I think that I found something interesting reading kernel/timer.c some weeks ago, well I would like to know about this idea. The comment above mod_timer speaks by itself...

* mod_timer(timer, expires) is equivalent to:
*
* del_timer(timer); timer->expires = expires; add_timer(timer);
*
* Note that if there are multiple unserialized concurrent users of the
* same timer, then mod_timer() is the only safe way to modify the timeout,
* since add_timer() cannot modify an already running timer.
*
* The function returns whether it has modified a pending timer or not.
* (ie. mod_timer() of an inactive timer returns 0, mod_timer() of an
* active timer returns 1.)

so I think that you could use mod_timer in 2.6 and remove that WRITE_LOCK when updating the timer, because it's there to serialize timer updates and now mod_timer controls this possible situation. I don't have a smp machine here but this week I'll look for a platform to test it.

If I'm missing something, please let me know.

regards,
Pablo

diff -u -r1.1.1.2 ip_conntrack_core.c
@@ -1162,14 +1162,13 @@
 		ct->timeout.expires = extra_jiffies;
 		ct_add_counters(ct, ctinfo, skb);
 	} else {
-		WRITE_LOCK(&ip_conntrack_lock);
-		/* Need del_timer for race avoidance (may already be dying). */
-		if (del_timer(&ct->timeout)) {
-			ct->timeout.expires = jiffies + extra_jiffies;
-			add_timer(&ct->timeout);
-		}
+		/* In 2.6 you can use mod_timer to add some extra jiffies
+		 * to an active timer, see kernel/timer.c. Even better, we
+		 * don't need to lock the conntrack table because mod_timer
+		 * takes care about a possible concurrent update, this
+		 * kills a pretty bottleneck ;-) */
+		mod_timer(&ct->timeout, jiffies + extra_jiffies);
 		ct_add_counters(ct, ctinfo, skb);
-		WRITE_UNLOCK(&ip_conntrack_lock);
 	}
 }
 

We can't use mod_timer because it will re-add a timer that already went off and dropped it's reference count to the conntrack. mod_timer is equivalent to del_timer(); add_timer(); but we do del_timer() && add_timer();

Regards
Patrick

thanks for the hint, in that case, is it worthy adding something like

----- from kernel/timer.c ----
261 /*

262          * This is a common optimization triggered by the
263          * networking code - if the timer is re-modified
264          * to be the same thing then just return:
265          */

266 if (timer->expires == expires && timer_pending(timer))
267 return 1;
----- end -------

inside the write_lock section?

regards,
Pablo

We already have something better in old pom:
netfilter/patch-o-matic/optimizations/ip_ct_refresh_optimization.patch:

@@ -1091,8 +1091,10 @@
       if (!is_confirmed(ct))
               ct->timeout.expires = extra_jiffies;
       else {
-               /* Need del_timer for race avoidance (may already be dying). */
-               if (del_timer(&ct->timeout)) {
+               /* Don't update timer for each packet, only if it's been >HZ
+                * ticks since last update.
+                * Need del_timer for race avoidance (may already be dying). */
+               if (abs(jiffies + extra_jiffies - ct->timeout.expires) >= HZ && del_timer(&ct->timeout)) {
                       ct->timeout.expires = jiffies + extra_jiffies;
                       add_timer(&ct->timeout);
               }

It is also included in the ctnetlink patch to avoid excessive timeout notifications.

Regards
Patrick