Re: [PATCH 11/11] Reduce timer updates in __nf_ct_refresh_acct()

[Patrick McHardy <kaber@trash.net>]

Martin Josefsson wrote:
> Only update the conntrack timer if there's been at least HZ jiffies since the
> last update. Reduces the number of del_timer/add_timer cycles from one per
> packet to one per connection per second (plus once for each state change of a
> connection)
> Should handle timer wraparounds and connection timeout changes.
> 
> Signed-off-by: Martin Josefsson <gandalf@wlug.westbo.se>
> 
> ---
>  net/netfilter/nf_conntrack_core.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6.19-rc3-git4.quilt/net/netfilter/nf_conntrack_core.c
> ===================================================================
> --- linux-2.6.19-rc3-git4.quilt.orig/net/netfilter/nf_conntrack_core.c	2006-11-01 21:40:13.000000000 +0100
> +++ linux-2.6.19-rc3-git4.quilt/net/netfilter/nf_conntrack_core.c	2006-11-01 21:40:21.000000000 +0100
> @@ -865,9 +865,14 @@ void __nf_ct_refresh_acct(struct nf_conn
>  		ct->timeout.expires = extra_jiffies;
>  		event = IPCT_REFRESH;
>  	} else {
> -		/* Need del_timer for race avoidance (may already be dying). */
> -		if (del_timer(&ct->timeout)) {
> -			ct->timeout.expires = jiffies + extra_jiffies;
> +		unsigned long newtime = jiffies + extra_jiffies;
> +
> +		/* Only update the timeout if the new timeout is at least
> +		   HZ jiffies from the old timeout. Need del_timer for race
> +		   avoidance (may already be dying). */
> +		if (newtime - ct->timeout.expires >= HZ
> +		    && del_timer(&ct->timeout)) {
> +			ct->timeout.expires = newtime;
>  			add_timer(&ct->timeout);
>  			event = IPCT_REFRESH;
>  		}

Applied, thanks. BTW, the "race avoidance" strikes me as racy,
there are multiple locations where we simply do

if (del_timer(...))
   ct->timeout.function(...)

and expect the conntrack to be either destroyed by the
ct->timeout.function call or by the expiring timer.
But without taking ip_conntrack_lock we could have:

CPU1 (refresh)			CPU2

if (del_timer) [success]
				if (del_timer) [no success]
	add_timer()

which means the conntrack won't be destroyed. Did I miss
something?