Re: [PATCH 2/2] netfilter: conntrack: optional reliable conntrack event delivery

[Patrick McHardy <kaber@trash.net>]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
>> This is a first attempt to replace some global locks by private
>> per conntrack locks. On 64 bit, it fits into a hole and doesn't
>> enlarge struct nf_conn.
>>
>> Wrt. to the event cache, we certainly don't want to take and release
>> the lock for every event. I was thinking about something like this:
>>
>> - add a new member to the event structure to hold undelivered events
>>   (named "missed" below)
>> - cache events in the existing member as you're doing currently
>> - on delivery, do something like this:
>>
>>     events = xchg(&e->cache, 0);
>>     missed = e->missed;
>                ^^^
> I think that we need to take the lock since we read e->missed, I see
> this possible issue:
> 
> CPU0 gets a copy of the missed events (without taking the lock)
> CPU1 has already delivered the missed events, it clears them
> CPU0 delivers missed events that were already delivered by CPU1.

Indeed, I forgot to mention that. Its harmless though, no?

>>     ret = notify->fcn(events | missed, &item);
>>     if (!success || missed) {
>>         spin_lock_bh(&ct->lock);
>>         if (!success)
>>             e->missed |= events;
>>         else
>>             e->missed &= ~missed;
>>         spin_unlock_bh(&ct->lock);
>>     }
>>
>> so if we failed to deliver the events, we add them to the missed
>> events for the next delivery attempt. Once we've delivered the
>> missed events, we clear them from the cache.
>>
>> Now is that really better - I'm not sure myself :) The per-conntrack
>> locking would be an improvement though. What do you think?
> 
> Indeed, I also think that the per-conntrack locking would be an
> improvement for the protocol helpers.
> 
> wrt. the event cache, the missed field can save us from doing the
> locking in every event caching at the cost of consuming a bit more of
> memory. I think this is more conservative but safer than my approach (no
> potential defering by calling cmpxchg forever, even if it's unlikely).
> Still, we would need to take the spin lock for the event delivery. Let
> me know what you think.

Would we really have to? The events are incremental anyways, so
it shouldn't matter if we very rarely deliver an event twice.