router: serialize directly into packet buffers

[jiceatscion]

In every case where the router modified packets it would serialize updated headers to a temporary buffer and then copy that to the packet buffer.

To avoid this extra copy, replaced gopacket.serializeBuffer with a custom implementation that writes to a given buffer. In this case, the packet's raw buffer.

There is one remaining copy for some SCMP messages because we have to move the existing packet to the payload. This too could be avoided but that's for another PR; it would require to leave headroom in received packets.

The performance impact is very small since, on the critical path, it just avoids copying a scion hdr per packet, but it is a simplification. It also pays back the copy added by a previous simplification of the BFD code. As such...

Contributes to #4593

[jiceatscion]

This change is 

[marcfrei]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @FR4NK-W, @jiceatscion, @lukedirtwalker, and @oncilla)

---

router/serializeProxy.go line 0 at r2 (raw file):
We don't usually have file names in camel case, right?

---

router/serializeProxy.go line 27 at r2 (raw file):

// panic. It is designed to be a local variable, so New() returns a value. The entire buffer
// underpinning the given slice may be used; that is, from the start up to the remaining capacity.
type serializeProxy struct {

How about serializeBuffer instead of serializeProxy. "Proxy" can have so many meanings, not sure that the name helps here.

---

router/serializeProxy.go line 28 at r2 (raw file):

// underpinning the given slice may be used; that is, from the start up to the remaining capacity.
type serializeProxy struct {
	initStart int // Initial starting point (where the first prepend or append occurs)

orig or origStart?

Code quote:

	initStart

---

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

	payloadOffset := len(rawPkt) - len(s.LayerPayload())
	serBuf := newSerializeProxy(rawPkt)
	serBuf.clear(payloadOffset) // Prepends will go just before payload. (Appends will wreck it)

Wouldn't it be cleaner to provide a special factory function for this use case instead of using serializeProxy.clear?

Code quote:

	serBuf := newSerializeProxy(rawPkt)
	serBuf.clear(payloadOffset) // Prepends will go just before payload. (Appends will wreck it)

[jiceatscion]

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @FR4NK-W, @lukedirtwalker, @marcfrei, and @oncilla)

---

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

Previously, marcfrei (Marc Frei) wrote…

Wouldn't it be cleaner to provide a special factory function for this use case instead of using serializeProxy.clear?

Yeah, I wasn't sure that I wanted to add a constructor for such a minority use case. Although it might become more frequent...So, reworked along those lines. I've changed initStart to restart speculatively.

---

router/serializeProxy.go line 27 at r2 (raw file):

Previously, marcfrei (Marc Frei) wrote…

How about serializeBuffer instead of serializeProxy. "Proxy" can have so many meanings, not sure that the name helps here.

I chose the name to distinguish it from the default implementation. The default implementation does supply the buffer, which is its main downside. This one acts as an intermediary between the using code and the actual buffer. So, a buffer-by-proxy. Of cause proxy is overloaded, but serializeBuffer is already taken and misleading. I have searched for a better name but couldn't find anything that wasn't just as overused. Ideas welcome.

---

router/serializeProxy.go line 28 at r2 (raw file):

Previously, marcfrei (Marc Frei) wrote…

orig or origStart?

uh, initStart is bad, but so is origStart. We reset to that value at the next Clear()... how about clearStart or resetStart or ...how about restart?

---

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

Previously, marcfrei (Marc Frei) wrote…

We don't usually have file names in camel case, right?

oops, fixed

[marcfrei]

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

---

router/serialize_proxy.go line 47 at r3 (raw file):

// newSerializeProxyOffset returns a new serializeProxy. The initial prepend/append point is set to
// the given start value. This has the same effect as calling clear(statr).
func newSerializeProxyOffset(buf []byte, start int) serializeProxy {

NIT: either name the function newSerializeProxyStart or rename the parameter from start to offset

Code quote:

Offset

---

router/#connector.go# line 0 at r3 (raw file):
Shouldn't be part of this commit, right?

[jiceatscion]

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @FR4NK-W, @lukedirtwalker, @marcfrei, and @oncilla)

---

router/#connector.go# line at r3 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Shouldn't be part of this commit, right?

nope. I was wondering where that file had gone (in another branch - expected to be stashed) :-) removed.

---

router/serialize_proxy.go line 47 at r3 (raw file):

Previously, marcfrei (Marc Frei) wrote…

NIT: either name the function newSerializeProxyStart or rename the parameter from start to offset

done

---

router/serializeProxy.go line 27 at r2 (raw file):

Previously, jiceatscion wrote…

I chose the name to distinguish it from the default implementation. The default implementation does supply the buffer, which is its main downside. This one acts as an intermediary between the using code and the actual buffer. So, a buffer-by-proxy. Of cause proxy is overloaded, but serializeBuffer is already taken and misleading. I have searched for a better name but couldn't find anything that wasn't just as overused. Ideas welcome.

I guess you have accepted my reasons.

[jiceatscion]

Dismissed @marcfrei from a discussion.
Reviewable status: 3 of 4 files reviewed, all discussions resolved (waiting on @FR4NK-W, @lukedirtwalker, and @oncilla)

---

router/#connector.go# line at r3 (raw file):

Previously, jiceatscion wrote…

nope. I was wondering where that file had gone (in another branch - expected to be stashed) :-) removed.

so, done

[marcfrei]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @FR4NK-W, @lukedirtwalker, and @oncilla)