Re: [PATCH] Very first try: ipt_connrate patch.

[Nuutti Kotivuori <naked@iki.fi>]

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