router: optimize computeProcID

[jiceatscion]

Fixes #4519

[matzf]

This change is 

[matzf]

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jiceatscion)

---

router/dataplane.go line 604 at r2 (raw file):

	if _, err := rand.Read(randomValue); err != nil {
		panic("Error while generating random value")
	}

Adding this randomness into the hash is a security mechanism, it makes it harder for an attacker to attack the hash function. E.g. an attacker could compute a flow ID to target processing on the same processing queue as a specific victim flow. Note that the output of the hash function is very difficult to observe for an attacker, and so recovering the random value, even if it's short, would be very hard.

That said, 16 bytes of randomness is possibly excessive; 4 bytes seem to suffice to sufficiently randomize over most of the "state space" (I ran a quick experiment, a bit under 2^31 distinct outputs are reached from the full set of 2^32 four byte inputs). If hashing this same random value repeatedly has a significant cost, it could also make sense to precompute the hash state for the random input.

---

router/dataplane.go line 693 at r2 (raw file):

	return s % uint32(numProcRoutines), nil
}

Would be nice to still implement the main body of the hash function in a separate function, perhaps also in a separate file (this dataplane.go is growing to contain everything).
Something like func hashFNV1a(state uint32, c byte) uint32, then use it as s = hashFNV1a(s, c) . This should be inlined and so have no overhead. The definition of the prime32 constant can be moved into that function body. The initial offset is needed for the initialization, I'd define it as something like fnv1Offset32.

[jiceatscion]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @matzf)

---

router/dataplane.go line 604 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Adding this randomness into the hash is a security mechanism, it makes it harder for an attacker to attack the hash function. E.g. an attacker could compute a flow ID to target processing on the same processing queue as a specific victim flow. Note that the output of the hash function is very difficult to observe for an attacker, and so recovering the random value, even if it's short, would be very hard.

That said, 16 bytes of randomness is possibly excessive; 4 bytes seem to suffice to sufficiently randomize over most of the "state space" (I ran a quick experiment, a bit under 2^31 distinct outputs are reached from the full set of 2^32 four byte inputs). If hashing this same random value repeatedly has a significant cost, it could also make sense to precompute the hash state for the random input.

Done.

---

router/dataplane.go line 693 at r2 (raw file):

Previously, matzf (Matthias Frei) wrote…

Would be nice to still implement the main body of the hash function in a separate function, perhaps also in a separate file (this dataplane.go is growing to contain everything).
Something like func hashFNV1a(state uint32, c byte) uint32, then use it as s = hashFNV1a(s, c) . This should be inlined and so have no overhead. The definition of the prime32 constant can be moved into that function body. The initial offset is needed for the initialization, I'd define it as something like fnv1Offset32.

Done.

[matzf]

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jiceatscion)

---

router/dataplane.go line 675 at r3 (raw file):

	}

	var s uint32 = hashSeed

s := hashSeed

---

router/dataplane_internal_test.go line 217 at r3 (raw file):

func TestComputeProcId(t *testing.T) {
	randomValue := uint32(1234)
	randomValueBytes := []byte{byte(randomValue & 0xff), byte(randomValue >> 8), 0, 0}

Maybe revert this bit here, IMO it was more readable before: randomValue := []byte{1, 2, 3, 4}

---

router/fnv1aCheap.go line 19 at r3 (raw file):

// fnv1aOffset32 is an initial offset that can be used as initial state when calling
// hashFNV1a.
const fnv1aOffset32 = 2166136261

Make this uint32, const fnv1aOffset32 uint32 = ..., and remove the cast where the value is referenced.

[jiceatscion]

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @matzf)

---

router/dataplane.go line 675 at r3 (raw file):

Previously, matzf (Matthias Frei) wrote…

s := hashSeed

Done.

---

router/fnv1aCheap.go line 19 at r3 (raw file):

Previously, matzf (Matthias Frei) wrote…

Make this uint32, const fnv1aOffset32 uint32 = ..., and remove the cast where the value is referenced.

Done.

[matzf]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @jiceatscion)

[matzf]

For whatever it's worth, comparing the router benchmark results with this PR against nightly, I see an improvement of 0.09%. Not a whole lot gained, but not a lot of complexity either.

[jiceatscion]

Yeah. Most likely the time removed from computing was added to the time sleeping :-( Oh well, when we improve the I/O side we'll be able to profit from this.

Reviewable status: complete! all files reviewed, all discussions resolved