Page MenuHomeFreeBSD

Proposed XXHASH implementation and change. Add support for XXHASH to pf. Leave the default as jenkins for pf and flowtable. Add an XXHASH kernel config switch.
AbandonedPublic

Authored by gnn on Oct 9 2014, 1:01 AM.

Details

Reviewers
emaste
des
Summary

Proposed XXHASH implementation.

Note that I will drop the kernel config when I commit this.

Test Plan

Using Conductor on 3 hosts, pass traffic through a host with PF enabled but with no filtering rules thereby testing purely the hash lookup.

Diff Detail

Repository
rS FreeBSD src repository
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

gnn updated this revision to Diff 1925.Oct 9 2014, 1:01 AM
gnn retitled this revision from to Proposed XXHASH implementation and change. Add support for XXHASH to pf. Leave the default as jenkins for pf and flowtable. Add an XXHASH kernel config switch..
gnn updated this object.
gnn edited the test plan for this revision. (Show Details)
gnn updated this object.Oct 9 2014, 1:04 AM
gnn added reviewers: emaste, des.
emaste added inline comments.Oct 9 2014, 1:25 AM
sys/amd64/conf/XXHASH
2

You know you can just do this, right?

include         GENERIC
ident XXHASH

device pf
options XXHASH
gnn added a comment.Oct 9 2014, 3:33 AM

Yes, I did know, but I was, clearly, lame on that one.

emaste edited edge metadata.Oct 9 2014, 3:23 PM

After all of your benchmarking is done I'd drop the knob and just make it use the new hash unconditionally.

sys/libkern/xxhash.h
31

Were these files taken verbatim from the google repo? Should it go in contrib?

gnn added a comment.Oct 9 2014, 3:26 PM

I'd prefer to pull in the code and mature it over time than put it in contrib. contrib and the kernel don't mix very well for us.

I have done extensive benchmarking but I'd prefer to keep this hash as optional for at least one release because I do not have a varied enough test setup to be sure of how well it will work for all cases.

Also, I have not tested it in the flow case which is unrelated to pf.

emaste added inline comments.Oct 9 2014, 5:38 PM
sys/netpfil/pf/pf.c
378

Looks like the same problem as before: the size arg to jenkins_hash32 is the # of uint32_ts, while xxhash is bytes.

Perhaps move the /sizeof(uint32_t) to the #define for the jenkins_hash32 case?

des edited edge metadata.Oct 9 2014, 5:49 PM

When did this simple patch turn into an overengineered mess just begging for the kind of bug Ed just pointed out? Please take a moment to read http://bikeshed.org/, then commit D461 instead.

emaste added a comment.Oct 9 2014, 6:07 PM

Note that the proposed patch in D461 has the same potential /4 issue, it's just that it was handled there.

gnn abandoned this revision.Oct 9 2014, 7:04 PM