Remove the need to allocate src addresses in the afpacket underlay.

The underlay shouldn't be producing any garbage any more. Src addresses get
stored at the head of the packet buffer.

Author: jiceathome

router/dataplane.go

@@ -204,7 +204,7 @@ func (p *Packet) reset(headroom int) {
 	// Everything else is reset to zero value.
 }
 
-// WithHeader returns the a slice of the underlying packet buffer that represents the same bytes as
+// WithHeader returns a slice of the underlying packet buffer that represents the same bytes as
 // p.rawPacket[:] plus the n prededing bytes. This slice is meant to be used when receiving a raw
 // packet with an n bytes header, such that the payload is exactly at p.rawPacket[0:]. p.RawPacket
 // is *not* modified. This method panics if n is greater than the available headroom in the packet
@@ -217,6 +217,14 @@ func (p *Packet) WithHeader(n int) []byte {
 	return p.buffer[start-n : end]
 }
 
+// BufHead returns a slice of bytes of the requested size borrowed from the head of the packet.
+// buffer. This space can be used safely by an underlay to store data on ingest and retrieve on
+// egress; should the same underlay perform both operations. The data is protected against
+// overwrites provided that n is included in the underlay's headroom requirements.
+func (p *Packet) BuffHead(n int) []byte {
+	return p.buffer[0:n]
+}
+
 // PacketPool allocates and resets packets. There is one packet pool per instance of the dataplane,
 // shared between all its underlay instances. This structure can be shared by copying (and doing so
 // is more efficient) because headroom is never changed after construction and channel is a
