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

[Patrick McHardy <kaber@trash.net>]

Martin Josefsson wrote:
> On Fri, 3 Nov 2006, Patrick McHardy wrote:
> 
>>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?
> 
> 
> You are absolutely correct. I discussed this with Rusty some time ago and
> he thought it was fine since we mostly aim for "best effort", but I don't
> like it either. I have a patch that adds another variant of mod_timer()
> that doesn't activate an inactive timer, and use this instead of
> del_timer()/add_timer(). This avoids this race and reduces the number of
> locks taken. I'll send this patch to you after this weekend I think.


That sounds like a good way to fix it. I think I saw that patch on the
link you sent me some time ago, IIRC it touches core kernel code, so
you should probably send the timer part seperately to lkml.