From 0f59f895a426170051341eff73c264788c988201 Mon Sep 17 00:00:00 2001 From: JackDoan Date: Thu, 15 Oct 2026 12:21:59 -0500 Subject: [PATCH] cpupick: Keep single readers off tun.avoid_cpus without cpupick, warn and ignore bad lists --- examples/config.yml | 12 ++++++------ interface.go | 8 ++++++++ interface_test.go | 20 ++++++++++++++++++++ main.go | 33 ++++++++++++++++----------------- main_test.go | 17 ++++------------- 5 files changed, 54 insertions(+), 36 deletions(-) diff --git a/examples/config.yml b/examples/config.yml index 4b1a00fe..4e0529d1 100644 --- a/examples/config.yml +++ b/examples/config.yml @@ -296,12 +296,12 @@ tun: # - 4 # Linux only. avoid_cpus lists CPU IDs the tun readers are never pinned to, exactly as listed (SMT siblings are not - # added for you). The default pin selection skips them, even with a single routine, and a cpu_affinity entry naming - # one is rejected like any other invalid entry. Other threads are not restricted, and the setting does nothing with - # pin_threads: false. Invalid IDs, or IDs covering every allowed CPU, fail startup. Not reloadable. To keep Nebula - # off the underlay NIC's receive IRQ CPUs, find them in /proc/interrupts and /proc/irq//effective_affinity_list, - # add their SMT siblings from /sys/devices/system/cpu/cpu/topology/thread_siblings_list if you want those kept - # clear too, and revisit the list when IRQ placement changes. + # added for you). The default pin selection skips them, and a cpu_affinity entry naming one is rejected like any + # other invalid entry. Other threads are not restricted, and the setting does nothing with pin_threads: false. An + # invalid list, or one covering every allowed CPU, is ignored with a warning. Not reloadable. To keep Nebula off the + # underlay NIC's receive IRQ CPUs, find them in /proc/interrupts and /proc/irq//effective_affinity_list, add + # their SMT siblings from /sys/devices/system/cpu/cpu/topology/thread_siblings_list if you want those kept clear + # too, and revisit the list when IRQ placement changes. #avoid_cpus: [] # Route based MTU overrides, you have known vpn ip paths that can support larger MTUs you can increase/decrease them here diff --git a/interface.go b/interface.go index a0d7487b..538727ef 100644 --- a/interface.go +++ b/interface.go @@ -60,6 +60,9 @@ type InterfaceConfig struct { // shorter lists than `routines` cycle. Empty list keeps the default // pin-to-(i % NumCPU) behavior. Only consulted when PinThreads is true. CpuAffinity []int + // AvoidCPUs (tun.avoid_cpus) lists CPUs the default pin spread skips when + // CpuAffinity is empty. Only consulted when PinThreads is true. + AvoidCPUs []int // PinThreads controls whether each TUN reader OS thread is pinned to a // single CPU (via tun.pin_threads, default true). Pinning keeps each // goroutine's sendmmsg on one XPS-selected NIC TX ring so per-flow @@ -95,6 +98,8 @@ type Interface struct { // Empty falls back to the default pin-to-(allowed CPU) behavior. // Only consulted when pinThreads is true. cpuAffinity []int + // avoidCPUs are removed from that fallback spread, unless they would empty it. + avoidCPUs []int // pinThreads controls whether listenIn pins each TUN reader OS thread to // a CPU at all (tun.pin_threads, default true). When false, threads are // left free to migrate as on stock nebula. @@ -247,6 +252,7 @@ func NewInterface(ctx context.Context, c *InterfaceConfig) (*Interface, error) { connectionManager: c.connectionManager, conntrackCacheTimeout: c.ConntrackCacheTimeout, cpuAffinity: c.CpuAffinity, + avoidCPUs: c.AvoidCPUs, pinThreads: c.PinThreads, udpTxErrLimiter: udpTxErrLimiter, udpRelayTxErrLimiter: udpRelayTxErrLimiter, @@ -443,6 +449,8 @@ func (f *Interface) pinThisThread(i int) { // Default: spread queues across the CPUs we're actually allowed to // run on. Under a cpuset/taskset mask these aren't 0..NumCPU-1, so // i % NumCPU would pick unrunnable IDs and every pin would fail. + // parseAvoidCPUs already dropped an avoid list that covers them all. + allowed = slices.DeleteFunc(allowed, func(c int) bool { return slices.Contains(f.avoidCPUs, c) }) cpu = allowed[i%len(allowed)] } else { cpu = i % runtime.NumCPU() diff --git a/interface_test.go b/interface_test.go index 1b912bbb..975600a0 100644 --- a/interface_test.go +++ b/interface_test.go @@ -7,6 +7,7 @@ import ( "github.com/slackhq/nebula/cert" "github.com/slackhq/nebula/config" "github.com/slackhq/nebula/test" + "github.com/slackhq/nebula/util" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -118,3 +119,22 @@ func TestReloadFirewall_NoChange(t *testing.T) { assert.Same(t, fw, f.firewall, "firewall should not have been replaced") } + +// TestPinThisThreadFallbackSkipsAvoided covers the single reader, which never goes through cpupick: its fallback +// spread over the allowed set must still skip tun.avoid_cpus. +func TestPinThisThreadFallbackSkipsAvoided(t *testing.T) { + allowed, err := util.AllowedCPUs() + require.NoError(t, err) + if len(allowed) < 2 { + t.Skip("requires at least two allowed CPUs") + } + f := &Interface{l: test.NewLogger(), pinThreads: true, avoidCPUs: []int{allowed[0]}} + got := make(chan []int) + go func() { + // The pin leaves this goroutine locked to its thread, so the runtime discards the thread when it returns. + f.pinThisThread(0) + cpus, _ := util.AllowedCPUs() + got <- cpus + }() + assert.Equal(t, []int{allowed[1]}, <-got) +} diff --git a/main.go b/main.go index 9485755a..a3063dcd 100644 --- a/main.go +++ b/main.go @@ -243,10 +243,7 @@ func Main(c *config.C, configTest bool, buildVersion string, l *slog.Logger, dev } pinThreads := c.GetBool("tun.pin_threads", true) - avoidCPUs, err := parseAvoidCPUs(c) - if err != nil { - return nil, err - } + avoidCPUs := parseAvoidCPUs(c, l) if len(avoidCPUs) > 0 && !pinThreads { l.Warn("tun.avoid_cpus has no effect with tun.pin_threads disabled") } @@ -255,12 +252,10 @@ func Main(c *config.C, configTest bool, buildVersion string, l *slog.Logger, dev // return nil, fmt.Errorf("tun.avoid_cpus: %w", err) //} cpuAffinity := parseCpuAffinity(c, l, routines, avoidCPUs) - if pinThreads && (routines > 1 || len(avoidCPUs) > 0) && len(cpuAffinity) == 0 && !configTest { + if pinThreads && routines > 1 && len(cpuAffinity) == 0 && !configTest { // The operator didn't choose pin CPUs, so pick a default set that // prefers performance cores, skips tun.avoid_cpus and doesn't stack - // co-located instances onto allowed[0]. A single reader would - // otherwise just take allowed[0], so it only comes here to dodge - // tun.avoid_cpus. + // co-located instances onto allowed[0]. // key is used to seed the spreading of routines->cores. // use PID if you want to ensure many different Nebulas in VMs or containers land on different cores @@ -308,6 +303,7 @@ func Main(c *config.C, configTest bool, buildVersion string, l *slog.Logger, dev punchy: punchy, ConntrackCacheTimeout: conntrackCacheTimeout, CpuAffinity: cpuAffinity, + AvoidCPUs: avoidCPUs, PinThreads: pinThreads, l: l, } @@ -437,30 +433,33 @@ func parseCpuAffinity(c *config.C, l *slog.Logger, routines int, avoid []int) [] } // parseAvoidCPUs reads tun.avoid_cpus, a list of CPU IDs the tun readers must not be pinned to. IDs outside the -// allowed set are accepted, since one config may serve differently shaped hosts. Unlike tun.cpu_affinity, a bad -// entry, or a list covering every allowed CPU, fails startup rather than being ignored. -func parseAvoidCPUs(c *config.C) ([]int, error) { +// allowed set are accepted, since one config may serve differently shaped hosts. Like tun.cpu_affinity, an invalid +// list is a warning and is ignored entirely, as is one covering every allowed CPU, which would leave nowhere to pin. +func parseAvoidCPUs(c *config.C, l *slog.Logger) []int { raw := c.Get("tun.avoid_cpus") if raw == nil { - return nil, nil + return nil } values, ok := raw.([]any) if !ok { - return nil, fmt.Errorf("tun.avoid_cpus must be a list of non-negative integers") + l.Warn("tun.avoid_cpus must be a list of integers; ignoring", "value", raw) + return nil } cpus := make([]int, 0, len(values)) - for _, value := range values { + for i, value := range values { cpu, ok := value.(int) if !ok || cpu < 0 { - return nil, fmt.Errorf("tun.avoid_cpus must contain non-negative integers, got %v", value) + l.Warn("tun.avoid_cpus entry not a non-negative integer; ignoring avoid_cpus", "index", i, "value", value) + return nil } cpus = append(cpus, cpu) } if allowed, err := util.AllowedCPUs(); err == nil && len(allowed) > 0 && !slices.ContainsFunc(allowed, func(cpu int) bool { return !slices.Contains(cpus, cpu) }) { - return nil, fmt.Errorf("tun.avoid_cpus %v leaves none of the allowed CPUs %v", cpus, allowed) + l.Warn("tun.avoid_cpus covers every allowed CPU; ignoring avoid_cpus", "cpus", cpus, "allowed", allowed) + return nil } - return cpus, nil + return cpus } func moduleVersion() string { diff --git a/main_test.go b/main_test.go index 5ca6a424..eb33eea1 100644 --- a/main_test.go +++ b/main_test.go @@ -67,23 +67,15 @@ func TestParseAvoidCPUs(t *testing.T) { for _, tc := range []struct { value any want []int - valid bool }{ - {nil, nil, true}, {[]any{}, []int{}, true}, {[]any{1, 1 << 20}, []int{1, 1 << 20}, true}, - {"auto", nil, false}, {"1,2", nil, false}, {[]any{-1}, nil, false}, {[]any{1.5}, nil, false}, - {[]any{"2"}, nil, false}, + {nil, nil}, {[]any{}, []int{}}, {[]any{1, 1 << 20}, []int{1, 1 << 20}}, + {"auto", nil}, {"1,2", nil}, {[]any{-1}, nil}, {[]any{1.5}, nil}, {[]any{1, "2"}, nil}, } { c := config.NewC(l) if tc.value != nil { c.Settings["tun"] = map[string]any{"avoid_cpus": tc.value} } - got, err := parseAvoidCPUs(c) - if tc.valid { - require.NoError(t, err) - assert.Equal(t, tc.want, got) - } else { - require.Error(t, err, "%v", tc.value) - } + assert.Equal(t, tc.want, parseAvoidCPUs(c, l), "%v", tc.value) } // avoiding every allowed CPU leaves nowhere to pin; only checkable where the allowed set is known @@ -94,8 +86,7 @@ func TestParseAvoidCPUs(t *testing.T) { } c := config.NewC(l) c.Settings["tun"] = map[string]any{"avoid_cpus": all} - _, err := parseAvoidCPUs(c) - require.Error(t, err) + assert.Nil(t, parseAvoidCPUs(c, l)) } }