Automated rollback of changelist 531020857

PiperOrigin-RevId: 532863869

pkg/tcpip/link/channel/channel.go

@@ -254,7 +254,7 @@ func (e *Endpoint) LinkAddress() tcpip.LinkAddress {
 // Multiple concurrent calls are permitted.
 func (e *Endpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.Error) {
 	n := 0
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for _, pkt := range pkts.AsSlice() {
 		if err := e.q.Write(pkt); err != nil {
 			if _, ok := err.(*tcpip.ErrNoBufferSpace); !ok && n == 0 {
 				return 0, err

pkg/tcpip/link/fdbased/endpoint.go

@@ -700,7 +700,7 @@ func (e *endpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.Error)
 	batch := make([]stack.PacketBufferPtr, 0, BatchSize)
 	batchFDInfo := fdInfo{fd: -1, isSocket: false}
 	sentPackets := 0
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for _, pkt := range pkts.AsSlice() {
 		if len(batch) == 0 {
 			batchFDInfo = e.fds[pkt.Hash%uint32(len(e.fds))]
 		}

pkg/tcpip/link/loopback/loopback.go

@@ -87,7 +87,7 @@ func (e *endpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.Error)
 	e.mu.RLock()
 	d := e.dispatcher
 	e.mu.RUnlock()
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for _, pkt := range pkts.AsSlice() {
 		// In order to properly loop back to the inbound side we must create a
 		// fresh packet that only contains the underlying payload with no headers
 		// or struct fields set.

pkg/tcpip/link/muxed/injectable.go

@@ -102,14 +102,14 @@ func (m *InjectableEndpoint) InjectInbound(protocol tcpip.NetworkProtocolNumber,
 // pkt.EgressRoute.RemoteAddress has a route registered in this endpoint.
 func (m *InjectableEndpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.Error) {
 	i := 0
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for _, pkt := range pkts.AsSlice() {
 		endpoint, ok := m.routes[pkt.EgressRoute.RemoteAddress]
 		if !ok {
 			return i, &tcpip.ErrHostUnreachable{}
 		}
 
 		var tmpPkts stack.PacketBufferList
-		tmpPkts.PushFront(pkt)
+		tmpPkts.PushBack(pkt)
 
 		n, err := endpoint.WritePackets(tmpPkts)
 		if err != nil {

pkg/tcpip/link/packetsocket/packetsocket.go

@@ -48,7 +48,7 @@ func (e *endpoint) DeliverNetworkPacket(protocol tcpip.NetworkProtocolNumber, pk
 
 // WritePackets implements stack.LinkEndpoint.
 func (e *endpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.Error) {
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for _, pkt := range pkts.AsSlice() {
 		e.Endpoint.DeliverLinkPacket(pkt.NetworkProtocolNumber, pkt)
 	}
 

pkg/tcpip/link/pipe/pipe.go

@@ -57,7 +57,7 @@ func (e *Endpoint) deliverPackets(pkts stack.PacketBufferList) {
 		return
 	}
 
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for _, pkt := range pkts.AsSlice() {
 		// Create a fresh packet with pkt's payload but without struct fields
 		// or headers set so the next link protocol can properly set the link
 		// header.

pkg/tcpip/link/qdisc/fifo/BUILD

@@ -9,6 +9,7 @@ go_library(
     name = "fifo",
     srcs = [
         "fifo.go",
+        "packet_buffer_circular_list.go",
     ],
     visibility = ["//visibility:public"],
     deps = [

pkg/tcpip/link/qdisc/fifo/fifo.go

@@ -53,13 +53,10 @@ type discipline struct {
 // through the lower LinkWriter.
 type queueDispatcher struct {
 	lower stack.LinkWriter
-	limit int
 
 	mu sync.Mutex
 	// +checklocks:mu
-	queue stack.PacketBufferList
-	// +checklocks:mu
-	used int
+	queue packetBufferCircularList
 
 	newPacketWaker sleep.Waker
 	closeWaker     sleep.Waker
@@ -67,6 +64,8 @@ type queueDispatcher struct {
 
 // New creates a new fifo queuing discipline with the n queues with maximum
 // capacity of queueLen.
+//
+// +checklocksignore: we don't have to hold locks during initialization.
 func New(lower stack.LinkWriter, n int, queueLen int) stack.QueueingDiscipline {
 	d := &discipline{
 		dispatchers: make([]queueDispatcher, n),
@@ -75,7 +74,7 @@ func New(lower stack.LinkWriter, n int, queueLen int) stack.QueueingDiscipline {
 	for i := range d.dispatchers {
 		qd := &d.dispatchers[i]
 		qd.lower = lower
-		qd.limit = queueLen
+		qd.queue.init(queueLen)
 
 		d.wg.Add(1)
 		go func() {
@@ -98,28 +97,23 @@ func (qd *queueDispatcher) dispatchLoop() {
 		case &qd.newPacketWaker:
 		case &qd.closeWaker:
 			qd.mu.Lock()
-			for p := qd.queue.Front(); !p.IsNil(); p = qd.queue.Front() {
-				qd.queue.Remove(p)
+			for p := qd.queue.removeFront(); !p.IsNil(); p = qd.queue.removeFront() {
 				p.DecRef()
-				qd.used--
 			}
-			qd.queue.DecRef()
+			qd.queue.decRef()
 			qd.mu.Unlock()
 			return
 		default:
 			panic("unknown waker")
 		}
 		qd.mu.Lock()
-		for pkt := qd.queue.Front(); !pkt.IsNil(); pkt = qd.queue.Front() {
-			qd.queue.Remove(pkt)
-			qd.used--
+		for pkt := qd.queue.removeFront(); !pkt.IsNil(); pkt = qd.queue.removeFront() {
 			batch.PushBack(pkt)
-			if batch.Len() < BatchSize && qd.used != 0 {
+			if batch.Len() < BatchSize && !qd.queue.isEmpty() {
 				continue
 			}
 			qd.mu.Unlock()
 			_, _ = qd.lower.WritePackets(batch)
-			batch.DecRef()
 			batch.Reset()
 			qd.mu.Lock()
 		}
@@ -139,10 +133,9 @@ func (d *discipline) WritePacket(pkt stack.PacketBufferPtr) tcpip.Error {
 	}
 	qd := &d.dispatchers[int(pkt.Hash)%len(d.dispatchers)]
 	qd.mu.Lock()
-	haveSpace := qd.used < qd.limit
+	haveSpace := qd.queue.hasSpace()
 	if haveSpace {
-		qd.queue.PushBack(pkt.IncRef())
-		qd.used++
+		qd.queue.pushBack(pkt.IncRef())
 	}
 	qd.mu.Unlock()
 	if !haveSpace {

pkg/tcpip/link/qdisc/fifo/packet_buffer_circular_list.go

@@ -0,0 +1,93 @@
+// Copyright 2022 The gVisor Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at //
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package fifo
+
+import "gvisor.dev/gvisor/pkg/tcpip/stack"
+
+// packetBufferCircularList is a slice-backed circular list. All operations are
+// O(1) unless otherwise noted. It only allocates once, during the call to
+// init().
+//
+// Users should call init() before using packetBufferCircularList.
+//
+// +stateify savable
+type packetBufferCircularList struct {
+	pbs  []stack.PacketBufferPtr
+	head int
+	size int
+}
+
+// init initializes the list with the given size.
+func (pl *packetBufferCircularList) init(size int) {
+	pl.pbs = make([]stack.PacketBufferPtr, size)
+}
+
+// length returns the number of elements in the list.
+//
+//go:nosplit
+func (pl *packetBufferCircularList) length() int {
+	return pl.size
+}
+
+// hasSpace returns whether there is space left in the list.
+//
+//go:nosplit
+func (pl *packetBufferCircularList) hasSpace() bool {
+	return pl.size < len(pl.pbs)
+}
+
+// isEmpty returns whether the list is empty.
+//
+//go:nosplit
+func (pl *packetBufferCircularList) isEmpty() bool {
+	return pl.size == 0
+}
+
+// pushBack inserts the PacketBuffer at the end of the list.
+//
+// Users must check beforehand that there is space via a call to hasSpace().
+// Failing to do so may clobber existing entries.
+//
+//go:nosplit
+func (pl *packetBufferCircularList) pushBack(pb stack.PacketBufferPtr) {
+	next := (pl.head + pl.size) % len(pl.pbs)
+	pl.pbs[next] = pb
+	pl.size++
+}
+
+// removeFront returns the first element of the list or nil.
+//
+//go:nosplit
+func (pl *packetBufferCircularList) removeFront() stack.PacketBufferPtr {
+	if pl.isEmpty() {
+		return nil
+	}
+	ret := pl.pbs[pl.head]
+	pl.pbs[pl.head] = nil
+	pl.head = (pl.head + 1) % len(pl.pbs)
+	pl.size--
+	return ret
+}
+
+// decRef decreases the reference count on each stack.PacketBuffer stored in
+// the list.
+//
+// NOTE: runs in O(n) time.
+//
+//go:nosplit
+func (pl *packetBufferCircularList) decRef() {
+	for i := 0; i < pl.size; i++ {
+		pl.pbs[(pl.head+i)%len(pl.pbs)].DecRef()
+	}
+}

pkg/tcpip/link/sharedmem/sharedmem.go

@@ -369,7 +369,7 @@ func (e *endpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.Error)
 	var err tcpip.Error
 	e.mu.Lock()
 	defer e.mu.Unlock()
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for _, pkt := range pkts.AsSlice() {
 		if err = e.writePacketLocked(pkt.EgressRoute, pkt.NetworkProtocolNumber, pkt); err != nil {
 			break
 		}

pkg/tcpip/link/sharedmem/sharedmem_server.go

@@ -256,7 +256,7 @@ func (e *serverEndpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.E
 	var err tcpip.Error
 	e.mu.Lock()
 	defer e.mu.Unlock()
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for _, pkt := range pkts.AsSlice() {
 		if err = e.writePacketLocked(pkt.EgressRoute, pkt.NetworkProtocolNumber, pkt); err != nil {
 			break
 		}

pkg/tcpip/link/sniffer/sniffer.go

@@ -163,7 +163,7 @@ func (e *endpoint) dumpPacket(dir Direction, protocol tcpip.NetworkProtocolNumbe
 // higher-level protocols to write packets; it just logs the packet and
 // forwards the request to the lower endpoint.
 func (e *endpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.Error) {
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for _, pkt := range pkts.AsSlice() {
 		e.dumpPacket(DirectionSend, pkt.NetworkProtocolNumber, pkt)
 	}
 	return e.Endpoint.WritePackets(pkts)

pkg/tcpip/link/xdp/endpoint.go

@@ -279,7 +279,7 @@ func (ep *endpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.Error)
 
 	// Allocate UMEM space. In order to release the UMEM lock as soon as
 	// possible, we allocate up-front and copy data in after releasing.
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for _, pkt := range pkts.AsSlice() {
 		batch = append(batch, unix.XDPDesc{
 			Addr: ep.control.UMEM.AllocFrame(),
 			Len:  uint32(pkt.Size()),
@@ -287,8 +287,7 @@ func (ep *endpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.Error)
 	}
 	ep.control.UMEM.Unlock()
 
-	i := 0
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for i, pkt := range pkts.AsSlice() {
 		// Copy packets into UMEM frame.
 		frame := ep.control.UMEM.Get(batch[i])
 		offset := 0
@@ -296,7 +295,6 @@ func (ep *endpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.Error)
 			offset += copy(frame[offset:], buf)
 		}
 		ep.control.TX.Set(index+uint32(i), batch[i])
-		i++
 	}
 
 	// Notify the kernel that there're packets to write.

pkg/tcpip/network/internal/testutil/testutil.go

@@ -68,7 +68,7 @@ func (*MockLinkEndpoint) LinkAddress() tcpip.LinkAddress { return "" }
 // WritePackets implements LinkEndpoint.WritePackets.
 func (ep *MockLinkEndpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.Error) {
 	var n int
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for _, pkt := range pkts.AsSlice() {
 		if ep.allowPackets == 0 {
 			return n, ep.err
 		}

pkg/tcpip/stack/BUILD

@@ -160,18 +160,6 @@ go_template_instance(
     },
 )
 
-go_template_instance(
-    name = "packet_buffer_list",
-    out = "packet_buffer_list.go",
-    package = "stack",
-    prefix = "PacketBuffer",
-    template = "//pkg/ilist:generic_list",
-    types = {
-        "Element": "*PacketBuffer",
-        "Linker": "*PacketBuffer",
-    },
-)
-
 go_template_instance(
     name = "tuple_list",
     out = "tuple_list.go",

pkg/tcpip/stack/forwarding_test.go

@@ -308,7 +308,7 @@ func (e *fwdTestLinkEndpoint) LinkAddress() tcpip.LinkAddress {
 // WritePackets stores outbound packets into the channel.
 func (e *fwdTestLinkEndpoint) WritePackets(pkts PacketBufferList) (int, tcpip.Error) {
 	n := 0
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for _, pkt := range pkts.AsSlice() {
 		select {
 		case e.C <- pkt:
 		default:

pkg/tcpip/stack/packet_buffer.go

@@ -108,10 +108,6 @@ type PacketBuffer struct {
 
 	packetBufferRefs
 
-	// PacketBufferEntry is used to build an intrusive list of
-	// PacketBuffers.
-	PacketBufferEntry
-
 	// buf is the underlying buffer for the packet. See struct level docs for
 	// details.
 	buf      bufferv2.Buffer
@@ -370,7 +366,6 @@ func (pk PacketBufferPtr) headerView(typ headerType) bufferv2.View {
 func (pk PacketBufferPtr) Clone() PacketBufferPtr {
 	newPk := pkPool.Get().(*PacketBuffer)
 	newPk.reset()
-	newPk.PacketBufferEntry = pk.PacketBufferEntry
 	newPk.buf = pk.buf.Clone()
 	newPk.reserved = pk.reserved
 	newPk.pushed = pk.pushed
@@ -470,28 +465,6 @@ func (pk PacketBufferPtr) IsNil() bool {
 	return pk == nil
 }
 
-// IncRef increases the reference count on each PacketBuffer
-// stored in the PacketBufferList.
-func (pk *PacketBufferList) IncRef() {
-	for pb := pk.Front(); pb != nil; pb = pb.Next() {
-		pb.IncRef()
-	}
-}
-
-// DecRef decreases the reference count on each PacketBuffer
-// stored in the PacketBufferList.
-func (pk *PacketBufferList) DecRef() {
-	// Using a while-loop here (instead of for-loop) because DecRef() can cause
-	// the pb to be recycled. If it is recycled during execution of this loop,
-	// there is a possibility of a data race during a call to pb.Next().
-	pb := pk.Front()
-	for pb != nil {
-		next := pb.Next()
-		pb.DecRef()
-		pb = next
-	}
-}
-
 // headerInfo stores metadata about a header in a packet.
 //
 // +stateify savable

pkg/tcpip/stack/packet_buffer_list.go

@@ -0,0 +1,75 @@
+// Copyright 2022 The gVisor Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at //
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package stack
+
+// PacketBufferList is a slice-backed list. All operations are O(1) unless
+// otherwise noted.
+//
+// Note: this is intentionally backed by a slice, not an intrusive list. We've
+// switched PacketBufferList back-and-forth between intrusive list and
+// slice-backed implementations, and the latter has proven to be preferable:
+//
+//   - Intrusive lists are a refcounting nightmare, as modifying the list
+//     sometimes-but-not-always modifies the list for others.
+//   - The slice-backed implementation has been benchmarked and is slightly more
+//     performant.
+//
+// +stateify savable
+type PacketBufferList struct {
+	pbs []*PacketBuffer
+}
+
+// AsSlice returns a slice containing the packets in the list.
+//
+//go:nosplit
+func (pl *PacketBufferList) AsSlice() []*PacketBuffer {
+	return pl.pbs
+}
+
+// Reset decrements all elements and resets the list to the empty state.
+//
+//go:nosplit
+func (pl *PacketBufferList) Reset() {
+	for i, pb := range pl.pbs {
+		pb.DecRef()
+		pl.pbs[i] = nil
+	}
+	pl.pbs = pl.pbs[:0]
+}
+
+// Len returns the number of elements in the list.
+//
+//go:nosplit
+func (pl *PacketBufferList) Len() int {
+	return len(pl.pbs)
+}
+
+// PushBack inserts the PacketBuffer at the back of the list.
+//
+//go:nosplit
+func (pl *PacketBufferList) PushBack(pb *PacketBuffer) {
+	pl.pbs = append(pl.pbs, pb)
+}
+
+// DecRef decreases the reference count on each PacketBuffer
+// stored in the list.
+//
+// NOTE: runs in O(n) time.
+//
+//go:nosplit
+func (pl PacketBufferList) DecRef() {
+	for _, pb := range pl.pbs {
+		pb.DecRef()
+	}
+}

pkg/tcpip/tests/integration/iptables_test.go

@@ -635,10 +635,7 @@ func TestIPTableWritePackets(t *testing.T) {
 			defer r.Release()
 
 			pkts := test.genPacket(r)
-			pktsLen := pkts.Len()
-			for i := 0; i < pktsLen; i++ {
-				pkt := pkts.Front()
-				pkts.Remove(pkt)
+			for _, pkt := range pkts.AsSlice() {
 				if err := r.WritePacket(stack.NetworkHeaderParams{
 					Protocol: header.UDPProtocolNumber,
 					TTL:      64,

pkg/tcpip/tests/integration/link_resolution_test.go

@@ -1698,7 +1698,7 @@ func newMonitorableLinkEndpoint(e stack.LinkEndpoint) *monitorableLinkEndpoint {
 }
 
 func (e *monitorableLinkEndpoint) WritePackets(pkts stack.PacketBufferList) (int, tcpip.Error) {
-	for pkt := pkts.Front(); pkt != nil; pkt = pkt.Next() {
+	for _, pkt := range pkts.AsSlice() {
 		dstAddr := header.Ethernet(pkt.LinkHeader().Slice()).DestinationAddress()
 		e.ch <- dstAddr
 	}