This change makes the zone table lock-free for reads. Previously, the
zone table used a red-black tree, which is not thread safe, so the hot
read path acquired both the per-view mutex and the per-zonetable
rwlock. (The double locking was to fix to cleanup races on shutdown.)
One visible difference is that zones are not necessarily shut down
promptly: it depends on when the qp-trie garbage collector cleans up
the zone table. The `catz` system test checks several times that zones
have been deleted; the test now checks for zones to be removed from
the server configuration, instead of being fully shut down. The catz
test does not churn through enough zones to trigger a gc, so the zones
are not fully detached until the server exits.
After this change, it is still possible to improve the way we handle
changes to the zone table, for instance, batching changes, or better
compaction heuristics.
This is a simple replacement using the semantic patch from the previous
commit and as added bonus, one removal of previously undetected unused
variable in named/server.c.
the new dns_view_addtrustedkey() function allows a view's trust
anchors to be updated directly. this code was formerly in
dns_client_addtrustedkey(), which is now a wrapper around
dns_view_addtrustedkey().
In !7538, the shutdown procedure was simplified, but the ordering was
wrong, we need to shutdown the resolver, adb and requestmgr before
detaching those objects from the view, because there are cross
dependencies between at least the resolver and the adb.
Execute the shutdown(s) first, only when all three shutdowns have been
executed, detach those objects from the view.
This change should make sure that catalog zone update processing
doesn't happen when the catalog zone is being shut down. This
should help avoid races when offloading the catalog zone updates
in the follow-up commit.
the 'dispatchmgr' member of the resolver object is used by both
the dns_resolver and dns_request modules, and may in the future
be used by others such as dns_xfrin. it doesn't make sense for it
to live in the resolver object; this commit moves it into dns_view.
as there is no further use of isc_task in BIND, this commit removes
it, along with isc_taskmgr, isc_event, and all other related types.
functions that accepted taskmgr as a parameter have been cleaned up.
as a result of this change, some functions can no longer fail, so
they've been changed to type void, and their callers have been
updated accordingly.
the tasks table has been removed from the statistics channel and
the stats version has been updated. dns_dyndbctx has been changed
to reference the loopmgr instead of taskmgr, and DNS_DYNDB_VERSION
has been udpated as well.
callback events from dns_resolver_createfetch() are now posted
using isc_async_run.
other modules which called the resolver and maintained task/taskmgr
objects for this purpose have been cleaned up.
dns_request_create() and _createraw() now take a 'loop' parameter
and run the callback event on the specified loop.
as the task manager is no longer used, it has been removed from
the dns_requestmgr structure. the dns_resolver_taskmgr() function
is also no longer used and has been removed.
This changes the internal isc_rwlock implementation to:
Irina Calciu, Dave Dice, Yossi Lev, Victor Luchangco, Virendra
J. Marathe, and Nir Shavit. 2013. NUMA-aware reader-writer locks.
SIGPLAN Not. 48, 8 (August 2013), 157–166.
DOI:https://doi.org/10.1145/2517327.24425
(The full article available from:
http://mcg.cs.tau.ac.il/papers/ppopp2013-rwlocks.pdf)
The implementation is based on the The Writer-Preference Lock (C-RW-WP)
variant (see the 3.4 section of the paper for the rationale).
The implemented algorithm has been modified for simplicity and for usage
patterns in rbtdb.c.
The changes compared to the original algorithm:
* We haven't implemented the cohort locks because that would require a
knowledge of NUMA nodes, instead a simple atomic_bool is used as
synchronization point for writer lock.
* The per-thread reader counters are not being used - this would
require the internal thread id (isc_tid_v) to be always initialized,
even in the utilities; the change has a slight performance penalty,
so we might revisit this change in the future. However, this change
also saves a lot of memory, because cache-line aligned counters were
used, so on 32-core machine, the rwlock would be 4096+ bytes big.
* The readers use a writer_barrier that will raise after a while when
readers lock can't be acquired to prevent readers starvation.
* Separate ingress and egress readers counters queues to reduce both
inter and intra-thread contention.
The dns_adb would serialize all fetches on a single task. Create a
per-thread task, so the fetches will stay local to the thread that
initiated the fetch.
The dns_rpz_zones structure was using .refs and .irefs for strong and
weak reference counting. Rewrite the unit to use just a single
reference counting + shutdown sequence (dns_rpz_destroy_rpzs) that must
be called by the creator of the dns_rpz_zones_t object. Remove the
reference counting from the dns_rpz_zone structure as it is not needed
because the zone objects are fully embedded into the dns_rpz_zones
structure and dns_rpz_zones_t object must never be destroyed before all
dns_rpz_zone_t objects.
The dns_rps_zones_t reference counting uses the new ISC_REFCOUNT_TRACE
capability - enable by defining DNS_RPZ_TRACE in the dns/rpz.h header.
Additionally, add magic numbers to the dns_rpz_zone and dns_rpz_zones
structures.
There were a number of places where the zone table should have been
locked, but wasn't, when dns_zt_apply was called.
Added a isc_rwlocktype_t type parameter to dns_zt_apply and adjusted
all calls to using it. Removed locks in callers.
The call to dns_view_flushcache() is done under exclusive mode, but we
still need to check if view->adb is still attached before calling
dns_adb_flush() because the shutdown might have been already
initialized. This most likely only a theoretical problem on shutdown
because there's either no way how to initiate cache flush when shutting
down or very slim window where the `rndc flush` would have to hit the
slim time during named shutdown.
When starting priming from dns_view_find(), the dns_view shutdown could
be initiated by different thread, detaching from the resolver. Use
dns_view_getresolver() to attach to the resolver under view->lock, so we
don't try to call dns_resolver_prime() with NULL pointer.
There are more accesses to view->resolver, (and also view->adb and
view->requestmgr that suffer from the same problem) in the dns_view
module, but they are all done in exclusive mode or under a view->lock.
Mostly generated automatically with the following semantic patch,
except where coccinelle was confused by #ifdef in lib/isc/net.c
@@ expression list args; @@
- UNEXPECTED_ERROR(__FILE__, __LINE__, args)
+ UNEXPECTED_ERROR(args)
@@ expression list args; @@
- FATAL_ERROR(__FILE__, __LINE__, args)
+ FATAL_ERROR(args)
Getting the recorded value of 'edns-udp-size' from the resolver requires
strong attach to the dns_view because we are accessing `view->resolver`.
This is not the case in places (f.e. dns_zone unit) where `.udpsize` is
accessed. By moving the .udpsize field from `struct dns_resolver` to
`struct dns_view`, we can access the value directly even with weakly
attached dns_view without the need to lock the view because `.udpsize`
can be accessed after the dns_view object has been shut down.
The dns_view implements weak and strong reference counting. When strong
reference counting reaches zero, the adb, ntatable and resolver objects
are shut down and detached.
In dns_zone and dns_nta the dns_view was weakly attached, but the
view->resolver reference was accessed directly leading to dereferencing
the NULL pointer.
Add dns_view_getresolver() method which attaches to view->resolver
object under the lock (if it still exists) ensuring the dns_resolver
will be kept referenced until not needed.
Previously:
* applications were using isc_app as the base unit for running the
application and signal handling.
* networking was handled in the netmgr layer, which would start a
number of threads, each with a uv_loop event loop.
* task/event handling was done in the isc_task unit, which used
netmgr event loops to run the isc_event calls.
In this refactoring:
* the network manager now uses isc_loop instead of maintaining its
own worker threads and event loops.
* the taskmgr that manages isc_task instances now also uses isc_loopmgr,
and every isc_task runs on a specific isc_loop bound to the specific
thread.
* applications have been updated as necessary to use the new API.
* new ISC_LOOP_TEST macros have been added to enable unit tests to
run isc_loop event loops. unit tests have been updated to use this
where needed.
* isc_timer was rewritten using the uv_timer, and isc_timermgr_t was
completely removed; isc_timer objects are now directly created on the
isc_loop event loops.
* the isc_timer API has been simplified. the "inactive" timer type has
been removed; timers are now stopped by calling isc_timer_stop()
instead of resetting to inactive.
* isc_manager now creates a loop manager rather than a timer manager.
* modules and applications using isc_timer have been updated to use the
new API.
When namespace is grafted on, the DNSSEC proofs for non existance
need to come from that namespace and not a higher namespace. We
add 3 function dns_view_sfd_add, dns_view_sfd_del and dns_view_sfd_find
to add, remove and find the namespace that should be used when
checking NSEC records.
dns_view_sfd_add adds a name to a tree, creating the tree if needed.
If the name already existed in the tree the reference count is
increased otherwise it is initalised to 1.
dns_view_sfd_del removes a reference to a name in the tree, if the
count goes to 0 the node is removed.
dns_view_sfd_find returns the namespace to be used to entered name.
If there isn't an enclosing name in the tree, or the tree does not
yet exist, the root name is returned.
Access to the tree is controlled by a read/write lock.
Previously, tasks could be created either unbound or bound to a specific
thread (worker loop). The unbound tasks would be assigned to a random
thread every time isc_task_send() was called. Because there's no logic
that would assign the task to the least busy worker, this just creates
unpredictability. Instead of random assignment, bind all the previously
unbound tasks to worker 0, which is guaranteed to exist.
Since the fctx hash table is now self-resizing, and resolver tasks are
selected to match the thread that created the fetch context, there
shouldn't be any significant advantage to having multiple tasks per CPU;
a single task per thread should be sufficient.
Additionally, the fetch context is always pinned to the calling netmgr
thread to minimize the contention just to coalesced fetches - if two
threads starts the same fetch, it will be pinned to the first one to get
the bucket.
the request manager has no direct dependency on the
view, so there's no need for a weak reference. remove the
dns_requestmgr_whenshutdown() mechanism since it is no longer
used.
weakly attaching and detaching when creating and destroying the
resolver obviates the need to have a callback event to do the weak
detach. remove the dns_resolver_whenshutdown() mechanism, as it is
now unused.
weakly attaching and detaching the view when creating or destroying
the ADB obviates the need for a whenshutdown callback event to do
the detaching. remove the dns_adb_whenshutdown() mechanism, since
it is no longer needed.
for better object separation, ADB and resolver statistics counters
are now stored in the ADB and resolver objects themsevles, rather than
in the associated view.
it's not necessary to use view attributes to determine whether
the ADB, resolver and requestmgr need to be shut down; checking
whether they're NULL is sufficient.
- eliminate dns_view_flushanddetach(), which was only called from
one place; instead, we now call a function dns_view_flushonshutdown()
which sets the view up to flush zones when it is detached normally
with dns_view_detach().
- cleaned up code in dns_view_create().
The ADB previously used separate reference counters for internal
and external references, plus additional counters for ABD find
and namehook objects, and used all these counters to coordinate
its shutdown process, which was a multi-stage affair involving
a sequence of control events.
It also used a complex interlocking set of static functions for
referencing, deferencing, linking, unlinking, and cleaning up various
internal objects; these functions returned boolean values to their
callers to indicate what additional processing was needed.
The changes in the previous two commits destabilized this fragile
system in a way that was difficult to recover from, so in this commit
we refactor all of it. The dns_adb and dns_adbentry objects now use
conventional attach and detach functions for reference counting, and
the shutdown process is much more straightforward. Instead of
handling shutdown asynchronously, we can just destroy the ADB when
references reach zero
In addition, ADB locking has been simplified. Instead of a
single `find_{name,entry}_and_lock()` function which searches for
a name or entry's hash bucket, locks it, and then searches for the
name or entry in the bucket, we now use one function to find the
bucket (leaving it to the caller to do the locking) and another
find the name or entry. Instead of locking the entire ADB when
modifying hash tables, we now use read-write locks around the
specific hash table. The only remaining need for adb->lock
is when modifying the `whenshutdown` list.
Comments throughout the module have been improved.
Historically, the inline keyword was a strong suggestion to the compiler
that it should inline the function marked inline. As compilers became
better at optimising, this functionality has receded, and using inline
as a suggestion to inline a function is obsolete. The compiler will
happily ignore it and inline something else entirely if it finds that's
a better optimisation.
Therefore, remove all the occurences of the inline keyword with static
functions inside single compilation unit and leave the decision whether
to inline a function or not entirely on the compiler
NOTE: We keep the usage the inline keyword when the purpose is to change
the linkage behaviour.
This commit converts the license handling to adhere to the REUSE
specification. It specifically:
1. Adds used licnses to LICENSES/ directory
2. Add "isc" template for adding the copyright boilerplate
3. Changes all source files to include copyright and SPDX license
header, this includes all the C sources, documentation, zone files,
configuration files. There are notes in the doc/dev/copyrights file
on how to add correct headers to the new files.
4. Handle the rest that can't be modified via .reuse/dep5 file. The
binary (or otherwise unmodifiable) files could have license places
next to them in <foo>.license file, but this would lead to cluttered
repository and most of the files handled in the .reuse/dep5 file are
system test files.
Commit 308bc46a59302c88ecff11d4831475ecfa8b8fb0 introduced a change to
the view_flushanddetach() function which makes the latter access
view->zonetable without holding view->lock. As confirmed by TSAN, this
enables races between threads for view->zonetable accesses.
Swap the view->zonetable pointer under view lock and then detach the
local swapped dns_zt_t later when the view lock is already unlocked.
This commit also changes the dns_zt interfaces, so the setting the
zonetable "flush" flag is separate operation to dns_zt_detach,
e.g. instead of doing:
if (view->flush) {
dns_zt_flushanddetach(&zt);
} else {
dns_zt_detach(&zt);
}
the code is now:
if (view->flush) {
dns_zt_flush(zt);
}
dns_zt_detach(&zt);
making the code more consistent with how we handle flushing and
detaching dns_zone_t pointers from the view.
The memory context created in the clientmgr context was missing a name,
so it was nameless in the memory context statistics.
Set the clientmgr memory context name to "clientmgr".
A number of DNS implementation produce NSEC records with bad type
maps that don't contain types that exist at the name leading to
NODATA responses being synthesize instead of the records in the
zone. NSEC records with these bad type maps often have the NSEC
NSEC field set to '\000.QNAME'. We look for the first label of
this pattern.
e.g.
example.com NSEC \000.example.com SOA NS NSEC RRSIG
example.com RRRSIG NSEC ...
example.com SOA ...
example.com RRRSIG SOA ...
example.com NS ...
example.com RRRSIG NS ...
example.com A ...
example.com RRRSIG A ...
A is missing from the type map.
This introduces a temporary option 'reject-000-label' to control
this behaviour.
With isc_mem_get() and dns_name_dup() no longer being able to fail, some
functions can now only return ISC_R_SUCCESS. Change the return type to
void for the following function(s):
* dns_view_adddelegationonly()
* dns_view_excludedelegationonly()
