[Snort-devel] The _real_ performance improvement: TestHeader was broken!

Christian Mock cm at ...1206...
Thu Mar 21 16:02:02 EST 2002


> This has been changed :-)  It will be changed further in the future
> but it's too easy a change and too big a difference for the multiple
> CIDR Folks to not do.

So, after an evening of improving my patch and rewriting the whole logic
of EvalHeader, I found the real culprit, and of course it wasn't anywhere
near that :-)

TestHeader() is borken -- it does an memcmp() on the IpAddrSet structures,
but that doesn't work since even with the same data, the next pointer is
different. So when building the RTN chains, it makes 982 chains of 987
rules, instead of 154 chains.

Changing this to explicit comparisons of the relevant struct members
fixes the problem, and brings runtime down from 2:56 to 0:25 (tcpdump
file with 100'000 packets from a real network, config and ruleset from
the same setup; the 2:56 is with snort 1.84 as distributed).

Combined with the position change of port and IP checking, we get down
to 0:20, or 11% of the original (can you tell I can get very excited
about performance-tuning stuff? :-).

The patch is attached as snort-testheader.diff, and I'm pretty sure it can't
break anything.

----

The other stuff I did brings the time further down to 0:18, but I'm not 
sure it doesn't break anything.

What I did was to "inline" all the port and IP address checks into EvalHeader,
on the assumption that since this is a critical code path, the CPU and 
compiler can be much more efficient if everything is right there in the cache
and available for optimiziation, plus we lose the function call overhead.

I did check that the pattern match and other functions get called the same
number of times as with the original code, but I'm not sure what it does
with features I'm not using, like dynamic rules etc. It also doesn't make
the code any easier to follow.

Given the small improvement it gives us above the two little and riskless 
changes, I don't know if it's worth the hassle; still, I attach it as 
snort-cm.diff.gz (includes the TestHeader patch).

I also tried changing the recursive nature of EvalHeader to a big while loop
(again on the assumption of lowering function call overhead), but that gains 
nothing.

g'night,

cm.

-- 
Christian Mock                          Wiedner Hauptstrasse 15
Senior Security Engineer                1040 Wien
CoreTEC IT Security Solutions GmbH      +43-1-5037273
-------------- next part --------------
--- snort-1.8.4/rules.c.orig	Fri Mar 22 00:22:50 2002
+++ snort-1.8.4/rules.c	Fri Mar 22 00:23:26 2002
@@ -2972,7 +2972,9 @@
     rtn_idx = rtn->sip;
     for(rule_idx=rule->sip;rule_idx!=NULL;rule_idx=rule_idx->next)
     {
-        if(!memcmp(rtn_idx, rule_idx, sizeof(IpAddrSet)))
+        if((rtn_idx->ip_addr == rule_idx->ip_addr) &&
+	   (rtn_idx->netmask == rule_idx->netmask) &&
+	   (rtn_idx->addr_flags == rule_idx->addr_flags))
         {
             rtn_idx = rtn_idx->next;
         }
@@ -2985,7 +2987,9 @@
     rtn_idx = rtn->dip;
     for(rule_idx=rule->dip;rule_idx!=NULL;rule_idx=rule_idx->next)
     {
-        if(!memcmp(rtn_idx, rule_idx, sizeof(IpAddrSet)))
+        if((rtn_idx->ip_addr == rule_idx->ip_addr) &&
+	   (rtn_idx->netmask == rule_idx->netmask) &&
+	   (rtn_idx->addr_flags == rule_idx->addr_flags))
         {
             rtn_idx = rtn_idx->next;
         }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: snort-cm.diff.gz
Type: application/octet-stream
Size: 2473 bytes
Desc: not available
URL: <https://lists.snort.org/pipermail/snort-devel/attachments/20020321/b930abfc/attachment.obj>


More information about the Snort-devel mailing list