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
Actions

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 Lint Coverage

Unit

No 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

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.

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_t`s, 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.