perf/fix(network): cut policy apply latency and harden TAP create under density #1318
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| package cubevs | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "sync" | ||
|
|
||
| "github.com/cilium/ebpf" | ||
| ) | ||
|
|
||
| // policyInnerCache keeps open FDs for HashOfMaps inners keyed by outer map name | ||
| // and TAP ifindex. Userspace BPF_MAP_LOOKUP_ELEM on HashOfMaps is extremely | ||
| // expensive on large hosts; create/apply/reaper paths must reuse cached inners | ||
| // instead of looking up the outer map on every call. | ||
| // | ||
| // FD budget: each live TAP may pin up to one FD per net-policy outer | ||
| // (allow_out_v2, deny_out, dns_allow) — about 3 × active-TAP-count on Cubelet, | ||
| // plus any Active ifindexes warmed by the DNS reaper. Entries are released only | ||
| // on TAP destroy / startup stale-outer GC; there is no size-bounded eviction. | ||
| // There is also no BPF-reload generation counter today (no reload path); if | ||
| // pinned outers are ever recreated, call clearPolicyInnerCacheForTest-style | ||
| // invalidation (or add a generation) before reuse. | ||
| type policyInnerKey struct { | ||
| mapName string | ||
| ifindex uint32 | ||
| } | ||
|
|
||
| var policyInnerMaps sync.Map // policyInnerKey -> *ebpf.Map | ||
|
|
||
| // acquireInnerMap returns the inner map for ifindex, creating it when missing | ||
| // and newInner is non-nil. The returned map is owned by the process-wide cache; | ||
| // callers must not Close it. A nil newInner means "must already exist". | ||
| // | ||
| // Cubelet completes stale-outer GC synchronously before starting background TAP | ||
| // work or serving requests, and its TAP lifecycle prevents concurrent ownership | ||
| // of one ifindex. This cache therefore does not add another per-key lock. | ||
| func acquireInnerMap(outerMap *ebpf.Map, ifindex uint32, mapName string, | ||
|
There was a problem hiding this comment. Design note: unbounded FD cache with no invalidation hook, and no direct unit tests. Two things worth addressing before this lands:
Also: this is the highest-risk new file in the PR (per-key locking + FD ownership invariants) but has no direct unit tests — the invariants are only exercised indirectly through netpolicy/dnspolicy tests. A focused test for the lock-ordering / cache-vs-outer consistency would be valuable. |
||
| newInner func() (*ebpf.Map, error), | ||
| ) (*ebpf.Map, error) { | ||
| key := policyInnerKey{mapName: mapName, ifindex: ifindex} | ||
| if cached, ok := policyInnerMaps.Load(key); ok { | ||
|
There was a problem hiding this comment. HIGH — stale-cache TOCTOU: a reused ifindex can silently lose all allow/deny policy. The cache-hit path returns the cached inner without verifying the outer HashOfMaps still maps this ifindex to it. The DNS reaper (
Nothing evicts it (destroy already ran its eviction; GC only runs at startup). When the kernel reuses ifindex X for a new TAP, The comment at lines 34–36 justifies skipping a per-key lock with the "no concurrent ownership of one ifindex" lifecycle invariant — but that invariant covers two creators of the same ifindex, not a background reader racing a destroy, which is precisely what the reaper now is. Suggested fix: per-key serialization around acquire/release (per-key mutex, or lock-per-key via |
||
| return cached.(*ebpf.Map), nil | ||
| } | ||
|
|
||
| var inner *ebpf.Map | ||
| err := outerMap.Lookup(&ifindex, &inner) | ||
| if err == nil { | ||
| actual, loaded := policyInnerMaps.LoadOrStore(key, inner) | ||
| if loaded { | ||
| _ = inner.Close() | ||
| return actual.(*ebpf.Map), nil | ||
| } | ||
| return inner, nil | ||
| } | ||
| if !errors.Is(err, ebpf.ErrKeyNotExist) { | ||
| return nil, fmt.Errorf("map.Lookup failed: %w, name: %s", err, mapName) | ||
| } | ||
| if newInner == nil { | ||
| return nil, fmt.Errorf("map.Lookup failed: %w, name: %s", ebpf.ErrKeyNotExist, mapName) | ||
| } | ||
|
|
||
| created, err := newInner() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if err := outerMap.Put(&ifindex, created); err != nil { | ||
|
There was a problem hiding this comment. Concurrency hazard in the create path: two goroutines that both miss the cache for the same Interleaving:
End state: the process-wide cache holds Worth serializing creation per There was a problem hiding this comment. The create-and-Put critical section is unsynchronized per key. Two concurrent Separately, a |
||
| _ = created.Close() | ||
| return nil, fmt.Errorf("map.Put failed: %w, name: %s", err, mapName) | ||
| } | ||
| actual, loaded := policyInnerMaps.LoadOrStore(key, created) | ||
|
There was a problem hiding this comment. MEDIUM — creation branch is only safe under an unenforced external invariant. If two goroutines create the same Today the only concurrent background reader (the reaper) passes There was a problem hiding this comment. Create race: if two goroutines both miss the cache and both observe |
||
| if loaded { | ||
| _ = created.Close() | ||
| return actual.(*ebpf.Map), nil | ||
| } | ||
| return created, nil | ||
| } | ||
|
|
||
| // releaseCachedInner closes and drops a cached inner FD. | ||
| func releaseCachedInner(mapName string, ifindex uint32) { | ||
| key := policyInnerKey{mapName: mapName, ifindex: ifindex} | ||
| if cached, ok := policyInnerMaps.LoadAndDelete(key); ok { | ||
| _ = cached.(*ebpf.Map).Close() | ||
| } | ||
| } | ||
|
|
||
| // deleteCachedInnerAndOuter removes the outer key and its cached userspace FD. | ||
| func deleteCachedInnerAndOuter(outer *ebpf.Map, mapName string, ifindex uint32) error { | ||
| key := policyInnerKey{mapName: mapName, ifindex: ifindex} | ||
|
There was a problem hiding this comment. Finding (Medium, PLAUSIBLE): use-after-close on a shared cached FD — the 5s DNS reaper can hold a pointer to an inner that
Pre-PR each reaper pass opened its own FD via |
||
| var toClose *ebpf.Map | ||
| if cached, ok := policyInnerMaps.LoadAndDelete(key); ok { | ||
| toClose = cached.(*ebpf.Map) | ||
| } | ||
| err := outer.Delete(&ifindex) | ||
| if toClose != nil { | ||
| _ = toClose.Close() | ||
| } | ||
| if err != nil && !errors.Is(err, ebpf.ErrKeyNotExist) { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func clearPolicyInnerCacheForTest() { | ||
|
There was a problem hiding this comment.
|
||
| policyInnerMaps.Range(func(k, v any) bool { | ||
| policyInnerMaps.Delete(k) | ||
| if m, ok := v.(*ebpf.Map); ok && m != nil { | ||
| _ = m.Close() | ||
| } | ||
| return true | ||
| }) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finding (Low, PLAUSIBLE): the fast path only reaps ifindices present in
ifindex_to_mvmmeta; theallow_out_v3outer scan is now reachable only when the metadata map fails to load, not when an ifindex is simply missing from it.If the "metadata deleted ⟺ allow_out flushed" invariant is ever violated (crash or failed cleanup between the two steps, partial pool cleanup failure), an allow_out_v3 inner whose ifindex is absent from mvmmeta will never have its expired DNS-learned entries reaped — the previous outer-scan covered every outer key. The fallback covers map-load failures only. Low likelihood given the invariant, but it's a silent robustness regression in exactly the partial-failure modes the old code tolerated. Consider also reaping allow_out outers whose ifindex is not in the metadata set, or documenting/asserting that the invariant is enforced atomically.