NLnetLabs / unbound

Unbound is a validating, recursive, and caching DNS resolver.
https://nlnetlabs.nl/unbound
BSD 3-Clause "New" or "Revised" License
3.15k stars 359 forks source link

fix: remove the auth_zones lock and count up/down queries in a common way #1160

Closed sakateka closed 1 week ago

sakateka commented 4 weeks ago

The lock field in the auth_zones struct is used only to protect the up/down query counters. My understanding is that the auth_zones struct is only constructed when there are no other threads, so there are no possibilities for concurrent access. However, concurrent modifications still occur in auth_zones.c, specifically during query counting. By adopting a common approach to tracking up/down queries, similar to that used for other server statistics, we can simplify the codebase and eliminate the need for locking mechanisms.

I benchmarked the performance of this patch using the following configuration to effectively disable cache:

... msg-cache-size: 2b msg-cache-slabs: 1 rrset-cache-size: 2b rrset-cache-slabs: 1 key-cache-size: 2b key-cache-slabs: 1 neg-cache-size: 2b ...

Results: Before

Read 9M.qerires, got 9000000 queries qps: 137430 ... qps: 137043 overall time: 15.0001 sec 0(NOERROR): 2070494 replies average qps: 138032

After

Read 9M.qerires, got 9000000 queries qps: 159759 qps: 161599 ... qps: 166993 qps: 157685 overall time: 15 sec 0(NOERROR): 2451661 replies average qps: 163444

sakateka commented 3 weeks ago

This patch can be implemented without removing the auth_zones lock. If there is any doubt about removing this lock, I can return it. But as far as I can see, this lock is not used anywhere in the current code to protect ztree for writing.

sakateka commented 3 weeks ago

If you don't like big lock removal, I create another PR that reworks counters but doesn't touch auth_zones' lock. https://github.com/NLnetLabs/unbound/pull/1169

sakateka commented 1 week ago

Closed in favor of #1169