@@ -2376,7 +2384,7 @@ func (p *slowPathPacketProcessor) prepareSCMP(
 			}
 		} else {
 			// Serialize in front of the quoted packet. The quoted packet must be included in the
-			// serialize buffer before we pack the SCMP header in from of it. AppendBytes will do
+			// serialize buffer before we pack the SCMP header in front of it. AppendBytes will do
 			// that; it exposes the underlying buffer but doesn't modify it.
 			p.pkt.RawPacket = p.pkt.buffer[0:(quoteLen + headroom)]
 			serBuf = NewSerializeProxyStart(p.pkt.RawPacket, headroom)

router/underlayproviders/afpacketudpip/afpacketudpip.go

@@ -64,6 +64,10 @@ const (
 	udpv6LenOffset     = udpv6Offset + 4
 	udpv6SumOffset     = udpv6Offset + 6
 	udpv6DstPortOffset = udpv6Offset + 2
+
+	ipv4AddrLen = 4
+	ipv6AddrLen = 16
+	portLen     = 2
 )
 
 var (
@@ -168,7 +172,7 @@ type udpLink interface {
 	router.Link
 	start(ctx context.Context, procQs []chan *router.Packet, pool router.PacketPool)
 	stop()
-	receive(srcAddr *netip.AddrPort, p *router.Packet)
+	receive(p *router.Packet)
 	handleNeighbor(isReq bool, targetIP, senderIP, rcptIP netip.Addr, remoteHw [6]byte)
 }
 
@@ -223,9 +227,9 @@ func (u *underlay) Headroom() int {
 	// header is v4 or v6 or has options or extensions. We align the packet with the assumtion that
 	// it is v4 with no options. As a result, the payload never starts earlier than planned. This is
 	// needed to ensure that the headroom we leave is never less than the worst case requirement
-	// across all underlays.
-
-	return ethLen + ipv6Len + udpLen
+	// across all underlays. We add the binary representation of src address and src port to our
+	// headroom requirements, so internal links can safely use packet.BuffHead() to store those.
+	return ethLen + ipv6Len + udpLen + ipv6AddrLen + portLen
 }
 
 func (u *underlay) SetDispatchPorts(start, end, redirect uint16) {

router/underlayproviders/afpacketudpip/internallink.go

@@ -21,7 +21,6 @@ import (
 	"net"
 	"net/netip"
 	"sync/atomic"
-	"unsafe"
 
 	"github.com/gopacket/gopacket"
 	"github.com/gopacket/gopacket/layers"
@@ -54,6 +53,36 @@ type internalLink struct {
 	is4              bool
 }
 
+// getRemote returns the ip address and port of the far end of the packet's trip.
+//
+// It is used as follows:
+// * We set it from src address when we ingest a packet: it will be used for any SCMP response.
+// * We set it when the main router code has us resolve the destination (for non-responses).
+// * We use it as destination when finally sending the packet.
+//
+// That address and port are stored at the beginning of the packet buffer so we do not need to
+// allocate. getRemoteAddr returns a slice pointing directly at that storage. It is meant to
+// copied into the outgoing packet header.
+func getRemoteAddr(p *router.Packet, is4 bool) ([]byte, uint16) {
+	if is4 {
+		bh := p.BuffHead(6)
+		return bh[:4], binary.BigEndian.Uint16(bh[4:6])
+	}
+	bh := p.BuffHead(18)
+	return bh[:16], binary.BigEndian.Uint16(bh[16:18])
+}
+
+// setRemote stores the ip address and port of the far end of the packet's trip.
+//
+// That address and port are stored at the beginning of the packet buffer.
+func setRemoteAddr(p *router.Packet, ip []byte, port uint16) {
+	// FWIW: The storage format is identical to that produced by AddrPort.marshalBinary
+	// as long as there's no zone. Just without all the hullabaloo because we never need a netip.
+	bh := p.BuffHead(len(ip) + 2)
+	copy(bh, ip)
+	binary.BigEndian.PutUint16(bh[len(ip):], port)
+}
+
 // This is called during initialization only and does not need the neighbors cache. The header
 // is incomplete and gets patched for each packet.
 func (l *internalLink) packHeader() {
@@ -119,9 +148,13 @@ func (l *internalLink) packHeader() {
 // in the destination. If the destination is not resolved, this method returns false and the
 // packet is left with an incorrect header. Note that an address resolution is triggered if the
 // destination is not already resolved.
-func (l *internalLink) addHeader(p *router.Packet, dst *netip.AddrPort) bool {
-	dstIP := dst.Addr()
-
+func (l *internalLink) addHeader(p *router.Packet) bool {
+	dstIPBytes, dstPort := getRemoteAddr(p, l.is4)
+	dstIP, ok := netip.AddrFromSlice(dstIPBytes)
+	if !ok {
+		// This is an internal error: these bytes were stored and validated by us.
+		panic("Broken remote address")
+	}
 	// Resolve the destination MAC address if we can.
 	l.neighbors.Lock()
 	dstMac, backlog := l.neighbors.get(dstIP) // Send ARP/NDP req as needed.
@@ -145,19 +178,19 @@ func (l *internalLink) addHeader(p *router.Packet, dst *netip.AddrPort) bool {
 	// Inject dest.
 	copy(p.RawPacket, dstMac[:])
 	if l.is4 {
-		copy(p.RawPacket[ipv4DstOffset:], dst.Addr().AsSlice()) // Can do cheaper?
-		binary.BigEndian.PutUint16(p.RawPacket[udpv4DstPortOffset:], dst.Port())
+		copy(p.RawPacket[ipv4DstOffset:], dstIPBytes) // Can do cheaper?
+		binary.BigEndian.PutUint16(p.RawPacket[udpv4DstPortOffset:], dstPort)
 	} else {
-		copy(p.RawPacket[ipv6DstOffset:], dst.Addr().AsSlice()) // Can do cheaper?
-		binary.BigEndian.PutUint16(p.RawPacket[udpv6DstPortOffset:], dst.Port())
+		copy(p.RawPacket[ipv6DstOffset:], dstIPBytes) // Can do cheaper?
+		binary.BigEndian.PutUint16(p.RawPacket[udpv6DstPortOffset:], dstPort)
 	}
 	return true
 }
 
 // TODO(jiceatscion): can do cleaner, more legible, faster?
-func (l *internalLink) finishPacket(p *router.Packet, dst *netip.AddrPort) bool {
+func (l *internalLink) finishPacket(p *router.Packet) bool {
 	payloadLen := len(p.RawPacket)
-	if !l.addHeader(p, dst) {
+	if !l.addHeader(p) {
 		return false
 	}
 	if l.is4 {
@@ -304,12 +337,7 @@ func (l *internalLink) Resolve(p *router.Packet, dst addr.Host, port uint16) err
 		port = l.dispatchRedirect
 	}
 
-	// Packets that get here must have come from an external or a sibling link; neither of which
-	// attach a RemoteAddr to the packet (besides; it could be a different type). So, RemoteAddr is
-	// not generally usable. We must allocate a new object. The precautions needed to pool them cost
-	// more than the pool saves (verified experimentally).
-	addrPort := netip.AddrPortFrom(dstAddr, port)
-	p.RemoteAddr = unsafe.Pointer(&addrPort)
+	setRemoteAddr(p, dstAddr.AsSlice(), port)
 	return nil
 }
 
@@ -332,8 +360,7 @@ func (l *internalLink) sendBacklog(dstAddr netip.Addr) {
 				continue
 			}
 			// The neighbor cache doesn't know the dest port, but the full address is in the packet.
-			dst := (*netip.AddrPort)(p.RemoteAddr)
-			if !l.finishPacket(p, dst) {
+			if !l.finishPacket(p) {
 				// Note that this packet goes back onto the backlog so we will drop it at the end of
 				// the loop. TODO(jiceatscion): need new drop reason.
 				givenup = true
@@ -360,8 +387,7 @@ func (l *internalLink) Send(p *router.Packet) {
 	// instead of just storing the destination in the packet structure. That would save us the
 	// allocation of address but requires some more changes to the dataplane code structure.
 
-	dst := (*netip.AddrPort)(p.RemoteAddr)
-	if !l.finishPacket(p, dst) {
+	if !l.finishPacket(p) {
 		// The packet got put on the backlog (or discarded if the backlog is full).
 		return
 	}
@@ -377,8 +403,7 @@ func (l *internalLink) Send(p *router.Packet) {
 // Only tests actually use this method, but since we have to have it, we might as well implement it
 // ~correctly. Doesn't hurt. TODO(jiceatscion): deal with backlog (or not).
 func (l *internalLink) SendBlocking(p *router.Packet) {
-	// Likewise: p.remoteAddress -> header.
-	if l.finishPacket(p, (*netip.AddrPort)(p.RemoteAddr)) {
+	if l.finishPacket(p) {
 		l.egressQ <- p
 	}
 	// else, backlog'd or discarded => non-blocking after all. Sorry.
@@ -388,7 +413,7 @@ func (l *internalLink) SendBlocking(p *router.Packet) {
 // Because this link is not associated with a specific remote address, the src
 // address of the packet is recorded in the packet structure. This may be used
 // as the destination if SCMP responds.
-func (l *internalLink) receive(srcAddr *netip.AddrPort, p *router.Packet) {
+func (l *internalLink) receive(p *router.Packet) {
 	metrics := l.metrics
 	sc := router.ClassOfSize(len(p.RawPacket))
 	metrics[sc].InputPacketsTotal.Inc()
@@ -403,10 +428,6 @@ func (l *internalLink) receive(srcAddr *netip.AddrPort, p *router.Packet) {
 
 	p.Link = l
 
-	// This is an unconnected link. We must record the src address in case the packet is turned
-	// around by SCMP.
-	p.RemoteAddr = unsafe.Pointer(srcAddr)
-
 	select {
 	case l.procQs[procID] <- p:
 	default:

router/underlayproviders/afpacketudpip/ptplink.go

@@ -364,7 +364,7 @@ func (l *ptpLink) SendBlocking(p *router.Packet) {
 }
 
 // receive delivers an incoming packet to the appropriate processing queue.
-func (l *ptpLink) receive(_ *netip.AddrPort, p *router.Packet) {
+func (l *ptpLink) receive(p *router.Packet) {
 	metrics := l.metrics
 	sc := router.ClassOfSize(len(p.RawPacket))
 	metrics[sc].InputPacketsTotal.Inc()

router/underlayproviders/afpacketudpip/udpconnection.go

@@ -235,6 +235,7 @@ func (u *udpConnection) receive(pool router.PacketPool) {
 		var ipv6Layer layers.IPv6
 		var udpLayer layers.UDP
 		var srcDst fourTuple
+		var srcIPBytes []byte
 		var validIP bool
 
 		// Since it may be recycled...
@@ -268,6 +269,7 @@ func (u *udpConnection) receive(pool router.PacketPool) {
 				// WTF?
 				continue
 			}
+			srcIPBytes = ipv4Layer.SrcIP
 			srcDst.dst.ip, validIP = netip.AddrFromSlice(ipv4Layer.DstIP)
 			if !validIP {
 				// WTF?
@@ -284,6 +286,7 @@ func (u *udpConnection) receive(pool router.PacketPool) {
 				// WTF?
 				continue
 			}
+			srcIPBytes = ipv6Layer.DstIP
 			srcDst.dst.ip, validIP = netip.AddrFromSlice(ipv6Layer.DstIP)
 			if !validIP {
 				// WTF?
@@ -318,16 +321,13 @@ func (u *udpConnection) receive(pool router.PacketPool) {
 		srcDst.src.port = uint16(udpLayer.SrcPort)
 		srcDst.dst.port = uint16(udpLayer.DstPort)
 		if l, found := u.ptpLinks[srcDst]; found {
-			l.receive(nil, p)
+			l.receive(p)
 			p = pool.Get() // we need a fresh packet buffer now.
 			continue
 		}
 		if l, found := u.intLinks[srcDst.dst]; found {
-			// TODO(jiceatscion): it is very unfortunate that we end-up allocating a copy of the src
-			// addr even in this implementation. Instead of using a netip.AddrPort, we could point
-			// directly at some space in the packet buffer.
-			srcAddr := netip.AddrPortFrom(srcDst.src.ip, srcDst.src.port) // Escapes.
-			l.receive(&srcAddr, p)
+			setRemoteAddr(p, srcIPBytes, srcDst.src.port)
+			l.receive(p)
 			p = pool.Get() // we need a fresh packet buffer now.
 			continue
 		}

router/underlayproviders/udpip/udpip.go

@@ -921,7 +921,8 @@ func (l *internalLink) Resolve(p *router.Packet, dst addr.Host, port uint16) err
 	// Packets that get here must have come from an external or a sibling link; neither of which
 	// attach a RemoteAddr to the packet (besides; it could be a different type).  So, RemoteAddr is
 	// not generally usable. We must allocate a new object. The precautions needed to pool them cost
-	// more than the pool saves (verified experimentally).
+	// more than the pool saves (verified experimentally). We should do like the afpacket underlay
+	// and store the bits at the head of the packet buffer instead.
 	p.RemoteAddr = unsafe.Pointer(&net.UDPAddr{
 		IP:   dstAddr.AsSlice(),
 		Zone: dstAddr.Zone(),