From 9510e0939a203d8e9fad28b25eeac7121708fa9b Mon Sep 17 00:00:00 2001
From: Kevin Krakauer <krakauer@google.com>
Date: Thu, 1 Jun 2023 15:22:20 -0700
Subject: [PATCH] netstack: speed up merge

Merge clears the other buffer, so rather than cloning and dealing with
refcounts, we can just steal other's view list and adjust sizes.

This increases the speed of Merge 10x, and this was with a benchmark that
didn't do any TrimFront/Truncate (which will induce copying in the old
implementation).

BenchmarkMerge
BenchmarkMerge-33               99412884                12.74 ns/op            0 B/op          0 allocs/op
BenchmarkOldMerge
BenchmarkOldMerge-33             6075222               217.4 ns/op             0 B/op          0 allocs/op

This change uncovered that PacketBuffer.DeepCopyForForwarding wasn't doing a
true deep copy. It seems that, in the ICMP error path, a call to Merge was
forcing a deep copy (maybe because Append was copying bytes), obfuscating the
issue. This change also addresses that issue by having DeepCopyForForwarding
produce a copy that shares no data with other packets.

PiperOrigin-RevId: 537143838
---
 pkg/buffer/buffer.go             | 18 +++++++++++++-----
 pkg/tcpip/stack/packet_buffer.go |  4 +++-
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/pkg/buffer/buffer.go b/pkg/buffer/buffer.go
index 1308cc73c..cc663ae78 100644
--- a/pkg/buffer/buffer.go
+++ b/pkg/buffer/buffer.go
@@ -414,6 +414,16 @@ func (b *Buffer) Clone() Buffer {
 	return other
 }
 
+// DeepClone creates a deep clone of b, copying data such that no bytes are
+// shared with any other Buffers.
+func (b *Buffer) DeepClone() Buffer {
+	newBuf := Buffer{}
+	buf := b.Clone()
+	reader := buf.AsBufferReader()
+	newBuf.WriteFromReader(&reader, b.size)
+	return newBuf
+}
+
 // Apply applies the given function across all valid data.
 func (b *Buffer) Apply(fn func(*View)) {
 	for v := b.data.Front(); v != nil; v = v.Next() {
@@ -468,13 +478,11 @@ func (b *Buffer) Checksum(offset int) uint16 {
 // The other Buffer will be appended to v, and other will be empty after this
 // operation completes.
 func (b *Buffer) Merge(other *Buffer) {
-	// Copy over all buffers.
-	for v := other.data.Front(); v != nil; v = other.data.Front() {
-		b.Append(v.Clone())
-		other.removeView(v)
-	}
+	b.data.PushBackList(&other.data)
+	other.data = viewList{}
 
 	// Adjust sizes.
+	b.size += other.size
 	other.size = 0
 }
 
diff --git a/pkg/tcpip/stack/packet_buffer.go b/pkg/tcpip/stack/packet_buffer.go
index 9e8567c24..85e6717ec 100644
--- a/pkg/tcpip/stack/packet_buffer.go
+++ b/pkg/tcpip/stack/packet_buffer.go
@@ -433,9 +433,11 @@ func (pk PacketBufferPtr) CloneToInbound() PacketBufferPtr {
 // The returned packet buffer will have the network and transport headers
 // set if the original packet buffer did.
 func (pk PacketBufferPtr) DeepCopyForForwarding(reservedHeaderBytes int) PacketBufferPtr {
+	payload := BufferSince(pk.NetworkHeader())
+	defer payload.Release()
 	newPk := NewPacketBuffer(PacketBufferOptions{
 		ReserveHeaderBytes: reservedHeaderBytes,
-		Payload:            BufferSince(pk.NetworkHeader()),
+		Payload:            payload.DeepClone(),
 		IsForwardedPacket:  true,
 	})