[PATCH 0/10] updates for ctnetlink and conntrack core

[PATCH 0/10] updates for ctnetlink and conntrack core

[Pablo Neira Ayuso]

Hi,

Here follows a set of patches for ctnetlink and the conntrack core API.

Basically, at the time that I was working on the ctnetlink patches, I 
tried to keep in mind the idea of reducing the netlink bandwidth 
consumption following the principle of just dumping meaningful fields 
depending on the type of event.

Please let me know what you think. Thanks!

cheers,
	Pablo

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of 
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

Re: [PATCH 0/10] updates for ctnetlink and conntrack core

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Hi,
> 
> Here follows a set of patches for ctnetlink and the conntrack core API.

I'm perfectly fine with 1, 2, and 9. Still need to look at 10 in
more detail, 3 - 5 need a bit more work. As for 6 - 8, see below.

> Basically, at the time that I was working on the ctnetlink patches, I
> tried to keep in mind the idea of reducing the netlink bandwidth
> consumption following the principle of just dumping meaningful fields
> depending on the type of event.
> 
> Please let me know what you think. Thanks!

I think this is something we need to agree on in principle. I'm
not convinced that we really do save much bandwidth in the
common case, and that its worth diverging from the usual update
notifications containing full updates sent by the remaining
network stack (besides unset fields or fields containing 0).

I'll see if I can get some numbers of the actual differences
without too much effort.

Re: [PATCH 0/10] updates for ctnetlink and conntrack core

[Patrick McHardy]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
> 
>>Basically, at the time that I was working on the ctnetlink patches, I
>>tried to keep in mind the idea of reducing the netlink bandwidth
>>consumption following the principle of just dumping meaningful fields
>>depending on the type of event.
>>
> I think this is something we need to agree on in principle. I'm
> not convinced that we really do save much bandwidth in the
> common case, and that its worth diverging from the usual update
> notifications containing full updates sent by the remaining
> network stack (besides unset fields or fields containing 0).
> 
> I'll see if I can get some numbers of the actual differences
> without too much effort.

In some very unscientific tests it showed a difference of about
4% when dumping everything compared to only diffs (measured
with conntrack accounting enabled). Not something to easily
throw away, but I bet we can easily increase the netlink bandwidth
for ctnetlink by using more reasonable allocation size and
avoiding the netlink_trim call for every single packet.

Re: [PATCH 0/10] updates for ctnetlink and conntrack core

[Patrick McHardy]

Patrick McHardy wrote:
> In some very unscientific tests it showed a difference of about
> 4% when dumping everything compared to only diffs (measured
> with conntrack accounting enabled). Not something to easily
> throw away, but I bet we can easily increase the netlink bandwidth
> for ctnetlink by using more reasonable allocation size and
> avoiding the netlink_trim call for every single packet.

