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 - subversion
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

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 added reviewers: emaste, des.
sys/amd64/conf/XXHASH
2

You know you can just do this, right?

include         GENERIC
ident XXHASH

device pf
options XXHASH

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

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?

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.

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?

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.

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