Avoid lock acquisition on every loop iteration

packetBufferQueue.dequeue acquires and releases the queue lock; unwrap
the function into its only caller so that the lock can be acquired only
once over many loop iterations.

Remove packetBufferQueue as it is no longer a meaningful abstraction.

PiperOrigin-RevId: 418712739

pkg/tcpip/link/qdisc/fifo/BUILD

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

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

@@ -40,8 +40,15 @@ type discipline struct {
 // queue. It will also smartly batch packets when possible and write them
 // through the lower LinkWriter.
 type queueDispatcher struct {
-	lower          stack.LinkWriter
-	queue          packetBufferQueue
+	lower stack.LinkWriter
+	limit int
+
+	mu sync.Mutex
+	// +checklocks:mu
+	queue stack.PacketBufferList
+	// +checklocks:mu
+	used int
+
 	newPacketWaker sleep.Waker
 	closeWaker     sleep.Waker
 }
@@ -56,7 +63,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.queue.limit = queueLen
+		qd.limit = queueLen
 
 		d.wg.Add(1)
 		go func() {
@@ -67,33 +74,39 @@ func New(lower stack.LinkWriter, n int, queueLen int) stack.QueueingDiscipline {
 	return d
 }
 
-func (q *queueDispatcher) dispatchLoop() {
+func (qd *queueDispatcher) dispatchLoop() {
 	s := sleep.Sleeper{}
-	s.AddWaker(&q.newPacketWaker)
-	s.AddWaker(&q.closeWaker)
+	s.AddWaker(&qd.newPacketWaker)
+	s.AddWaker(&qd.closeWaker)
 	defer s.Done()
 
 	const batchSize = 32
 	var batch stack.PacketBufferList
 	for {
 		switch w := s.Fetch(true); w {
-		case &q.newPacketWaker:
-		case &q.closeWaker:
+		case &qd.newPacketWaker:
+		case &qd.closeWaker:
 			return
 		default:
 			panic("unknown waker")
 		}
-		for pkt := q.queue.dequeue(); pkt != nil; pkt = q.queue.dequeue() {
-			batch.PushBack(pkt)
-			if batch.Len() < batchSize && !q.queue.empty() {
-				continue
+		qd.mu.Lock()
+		for batch.Len() < batchSize {
+			pkt := qd.queue.Front()
+			if pkt == nil {
+				break
 			}
-			// We pass a protocol of zero here because each packet carries its
-			// NetworkProtocol.
-			q.lower.WritePackets(stack.RouteInfo{}, batch, 0 /* protocol */)
-			batch.DecRef()
-			batch.Reset()
+			qd.queue.Remove(pkt)
+			qd.used--
+			batch.PushBack(pkt)
 		}
+		qd.mu.Unlock()
+
+		// We pass a protocol of zero here because each packet carries its
+		// NetworkProtocol.
+		_, _ = qd.lower.WritePackets(stack.RouteInfo{}, batch, 0 /* protocol */)
+		batch.DecRef()
+		batch.Reset()
 	}
 }
 
@@ -105,7 +118,15 @@ func (q *queueDispatcher) dispatchLoop() {
 //  - pkt.NetworkProtocolNumber
 func (d *discipline) WritePacket(_ stack.RouteInfo, _ tcpip.NetworkProtocolNumber, pkt *stack.PacketBuffer) tcpip.Error {
 	qd := &d.dispatchers[int(pkt.Hash)%len(d.dispatchers)]
-	if !qd.queue.enqueue(pkt) {
+	qd.mu.Lock()
+	haveSpace := qd.used < qd.limit
+	if haveSpace {
+		pkt.IncRef()
+		qd.queue.PushBack(pkt)
+		qd.used++
+	}
+	qd.mu.Unlock()
+	if !haveSpace {
 		return &tcpip.ErrNoBufferSpace{}
 	}
 	qd.newPacketWaker.Assert()

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

@@ -1,84 +0,0 @@
-// Copyright 2020 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/sync"
-	"gvisor.dev/gvisor/pkg/tcpip/stack"
-)
-
-// packetBufferQueue is a bounded, thread-safe queue of PacketBuffers.
-//
-type packetBufferQueue struct {
-	mu    sync.Mutex
-	list  stack.PacketBufferList
-	limit int
-	used  int
-}
-
-// emptyLocked determines if the queue is empty.
-// Preconditions: q.mu must be held.
-func (q *packetBufferQueue) emptyLocked() bool {
-	return q.used == 0
-}
-
-// empty determines if the queue is empty.
-func (q *packetBufferQueue) empty() bool {
-	q.mu.Lock()
-	r := q.emptyLocked()
-	q.mu.Unlock()
-
-	return r
-}
-
-// setLimit updates the limit. No PacketBuffers are immediately dropped in case
-// the queue becomes full due to the new limit.
-func (q *packetBufferQueue) setLimit(limit int) {
-	q.mu.Lock()
-	q.limit = limit
-	q.mu.Unlock()
-}
-
-// enqueue adds the given packet to the queue.
-//
-// Returns true when the PacketBuffer is successfully added to the queue, in
-// which case the queue acquires a reference to the PacketBuffer, and
-// returns false if the queue is full.
-func (q *packetBufferQueue) enqueue(s *stack.PacketBuffer) bool {
-	q.mu.Lock()
-	r := q.used < q.limit
-	if r {
-		s.IncRef()
-		q.list.PushBack(s)
-		q.used++
-	}
-	q.mu.Unlock()
-
-	return r
-}
-
-// dequeue removes and returns the next PacketBuffer from queue, if one exists.
-// Caller is responsible for calling DecRef on the PacketBuffer.
-func (q *packetBufferQueue) dequeue() *stack.PacketBuffer {
-	q.mu.Lock()
-	s := q.list.Front()
-	if s != nil {
-		q.list.Remove(s)
-		q.used--
-	}
-	q.mu.Unlock()
-
-	return s
-}