From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nuutti Kotivuori Subject: Re: [PATCH] Very first try: ipt_connrate patch. Date: Mon, 16 Feb 2004 14:34:41 +0200 Sender: netfilter-devel-admin@lists.netfilter.org Message-ID: <878yj39p9a.fsf@iki.fi> References: <87bro9fiqq.fsf@iki.fi> <4030A6F8.5030606@trash.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: To: netfilter-devel@lists.netfilter.org Errors-To: netfilter-devel-admin@lists.netfilter.org List-Help: List-Post: List-Subscribe: , List-Unsubscribe: , List-Archive: List-Id: netfilter-devel.vger.kernel.org Patrick McHardy wrote: > Nuutti Kotivuori wrote: >> +void >> +ip_conntrack_rate_count(struct ip_conntrack_rate *ctr, >> + unsigned int len) [...] > > You need locking here. Other CPUs can change ctr->avgrate between > reading and updating it. The xchg operation becomes unneccessary > with proper locking. Right. I didn't see any locking in resolve_normal_ct, so I assumed no locking was needed. Then I peeked in the functions it calls and every one of them gets the ip_conntrack_lock. But, I am a bit confused on the best way to handle locking. For example, ip_conntrack_proto_tcp has only one RWLOCK which it uses. I can undestand this for the actual connection tracking table, but for the subentries in it... well, why would writing to one conntrack element block reading from another conntrack element - or simultaneous writing to two different conntrack elements and so on. Shouldn't there be some locking per connection tracking entry, so other entries could simultaneously be processed on other processors? SMP performance and correctness is an important issue for me, as I am specifically running this on an SMP machine. Luckily I haven't ran into any problems yet - thanks for spotting this. I shall implement things as proto_tcp does it for now. -- Naked