[PATCH/RFC 1/2] optimization updating timer in connection tracking

[PATCH/RFC 1/2] optimization updating timer in connection tracking

[Pablo Neira]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

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

[-- Attachment #2: conntrack-update-timer-optimize.patch --]
[-- Type: text/x-patch, Size: 941 bytes --]

diff -u -r1.1.1.2 ip_conntrack_core.c
--- a/net/ipv4/netfilter/ip_conntrack_core.c	4 Sep 2004 17:33:46 -0000	1.1.1.2
+++ b/net/ipv4/netfilter/ip_conntrack_core.c	18 Sep 2004 18:59:07 -0000
@@ -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);
 	}
 }
 

Re: [PATCH/RFC 1/2] optimization updating timer in connection tracking

[Patrick McHardy]

Pablo Neira wrote:

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

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

Re: [PATCH/RFC 1/2] optimization updating timer in connection tracking

[Pablo Neira]

Patrick McHardy escribió:

> 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();


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

Re: [PATCH/RFC 1/2] optimization updating timer in connection tracking

[Patrick McHardy]

Pablo Neira wrote:

> Patrick McHardy escribió:
>
>> 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();
>
>
>
> 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?


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

--- linux-2.4.19-pre9/net/ipv4/netfilter/ip_conntrack_core.c.orig       
Mon Jun  3 20:32:28 2002
+++ linux-2.4.19-pre9/net/ipv4/netfilter/ip_conntrack_core.c    Mon Jun  
3 20:48:13 2002
@@ -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