From add538a44f15b449f24f262cdd85844c2b26d6e2 Mon Sep 17 00:00:00 2001 From: Nate Hurley Date: Fri, 13 May 2022 09:11:22 -0700 Subject: [PATCH] Run multicast cleanup routine only when needed. Aside from saving resources, this is also a workaround for the case where timers are bound after the netstack is initialized. Updates #7338. PiperOrigin-RevId: 448509194 --- .../network/internal/multicast/route_table.go | 53 ++++++++++++++++--- .../internal/multicast/route_table_test.go | 37 +++++++------ 2 files changed, 66 insertions(+), 24 deletions(-) diff --git a/pkg/tcpip/network/internal/multicast/route_table.go b/pkg/tcpip/network/internal/multicast/route_table.go index 42722978a..e98ae2d42 100644 --- a/pkg/tcpip/network/internal/multicast/route_table.go +++ b/pkg/tcpip/network/internal/multicast/route_table.go @@ -52,12 +52,14 @@ type RouteTable struct { pendingMu sync.RWMutex // +checklocks:pendingMu pendingRoutes map[RouteKey]PendingRoute - - config Config - // cleanupPendingRoutesTimer is a timer that triggers a routine to remove // pending routes that are expired. + // +checklocks:pendingMu cleanupPendingRoutesTimer tcpip.Timer + // +checklocks:pendingMu + isCleanupRoutineRunning bool + + config Config } var ( @@ -231,7 +233,6 @@ func (r *RouteTable) Init(config Config) error { r.installedRoutes = make(map[RouteKey]*InstalledRoute) r.pendingRoutes = make(map[RouteKey]PendingRoute) - r.cleanupPendingRoutesTimer = r.config.Clock.AfterFunc(DefaultCleanupInterval, r.cleanupPendingRoutes) return nil } @@ -240,19 +241,39 @@ func (r *RouteTable) Init(config Config) error { // Calling this will stop the cleanup routine and release any packets owned by // the table. func (r *RouteTable) Close() { + r.pendingMu.Lock() + defer r.pendingMu.Unlock() + if r.cleanupPendingRoutesTimer != nil { r.cleanupPendingRoutesTimer.Stop() } - r.pendingMu.Lock() - defer r.pendingMu.Unlock() - for key, route := range r.pendingRoutes { delete(r.pendingRoutes, key) route.releasePackets() } } +// maybeStopCleanupRoutine stops the pending routes cleanup routine if no +// pending routes exist. +// +// Returns true if the timer is not running. Otherwise, returns false. +// +// +checklocks:r.pendingMu +func (r *RouteTable) maybeStopCleanupRoutineLocked() bool { + if !r.isCleanupRoutineRunning { + return true + } + + if len(r.pendingRoutes) == 0 { + r.cleanupPendingRoutesTimer.Stop() + r.isCleanupRoutineRunning = false + return true + } + + return false +} + func (r *RouteTable) cleanupPendingRoutes() { currentTime := r.config.Clock.NowMonotonic() r.pendingMu.Lock() @@ -264,7 +285,10 @@ func (r *RouteTable) cleanupPendingRoutes() { route.releasePackets() } } - r.cleanupPendingRoutesTimer.Reset(DefaultCleanupInterval) + + if stopped := r.maybeStopCleanupRoutineLocked(); !stopped { + r.cleanupPendingRoutesTimer.Reset(DefaultCleanupInterval) + } } func (r *RouteTable) newPendingRoute() PendingRoute { @@ -358,6 +382,16 @@ func (r *RouteTable) GetRouteOrInsertPending(key RouteKey, pkt *stack.PacketBuff pendingRoute.packets = append(pendingRoute.packets, pkt.Clone()) r.pendingRoutes[key] = pendingRoute + if !r.isCleanupRoutineRunning { + // The cleanup routine isn't running, but should be. Start it. + if r.cleanupPendingRoutesTimer == nil { + r.cleanupPendingRoutesTimer = r.config.Clock.AfterFunc(DefaultCleanupInterval, r.cleanupPendingRoutes) + } else { + r.cleanupPendingRoutesTimer.Reset(DefaultCleanupInterval) + } + r.isCleanupRoutineRunning = true + } + return GetRouteResult{PendingRouteState: pendingRouteState, InstalledRoute: nil}, nil } @@ -383,6 +417,9 @@ func (r *RouteTable) AddInstalledRoute(key RouteKey, route *InstalledRoute) []*s r.pendingMu.Lock() pendingRoute, ok := r.pendingRoutes[key] delete(r.pendingRoutes, key) + // No need to reset the timer here. The cleanup routine is responsible for + // doing so. + _ = r.maybeStopCleanupRoutineLocked() r.pendingMu.Unlock() // Ignore the pending route if it is expired. It may be in this state since diff --git a/pkg/tcpip/network/internal/multicast/route_table_test.go b/pkg/tcpip/network/internal/multicast/route_table_test.go index 8a60f93cb..8a799a1a2 100644 --- a/pkg/tcpip/network/internal/multicast/route_table_test.go +++ b/pkg/tcpip/network/internal/multicast/route_table_test.go @@ -126,7 +126,7 @@ func TestInit(t *testing.T) { } if !cmp.Equal(err, tc.wantErr, cmpopts.EquateErrors()) { - t.Errorf("got table.Init(%#v) = %s, want %s", tc.config, err, tc.wantErr) + t.Errorf("table.Init(%#v) = %s, want %s", tc.config, err, tc.wantErr) } }) } @@ -147,7 +147,7 @@ func TestNewInstalledRoute(t *testing.T) { expectedRoute := &InstalledRoute{expectedInputInterface: inputNICID, outgoingInterfaces: defaultOutgoingInterfaces, lastUsedTimestamp: clock.NowMonotonic()} if diff := cmp.Diff(expectedRoute, route, cmp.Comparer(installedRouteComparer)); diff != "" { - t.Errorf("installed route mismatch (-want +got):\n%s", diff) + t.Errorf("Installed route mismatch (-want +got):\n%s", diff) } } @@ -167,7 +167,7 @@ func TestPendingRouteStates(t *testing.T) { routeResult, err := table.GetRouteOrInsertPending(defaultRouteKey, pkt) if err != nil { - t.Errorf("got table.GetRouteOrInsertPending(%#v, %#v) = (_, %v), want = (_, nil)", defaultRouteKey, pkt, err) + t.Errorf("table.GetRouteOrInsertPending(%#v, %#v) = (_, %v), want = (_, nil)", defaultRouteKey, pkt, err) } expectedResult := GetRouteResult{PendingRouteState: wantPendingRouteState} @@ -179,7 +179,7 @@ func TestPendingRouteStates(t *testing.T) { // Queuing a third packet should yield an error since the pending queue is // already at max capacity. if _, err := table.GetRouteOrInsertPending(defaultRouteKey, pkt); err != ErrNoBufferSpace { - t.Errorf("got table.GetRouteOrInsertPending(%#v, %#v) = (_, %v), want = (_, ErrNoBufferSpace)", defaultRouteKey, pkt, err) + t.Errorf("table.GetRouteOrInsertPending(%#v, %#v) = (_, %v), want = (_, ErrNoBufferSpace)", defaultRouteKey, pkt, err) } } @@ -233,10 +233,14 @@ func TestPendingRouteExpiration(t *testing.T) { table.pendingMu.RLock() _, ok := table.pendingRoutes[defaultRouteKey] + + if table.isCleanupRoutineRunning != test.wantPendingRoute { + t.Errorf("table.isCleanupRoutineRunning = %t, want = %t", table.isCleanupRoutineRunning, test.wantPendingRoute) + } table.pendingMu.RUnlock() if test.wantPendingRoute != ok { - t.Errorf("got table.pendingRoutes[%#v] = (_, %t), want = (_, %t)", defaultRouteKey, ok, test.wantPendingRoute) + t.Errorf("table.pendingRoutes[%#v] = (_, %t), want = (_, %t)", defaultRouteKey, ok, test.wantPendingRoute) } }) } @@ -283,20 +287,21 @@ func TestAddInstalledRouteWithPending(t *testing.T) { if err := table.Init(config); err != nil { t.Fatalf("table.Init(%#v): %s", config, err) } - // Disable the cleanup routine. - table.cleanupPendingRoutesTimer.Stop() if _, err := table.GetRouteOrInsertPending(defaultRouteKey, pkt); err != nil { t.Fatalf("table.GetRouteOrInsertPending(%#v, %#v): %v", defaultRouteKey, pkt, err) } + // Disable the cleanup routine. + table.cleanupPendingRoutesTimer.Stop() + clock.Advance(test.advance) route := table.NewInstalledRoute(inputNICID, defaultOutgoingInterfaces) pendingPackets := table.AddInstalledRoute(defaultRouteKey, route) if diff := cmp.Diff(test.want, pendingPackets, cmpOpts...); diff != "" { - t.Errorf("tableAddInstalledRoute(%#v, %#v) mismatch (-want +got):\n%s", defaultRouteKey, route, diff) + t.Errorf("table.AddInstalledRoute(%#v, %#v) mismatch (-want +got):\n%s", defaultRouteKey, route, diff) } for _, pendingPkt := range pendingPackets { @@ -306,7 +311,7 @@ func TestAddInstalledRouteWithPending(t *testing.T) { // Verify that the pending route is actually deleted. table.pendingMu.RLock() if pendingRoute, ok := table.pendingRoutes[defaultRouteKey]; ok { - t.Errorf("got table.pendingRoutes[%#v] = (%#v, true), want (_, false)", defaultRouteKey, pendingRoute) + t.Errorf("table.pendingRoutes[%#v] = (%#v, true), want (_, false)", defaultRouteKey, pendingRoute) } table.pendingMu.RUnlock() }) @@ -341,7 +346,7 @@ func TestAddInstalledRouteWithNoPending(t *testing.T) { } if routeResult.PendingRouteState != PendingRouteStateNone { - t.Errorf("got routeResult.PendingRouteState = %s, want = PendingRouteStateNone", routeResult.PendingRouteState) + t.Errorf("routeResult.PendingRouteState = %s, want = PendingRouteStateNone", routeResult.PendingRouteState) } if diff := cmp.Diff(route, routeResult.InstalledRoute, cmp.Comparer(installedRouteComparer)); diff != "" { @@ -363,7 +368,7 @@ func TestRemoveInstalledRoute(t *testing.T) { table.AddInstalledRoute(defaultRouteKey, route) if removed := table.RemoveInstalledRoute(defaultRouteKey); !removed { - t.Errorf("got table.RemoveInstalledRoute(%#v) = false, want = true", defaultRouteKey) + t.Errorf("table.RemoveInstalledRoute(%#v) = false, want = true", defaultRouteKey) } pkt := newPacketBuffer("hello") @@ -376,7 +381,7 @@ func TestRemoveInstalledRoute(t *testing.T) { } if result.InstalledRoute != nil { - t.Errorf("got result.InstalledRoute = %v, want = nil", result.InstalledRoute) + t.Errorf("result.InstalledRoute = %v, want = nil", result.InstalledRoute) } } @@ -389,7 +394,7 @@ func TestRemoveInstalledRouteWithNoMatchingRoute(t *testing.T) { } if removed := table.RemoveInstalledRoute(defaultRouteKey); removed { - t.Errorf("got table.RemoveInstalledRoute(%#v) = true, want = false", defaultRouteKey) + t.Errorf("table.RemoveInstalledRoute(%#v) = true, want = false", defaultRouteKey) } } @@ -402,7 +407,7 @@ func TestGetLastUsedTimestampWithNoMatchingRoute(t *testing.T) { } if _, found := table.GetLastUsedTimestamp(defaultRouteKey); found { - t.Errorf("got table.GetLastUsedTimetsamp(%#v) = (_, true), want = (_, false)", defaultRouteKey) + t.Errorf("table.GetLastUsedTimetsamp(%#v) = (_, true), want = (_, false)", defaultRouteKey) } } @@ -449,11 +454,11 @@ func TestSetLastUsedTimestamp(t *testing.T) { timestamp, found := table.GetLastUsedTimestamp(defaultRouteKey) if !found { - t.Fatalf("got table.GetLastUsedTimestamp(%#v) = (_, false_), want = (_, true)", defaultRouteKey) + t.Fatalf("table.GetLastUsedTimestamp(%#v) = (_, false_), want = (_, true)", defaultRouteKey) } if timestamp != test.wantLastUsedTime { - t.Errorf("got table.GetLastUsedTimestamp(%#v) = (%s, _), want = (%s, _)", defaultRouteKey, timestamp, test.wantLastUsedTime) + t.Errorf("table.GetLastUsedTimestamp(%#v) = (%s, _), want = (%s, _)", defaultRouteKey, timestamp, test.wantLastUsedTime) } }) }