From 108885b9e3f91be7f7ec78a7549500b1ca400f84 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Tue, 28 Dec 2021 19:40:40 -0800 Subject: [PATCH] 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 | 1 - pkg/tcpip/link/qdisc/fifo/fifo.go | 57 +++++++++---- .../link/qdisc/fifo/packet_buffer_queue.go | 84 ------------------- 3 files changed, 39 insertions(+), 103 deletions(-) delete mode 100644 pkg/tcpip/link/qdisc/fifo/packet_buffer_queue.go diff --git a/pkg/tcpip/link/qdisc/fifo/BUILD b/pkg/tcpip/link/qdisc/fifo/BUILD index fee5ab450..11db3b85b 100644 --- a/pkg/tcpip/link/qdisc/fifo/BUILD +++ b/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 = [ diff --git a/pkg/tcpip/link/qdisc/fifo/fifo.go b/pkg/tcpip/link/qdisc/fifo/fifo.go index 9fc65c811..73f351ada 100644 --- a/pkg/tcpip/link/qdisc/fifo/fifo.go +++ b/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() diff --git a/pkg/tcpip/link/qdisc/fifo/packet_buffer_queue.go b/pkg/tcpip/link/qdisc/fifo/packet_buffer_queue.go deleted file mode 100644 index 09e5b8314..000000000 --- a/pkg/tcpip/link/qdisc/fifo/packet_buffer_queue.go +++ /dev/null @@ -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 -}