RFC: conntrack: use mod_timer_pending()

RFC: conntrack: use mod_timer_pending()

[Patrick McHardy]

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

This patch changes conntrack to use the new mod_timer_pending()
function, which only rearms the timer if it was still active.
This allows to only grab the lock in __nf_ct_refresh_acct when
accounting is used.

Anyone seing anything wrong with this?

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1349 bytes --]

diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index edf9569..2737f5d 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -807,8 +807,6 @@ void __nf_ct_refresh_acct(struct nf_conn *ct,
 	NF_CT_ASSERT(ct->timeout.data == (unsigned long)ct);
 	NF_CT_ASSERT(skb);
 
-	spin_lock_bh(&nf_conntrack_lock);
-
 	/* Only update if this is not a fixed timeout */
 	if (test_bit(IPS_FIXED_TIMEOUT_BIT, &ct->status))
 		goto acct;
@@ -822,11 +820,8 @@ void __nf_ct_refresh_acct(struct nf_conn *ct,
 		/* 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);
-		}
+		if (newtime - ct->timeout.expires >= HZ)
+			mod_timer_pending(&ct->timeout, newtime);
 	}
 
 acct:
@@ -835,13 +830,13 @@ acct:
 
 		acct = nf_conn_acct_find(ct);
 		if (acct) {
+			spin_lock_bh(&nf_conntrack_lock);
 			acct[CTINFO2DIR(ctinfo)].packets++;
 			acct[CTINFO2DIR(ctinfo)].bytes +=
 				skb->len - skb_network_offset(skb);
+			spin_unlock_bh(&nf_conntrack_lock);
 		}
 	}
-
-	spin_unlock_bh(&nf_conntrack_lock);
 }
 EXPORT_SYMBOL_GPL(__nf_ct_refresh_acct);
 

Re: RFC: conntrack: use mod_timer_pending()

[Patrick McHardy]

Patrick McHardy wrote:
> This patch changes conntrack to use the new mod_timer_pending()
> function, which only rearms the timer if it was still active.
> This allows to only grab the lock in __nf_ct_refresh_acct when
> accounting is used.
> 
> Anyone seing anything wrong with this?

>  		if (acct) {
> +			spin_lock_bh(&nf_conntrack_lock);
>  			acct[CTINFO2DIR(ctinfo)].packets++;
>  			acct[CTINFO2DIR(ctinfo)].bytes +=
>  				skb->len - skb_network_offset(skb);
> +			spin_unlock_bh(&nf_conntrack_lock);
>  		}

In fact, we could additionally change it to use the per-conntrack lock.

Re: RFC: conntrack: use mod_timer_pending()

[Stephen Hemminger]

On Wed, 10 Jun 2009 15:07:02 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Patrick McHardy wrote:
> > This patch changes conntrack to use the new mod_timer_pending()
> > function, which only rearms the timer if it was still active.
> > This allows to only grab the lock in __nf_ct_refresh_acct when
> > accounting is used.
> > 
> > Anyone seing anything wrong with this?
> 
> >  		if (acct) {
> > +			spin_lock_bh(&nf_conntrack_lock);
> >  			acct[CTINFO2DIR(ctinfo)].packets++;
> >  			acct[CTINFO2DIR(ctinfo)].bytes +=
> >  				skb->len - skb_network_offset(skb);
> > +			spin_unlock_bh(&nf_conntrack_lock);
> >  		}
> 
> In fact, we could additionally change it to use the per-conntrack lock.

That is what the conntrack RCU patch did.

Re: RFC: conntrack: use mod_timer_pending()

[Patrick McHardy]

Stephen Hemminger wrote:
> On Wed, 10 Jun 2009 15:07:02 +0200
> Patrick McHardy <kaber@trash.net> wrote:
> 
>> Patrick McHardy wrote:
>>> This patch changes conntrack to use the new mod_timer_pending()
>>> function, which only rearms the timer if it was still active.
>>> This allows to only grab the lock in __nf_ct_refresh_acct when
>>> accounting is used.
>>>
>>> Anyone seing anything wrong with this?
>>>  		if (acct) {
>>> +			spin_lock_bh(&nf_conntrack_lock);
>>>  			acct[CTINFO2DIR(ctinfo)].packets++;
>>>  			acct[CTINFO2DIR(ctinfo)].bytes +=
>>>  				skb->len - skb_network_offset(skb);
>>> +			spin_unlock_bh(&nf_conntrack_lock);
>>>  		}
>> In fact, we could additionally change it to use the per-conntrack lock.
> 
> That is what the conntrack RCU patch did.

Could you remind me which one exactly? There have been a lot of
patches passed around :)

Re: RFC: conntrack: use mod_timer_pending()

[Stephen Hemminger]

On Wed, 10 Jun 2009 16:53:14 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Stephen Hemminger wrote:
> > On Wed, 10 Jun 2009 15:07:02 +0200
> > Patrick McHardy <kaber@trash.net> wrote:
> > 
> >> Patrick McHardy wrote:
> >>> This patch changes conntrack to use the new mod_timer_pending()
> >>> function, which only rearms the timer if it was still active.
> >>> This allows to only grab the lock in __nf_ct_refresh_acct when
> >>> accounting is used.
> >>>
> >>> Anyone seing anything wrong with this?
> >>>  		if (acct) {
> >>> +			spin_lock_bh(&nf_conntrack_lock);
> >>>  			acct[CTINFO2DIR(ctinfo)].packets++;
> >>>  			acct[CTINFO2DIR(ctinfo)].bytes +=
> >>>  				skb->len - skb_network_offset(skb);
> >>> +			spin_unlock_bh(&nf_conntrack_lock);
> >>>  		}
> >> In fact, we could additionally change it to use the per-conntrack lock.
> > 
> > That is what the conntrack RCU patch did.
> 
> Could you remind me which one exactly? There have been a lot of
> patches passed around :)
> 
> 

Sorry, got confused, the patch I was thinking of was for the
TCP conntrack lock.

-- 

Re: RFC: conntrack: use mod_timer_pending()

[Martin Josefsson]

On Wed, 10 Jun 2009, Patrick McHardy wrote:

> This patch changes conntrack to use the new mod_timer_pending()
> function, which only rearms the timer if it was still active.
> This allows to only grab the lock in __nf_ct_refresh_acct when
> accounting is used.
>
> Anyone seing anything wrong with this?

I don't see any problem with it.
Looks good, thanks.

/Martin