Some more unscientific tests: saving the netlink_trim call by using
an allocation size of 200 reduces overhead for the entire notification
function (including broadcasting) by about 13% (and also reduces
the overruns experienced by conntrack -E for ~390000 packets with
65536 new connections from 17 to 7, which I don't really understand
since it shouldn't save any netlink bandwidth and this is a SMP system).

Re: [PATCH 0/10] updates for ctnetlink and conntrack core

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Patrick McHardy wrote:
> 
>>In some very unscientific tests it showed a difference of about
>>4% when dumping everything compared to only diffs (measured
>>with conntrack accounting enabled). Not something to easily
>>throw away, but I bet we can easily increase the netlink bandwidth
>>for ctnetlink by using more reasonable allocation size and
>>avoiding the netlink_trim call for every single packet..

4% is not too much but it is something, anyway it is true that my 
argument of bandwidth consumption is weak.

But I still like patch 8, if the event cache works fine, I can't see why 
we should force the dumping of all fields. BTW, patch 7 is a new feature 
that I think that we need.

> Some more unscientific tests: saving the netlink_trim call by using
> an allocation size of 200 reduces overhead for the entire notification
> function (including broadcasting) by about 13% (and also reduces
> the overruns experienced by conntrack -E for ~390000 packets with
> 65536 new connections from 17 to 7, which I don't really understand
> since it shouldn't save any netlink bandwidth and this is a SMP system).

Who ever said that benchmarking can be scientific? :) I usually have to 
redo my tests several times. These results are interesting but, in 
previous discussions, I thought that we could not change current 
allocation size because of memory fragmentation issues?

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of 
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

Re: [PATCH 0/10] updates for ctnetlink and conntrack core

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> Patrick McHardy wrote:
> 
>> Patrick McHardy wrote:
>>
>>> [...]
> 
> 4% is not too much but it is something, anyway it is true that my
> argument of bandwidth consumption is weak.
> 
> But I still like patch 8, if the event cache works fine, I can't see why
> we should force the dumping of all fields. BTW, patch 7 is a new feature
> that I think that we need.

Patch 7 is something we definitely want. For patch 8 we need to come
to a conclusion on how ctnetlink should work first, I prefer to keep
it similar to other network netlink protocols and have it send full
update notifications, containing the entire current state. I also
fail to see the difference it makes, of the four things that are
dumped conditionally (status, refresh, protoinfo, helpinfo) the
first three are always set for new conntracks. The last one only
if we have a helper, but if we don't nothing is dumped anyway.

>> Some more unscientific tests: saving the netlink_trim call by using
>> an allocation size of 200 reduces overhead for the entire notification
>> function (including broadcasting) by about 13% (and also reduces
>> the overruns experienced by conntrack -E for ~390000 packets with
>> 65536 new connections from 17 to 7, which I don't really understand
>> since it shouldn't save any netlink bandwidth and this is a SMP system).
> 
> 
> Who ever said that benchmarking can be scientific? :) I usually have to
> redo my tests several times. These results are interesting but, in
> previous discussions, I thought that we could not change current
> allocation size because of memory fragmentation issues?

I _decreased_ the size, the messages sent are about 150-200 bytes, yet
we're allocating an entire page, which makes netlink clone the skb
and shrink it to its actual size. We should just to it right ourselves.

Re: [PATCH 0/10] updates for ctnetlink and conntrack core

[Pablo Neira Ayuso]

Patrick McHardy wrote:
> Pablo Neira Ayuso wrote:
> 
>>Patrick McHardy wrote:
>>
>>
>>>Patrick McHardy wrote:
>>>
>>>
>>>>[...]
>>
>>4% is not too much but it is something, anyway it is true that my
>>argument of bandwidth consumption is weak.
>>
>>But I still like patch 8, if the event cache works fine, I can't see why
>>we should force the dumping of all fields. BTW, patch 7 is a new feature
>>that I think that we need.
> 
> 
> Patch 7 is something we definitely want. For patch 8 we need to come
> to a conclusion on how ctnetlink should work first, I prefer to keep
> it similar to other network netlink protocols and have it send full
> update notifications, containing the entire current state.

If other network netlink protocols behave like that, I agree that we 
should send full update notifications, I like this argument. The only 
thing that annoys me is the limited netlink throughput, sending more 
data means that on stress situations an overflow could comes before.

> I also fail to see the difference it makes, of the four things that are
> dumped conditionally (status, refresh, protoinfo, helpinfo) the
> first three are always set for new conntracks. The last one only
> if we have a helper, but if we don't nothing is dumped anyway.

So, to keep consistency with other netlink subsystem, I can remake my 
patch based on this assumptions:

- NEW events dump everything but values that are unset like counters, 
mark, ...
- UPDATE events dump everything but unset fields and the counters, they 
don't provide very useful information AFAICS.
- DESTROY, dump everything, counters included.

Please let me know what you think.

>>>Some more unscientific tests: saving the netlink_trim call by using
>>>an allocation size of 200 reduces overhead for the entire notification
>>>function (including broadcasting) by about 13% (and also reduces
>>>the overruns experienced by conntrack -E for ~390000 packets with
>>>65536 new connections from 17 to 7, which I don't really understand
>>>since it shouldn't save any netlink bandwidth and this is a SMP system).
>>
>>
>>Who ever said that benchmarking can be scientific? :) I usually have to
>>redo my tests several times. These results are interesting but, in
>>previous discussions, I thought that we could not change current
>>allocation size because of memory fragmentation issues?
> 
> 
> I _decreased_ the size, the messages sent are about 150-200 bytes, yet
> we're allocating an entire page, which makes netlink clone the skb
> and shrink it to its actual size. We should just to it right ourselves.

OK, I see, sorry I misunderstood.

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of 
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

Re: [PATCH 0/10] updates for ctnetlink and conntrack core

[Patrick McHardy]

Pablo Neira Ayuso wrote:
> So, to keep consistency with other netlink subsystem, I can remake my
> patch based on this assumptions:
> 
> - NEW events dump everything but values that are unset like counters,
> mark, ...
> - UPDATE events dump everything but unset fields and the counters, they
> don't provide very useful information AFAICS.
> - DESTROY, dump everything, counters included.
> 
> Please let me know what you think.

That sounds good, but I don't think DESTROY needs to dump more than
NEW or UPDATE.