Address ipcache startup perfomance regression

[bimmlerd]

The commit messages should be descriptive, here's the summary line for each.

1. ipcache: add benchmark for Upsert (regression.txt below)
2. ipcache: make NamedPortMultiMap an interface (interface.txt below)
3. ipcache: switch named ports to reference counting (fix.txt below)
4. ipcache: simplify the named ports update code (cleanup.txt below)

The following is a comparison of running the benchmark added in commit 1 over the course of the four commits.

goos: linux
goarch: arm64
pkg: github.com/cilium/cilium/pkg/ipcache
                      │ regression.txt │             interface.txt             │               fix.txt               │             cleanup.txt             │
                      │     sec/op     │     sec/op      vs base               │   sec/op     vs base                │   sec/op     vs base                │
IPCacheUpsert10-10        97.10µ ±  3%     94.58µ ±  3%       ~ (p=0.052 n=10)   48.84µ ± 5%  -49.70% (p=0.000 n=10)   47.59µ ± 5%  -50.98% (p=0.000 n=10)
IPCacheUpsert100-10      2672.3µ ±  5%    2718.4µ ±  8%       ~ (p=0.481 n=10)   508.3µ ± 5%  -80.98% (p=0.000 n=10)   485.0µ ± 2%  -81.85% (p=0.000 n=10)
IPCacheUpsert1000-10     69.543m ± 13%    66.515m ± 17%       ~ (p=0.481 n=10)   5.282m ± 5%  -92.40% (p=0.000 n=10)   5.056m ± 2%  -92.73% (p=0.000 n=10)
IPCacheUpsert10000-10   4166.59m ±  1%   4299.95m ±  2%  +3.20% (p=0.000 n=10)   48.63m ± 5%  -98.83% (p=0.000 n=10)   44.87m ± 5%  -98.92% (p=0.000 n=10)
geomean                   16.56m           16.47m        -0.55%                  1.589m       -90.40%                  1.513m       -90.86%

The last commit, cleanup.txt removes some now redundant work, and hence gets a mild speedup. If preferred, it can be squashed into the third commit, but like this the algorithmic changes are easier to understand.

goos: linux
goarch: arm64
pkg: github.com/cilium/cilium/pkg/ipcache
                      │   fix.txt   │            cleanup.txt             │
                      │   sec/op    │   sec/op     vs base               │
IPCacheUpsert10-10      48.84µ ± 5%   47.59µ ± 5%       ~ (p=0.165 n=10)
IPCacheUpsert100-10     508.3µ ± 5%   485.0µ ± 2%  -4.58% (p=0.005 n=10)
IPCacheUpsert1000-10    5.282m ± 5%   5.056m ± 2%  -4.28% (p=0.011 n=10)
IPCacheUpsert10000-10   48.63m ± 5%   44.87m ± 5%  -7.74% (p=0.009 n=10)
geomean                 1.589m        1.513m       -4.81%

Note that simply reverting 93cd67f is not actually a fix, as the quadratic behaviour still lurks. This can be observed by forcefully setting ipcache.needNamedPorts = true when benchmarking:

                      │ main-head.txt  │    revert-with-neednamedports.txt     │ 
                      │     sec/op     │     sec/op      vs base               │  
IPCacheUpsert10-10        95.00µ ±  5%     92.12µ ±  3%  -3.04% (p=0.000 n=10)   
IPCacheUpsert100-10      2729.7µ ±  4%    2617.5µ ± 12%       ~ (p=0.165 n=10)   
IPCacheUpsert1000-10     70.192m ± 11%    68.974m ± 10%       ~ (p=0.579 n=10)   
IPCacheUpsert10000-10   4247.52m ±  3%   4233.62m ±  1%       ~ (p=0.853 n=10)   
geomean                   16.68m           16.29m        -2.31%                 

Fixes: #24987

Address cilium-agent startup performance regression.

[christarazi]

Thanks for the fix, nice work! The fix looks good to me, I have a few minor questions below.

Just a nit on the commit msg: technically the namedport map is not the only lock being held while it's locked; the ipcache lock is always held before the namedport map lock is taken. Actually, it seems like we should "promote" this relationship to say that the ipcache lock must be taken before the namedport map.

[joestringer]

/test

[bimmlerd]

technically the namedport map is not the only lock being held while it's locked; the ipcache lock is always held before the namedport map lock is taken. Actually, it seems like we should "promote" this relationship to say that the ipcache lock must be taken before the namedport map.

What I was trying to express in the commit message was that we don't acquire a new lock while holding the namedport mutex (M), and hence don't establish any relations of the form M > X, for any X. I think that's correct. But yes, we do hold the ipcache mutex while acquiring the namedport mutex, and establish IPCache > M. This is precisely why it's crucial to not acquire any other mutex while also holding M.

Tests were green before pushing to clarify the commitmsg and fix a cosmetic issue in the benchmark, but more reruns can't hurt.

I've also dropped the Get interface from the NamedPortsMultiMap since it was needed only for tests, which I modified slightly to use GetNamedPorts instead. This also has the benefit that consumers of the API now don't have access to the raw PortProtoSet anymore, and thus are truly read-only by API.

[bimmlerd]

/test

Job 'Cilium-PR-K8s-1.25-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With native routing

Failure Output

FAIL: Pods are not ready in time: timed out waiting for pods with filter  to be ready: 4m0s timeout expired

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-5.4/66/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-5.4 so I can create one.

[bimmlerd]

/mlh new-flake Cilium-PR-K8s-1.25-kernel-5.4

👍 created #25042

[bimmlerd]

/test-1.25-5.4

[gandro]

Awesome work, thanks for taking care of this! 🙏

There is one issue around ipc.needNamedPorts and the externally visible behavior that I'm not sure how to address, see inline comment.

[bimmlerd]

/test-1.24-5.4

(Required tests have changed, running the new ones now)

[bimmlerd]

/test-1.25-4.19

[bimmlerd]

/test-1.26-net-next

[bimmlerd]

CI triage:

• k8s-1.26-kernel-net-next (test-1.26-net-next) failed with CI: Suite-k8s-1.26.K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master #24687 (we don't have the fix commit 0691784)

I'll gamble one rerun and otherwise rebase and run it all again

[bimmlerd]

/test-1.26-net-next

No bueno. Rebasing and let's run it all.

[bimmlerd]

/test