Experiment ac rust/v6 #9904

victorjulien · 2023-11-27T21:08:57Z

Some results:

AMD Ryzen 7 PRO 4750U

ET Open

open-acbs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401141              646         742781         33429         13.4b  39.79 
open-acks/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401143              646         492116         16103          6.5b  33.56 
open-ac/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401143              646         407711         16314          6.5b  32.83 
open-acrs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401142              629         509745         27460         11.0b  39.35 
open-hs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401140              646         185555          7636          3.1b  24.93

So HS > AC-KS > AC -> AC-RS > AC-BS.

ET pro

pro-acbs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401140              646        1117291         46861         18.8b  39.77 
pro-acks/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401139              646        1077987         26305         10.6b  36.72 
pro-ac/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401141              646        1076882         26372         10.6b  35.14 
pro-acrs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401145              629         630326         31790         12.8b  37.11 
pro-hs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401142              646         529499         13350          5.4b  27.80

So HS > AC-KS > AC > AC-RS > AC-BS.

Intel Xeon W-2245

ET Open

open-acbs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401143             1101        2702821         82287         33.0b  41.89 
open-acks/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401140             1095        2397842         28562         11.5b  34.55 
open-ac/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401145             1107        2440875         27225         10.9b  33.30 
open-acrs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401141             1093        2404475         64559         25.9b  43.01 
open-hs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401140             1268        2411467         14876          6.0b  26.41

So HS > AC > AC-KS > AC-RS > AC-BS.

ET Pro

pro-acbs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401141             1111        2596829        106931         42.9b  42.16 
pro-acks/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401142             1080        2530589         40839         16.4b  35.41 
pro-ac/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401141             1099        2456543         38078         15.3b  33.81 
pro-acrs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401138             1091        2889362         66772         26.8b  39.55 
pro-hs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        401142             1264        2453707         23711          9.5b  29.24

So HS > AC > AC-KS > AC-RS > AC-BS.

Rock 5b (arm64)

ET Open

open-acbs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        399575                1            712            32         13.0m  43.02 
open-acks/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        393397                1           1120            15          6.0m  37.96 
open-ac/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        391273                1            625            14          5.7m  35.92 
open-acrs/packet_stats.log:PROF_DETECT_PF_PAYLOAD      IPv4       6        400380                1           1147            26         10.8m  42.33

AC > AC-KS > AC-RS > AC-BS.

Results are in a different unit than ticks in our arm64 code.

Avoid redundant pointer checks; instead check once.

victorjulien · 2023-11-27T21:10:59Z

So in conclusion, it seems that "AC-RS" in this PR is better than our "AC-BS", but not nearly as good as our regular "AC" and "AC-KS". Hyperscan is even better than those.

I think I'd like to merge this still, perhaps replacing "ac-bs" so we can revisit this as Rust and/or the aho-corasick crate improve.

Count uniq matches only, like the Hyperscan implementation. Ticket: OISF#3847.

victorjulien · 2023-11-27T21:35:11Z

OTOH maybe just remove ac-bs and keep this work as a branch to revisit every once in a while.

codecov · 2023-11-27T21:35:19Z

Codecov Report

Merging #9904 (4e722ba) into master (d005fff) will decrease coverage by 0.16%.
The diff coverage is 90.74%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9904      +/-   ##
==========================================
- Coverage   82.45%   82.29%   -0.16%     
==========================================
  Files         972      973       +1     
  Lines      273057   273878     +821     
==========================================
+ Hits       225156   225400     +244     
- Misses      47901    48478     +577

Flag	Coverage Δ
fuzzcorpus	`63.78% <85.53%> (-0.59%)`	⬇️
suricata-verify	`61.05% <85.53%> (-0.04%)`	⬇️
unittests	`62.98% <90.49%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

jasonish · 2023-11-27T22:52:56Z

OTOH maybe just remove ac-bs and keep this work as a branch to revisit every once in a while.

Yeah, was going to ask, whats the purpose of keeping ac-bs?

jasonish · 2023-11-27T22:54:30Z

rust/src/common.rs

+}
+
+#[no_mangle]
+pub extern "C" fn rs_mpm_acrs_new_builder() -> *mut std::os::raw::c_void {


In addition to the unsafe notes from the previous PR... We've been doing C style naming for functions that form the C API recently.. So SCMpmAcRsNewBuilder or whatever.

I guess this kind of breaks the way we've been looking at the impact of Rust functions tho.

These functions would not need to be part of a public C API. C API should only expose it through the generic MPM support. So would that mean we drop the SC? MpmAcRsNewBuilder?

If its no_mangle extern "C" its part of the public API, as far as linking goes which is why we want to name space under SC.

Hmm can we have a way to make things not public from a libsuri perspective? This stuff is public not because I want it to be, but because the C-Rust FFI.

Name, like SCPrivate... or _SC to make it clear its for private use only. Additionally removing or ifdef out of the public headers, either would require some creative use or post-processing of the cbindgen output. The namespacing is to avoid a collision, and the name scheme or ifdef is to avoid unintentional linking. Nothing we can do about intentional linking.

suricata-qa · 2023-11-27T23:00:04Z

ERROR:

ERROR: QA failed on TREX_GENERIC_collect.

Pipeline 16748

victorjulien · 2023-11-28T05:54:54Z

OTOH maybe just remove ac-bs and keep this work as a branch to revisit every once in a while.

Yeah, was going to ask, whats the purpose of keeping ac-bs?

Will do a few more tests to see if there is hw where ac-bs works well, but will remove it otherwise.

victorjulien · 2023-11-28T13:33:49Z

OTOH maybe just remove ac-bs and keep this work as a branch to revisit every once in a while.

Yeah, was going to ask, whats the purpose of keeping ac-bs?

Will do a few more tests to see if there is hw where ac-bs works well, but will remove it otherwise.

https://redmine.openinfosecfoundation.org/issues/6586

victorjulien · 2023-11-28T16:37:41Z

closing in favor of #9911

victorjulien added 4 commits November 27, 2023 12:48

mpm: cleanup pattern free function

99f9179

Avoid redundant pointer checks; instead check once.

mpm: free sids in MpmFreePattern as well

c2e4527

ac/bs: reset sids pointer after xfer

1fca02d

util/prefilter: constify sids

47f2bc3

victorjulien added 2 commits November 27, 2023 22:16

mpm/ac-rs: AC matcher based on aho-corasick crate

67bdeda

Count uniq matches only, like the Hyperscan implementation. Ticket: OISF#3847.

TEST default to mpm-algo=ac-rs for testing purposes

4e722ba

victorjulien force-pushed the experiment-ac-rust/v6 branch from efff325 to 4e722ba Compare November 27, 2023 21:16

victorjulien mentioned this pull request Nov 27, 2023

Experiment ac rust/v4 #9902

Closed

jasonish reviewed Nov 27, 2023

View reviewed changes

victorjulien mentioned this pull request Nov 28, 2023

Experiment ac rust/v11 #9911

Closed

victorjulien closed this Nov 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Experiment ac rust/v6 #9904

Experiment ac rust/v6 #9904

victorjulien commented Nov 27, 2023

victorjulien commented Nov 27, 2023

victorjulien commented Nov 27, 2023

codecov bot commented Nov 27, 2023

jasonish commented Nov 27, 2023

jasonish Nov 27, 2023

victorjulien Nov 28, 2023

jasonish Nov 28, 2023

victorjulien Nov 28, 2023

jasonish Nov 28, 2023

suricata-qa commented Nov 27, 2023

victorjulien commented Nov 28, 2023

victorjulien commented Nov 28, 2023

victorjulien commented Nov 28, 2023

Experiment ac rust/v6 #9904

Experiment ac rust/v6 #9904

Conversation

victorjulien commented Nov 27, 2023

AMD Ryzen 7 PRO 4750U

ET Open

ET pro

Intel Xeon W-2245

ET Open

ET Pro

Rock 5b (arm64)

ET Open

victorjulien commented Nov 27, 2023

victorjulien commented Nov 27, 2023

codecov bot commented Nov 27, 2023

Codecov Report

jasonish commented Nov 27, 2023

jasonish Nov 27, 2023

Choose a reason for hiding this comment

victorjulien Nov 28, 2023

Choose a reason for hiding this comment

jasonish Nov 28, 2023

Choose a reason for hiding this comment

victorjulien Nov 28, 2023

Choose a reason for hiding this comment

jasonish Nov 28, 2023

Choose a reason for hiding this comment

suricata-qa commented Nov 27, 2023

victorjulien commented Nov 28, 2023

victorjulien commented Nov 28, 2023

victorjulien commented Nov 28, 2023