chg: dev: Remove locking from rdataslab_getownercase()

Under normal circumstances, the case bitfield in the slabheader should
be set only once.  By actually (soft-)enforcing this, the read locking
can be completely removed from the rdataslab_getownercase() as we can
check whether the case has been already set or not and making everything
immutable once the case has been set.

Merge branch 'ondrej/remove-locking-from-slabheader-ownercase' into 'main'

See merge request isc-projects/bind9!10843

lib/dns/qpcache.c

@@ -59,6 +59,7 @@
 
 #include "db_p.h"
 #include "qpcache_p.h"
+#include "rdataslab_p.h"
 
 #ifndef DNS_QPCACHE_LOG_STATS_LEVEL
 #define DNS_QPCACHE_LOG_STATS_LEVEL 3
@@ -71,37 +72,6 @@
 			goto failure;        \
 	} while (0)
 
-#define EXISTS(header)                                 \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_NONEXISTENT) == 0)
-#define NXDOMAIN(header)                               \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_NXDOMAIN) != 0)
-#define STALE(header)                                  \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_STALE) != 0)
-#define STALE_WINDOW(header)                           \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_STALE_WINDOW) != 0)
-#define OPTOUT(header)                                 \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_OPTOUT) != 0)
-#define NEGATIVE(header)                               \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_NEGATIVE) != 0)
-#define PREFETCH(header)                               \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_PREFETCH) != 0)
-#define ZEROTTL(header)                                \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_ZEROTTL) != 0)
-#define ANCIENT(header)                                \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_ANCIENT) != 0)
-#define STATCOUNT(header)                              \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_STATCOUNT) != 0)
-
 #define STALE_TTL(header, qpdb) \
 	(NXDOMAIN(header) ? 0 : qpdb->common.serve_stale_ttl)
 

lib/dns/qpzone.c

@@ -62,6 +62,7 @@
 
 #include "db_p.h"
 #include "qpzone_p.h"
+#include "rdataslab_p.h"
 
 #define CHECK(op)                            \
 	do {                                 \
@@ -70,22 +71,6 @@
 			goto failure;        \
 	} while (0)
 
-#define NONEXISTENT(header)                            \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_NONEXISTENT) != 0)
-#define IGNORE(header)                                 \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_IGNORE) != 0)
-#define RESIGN(header)                                 \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_RESIGN) != 0)
-#define OPTOUT(header)                                 \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_OPTOUT) != 0)
-#define STATCOUNT(header)                              \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_STATCOUNT) != 0)
-
 #define HEADERNODE(h) ((qpznode_t *)((h)->node))
 
 #define QPDB_ATTR_LOADED  0x01
@@ -1123,7 +1108,7 @@ setnsec3parameters(dns_db_t *db, qpz_version_t *version) {
 			if (header->serial <= version->serial &&
 			    !IGNORE(header))
 			{
-				if (NONEXISTENT(header)) {
+				if (!EXISTS(header)) {
 					header = NULL;
 				}
 				break;
@@ -1655,7 +1640,7 @@ qpzone_findrdataset(dns_db_t *db, dns_dbnode_t *dbnode,
 		header_next = header->next;
 		do {
 			if (header->serial <= serial && !IGNORE(header)) {
-				if (NONEXISTENT(header)) {
+				if (!EXISTS(header)) {
 					header = NULL;
 				}
 				break;
@@ -1787,7 +1772,7 @@ cname_and_other(qpznode_t *node, uint32_t serial) {
 			do {
 				if (header->serial <= serial && !IGNORE(header))
 				{
-					if (NONEXISTENT(header)) {
+					if (!EXISTS(header)) {
 						header = NULL;
 					}
 					break;
@@ -1805,7 +1790,7 @@ cname_and_other(qpznode_t *node, uint32_t serial) {
 			do {
 				if (header->serial <= serial && !IGNORE(header))
 				{
-					if (NONEXISTENT(header)) {
+					if (!EXISTS(header)) {
 						header = NULL;
 					}
 					break;
@@ -1862,7 +1847,7 @@ recordsize(dns_slabheader_t *header, unsigned int namelen) {
 static void
 maybe_update_recordsandsize(bool add, qpz_version_t *version,
 			    dns_slabheader_t *header, unsigned int namelen) {
-	if (NONEXISTENT(header)) {
+	if (!EXISTS(header)) {
 		return;
 	}
 
@@ -1934,7 +1919,7 @@ add(qpzonedb_t *qpdb, qpznode_t *node, const dns_name_t *nodename,
 		 * we'll try to create a new rdataset that is the union
 		 * of 'newheader' and 'header'.
 		 */
-		if (merge && !NONEXISTENT(header)) {
+		if (merge && EXISTS(header)) {
 			unsigned int flags = 0;
 			INSIST(version->serial >= header->serial);
 			merged = NULL;
@@ -2043,7 +2028,7 @@ add(qpzonedb_t *qpdb, qpznode_t *node, const dns_name_t *nodename,
 		 *
 		 * If we're trying to delete the type, don't bother.
 		 */
-		if (NONEXISTENT(newheader)) {
+		if (!EXISTS(newheader)) {
 			dns_slabheader_destroy(&newheader);
 			return DNS_R_UNCHANGED;
 		}
@@ -2751,7 +2736,7 @@ step(qpz_search_t *search, dns_qpiter_t *it, direction_t direction,
 				if (header->serial <= search->serial &&
 				    !IGNORE(header))
 				{
-					if (NONEXISTENT(header)) {
+					if (!EXISTS(header)) {
 						header = NULL;
 					}
 					break;
@@ -2914,7 +2899,7 @@ find_wildcard(qpz_search_t *search, qpznode_t **nodep, const dns_name_t *qname,
 		for (header = node->data; header != NULL; header = header->next)
 		{
 			if (header->serial <= search->serial &&
-			    !IGNORE(header) && !NONEXISTENT(header))
+			    !IGNORE(header) && EXISTS(header))
 			{
 				break;
 			}
@@ -2954,8 +2939,7 @@ find_wildcard(qpz_search_t *search, qpznode_t **nodep, const dns_name_t *qname,
 				     header = header->next)
 				{
 					if (header->serial <= search->serial &&
-					    !IGNORE(header) &&
+					    !IGNORE(header) && EXISTS(header))
-					    !NONEXISTENT(header))
 					{
 						break;
 					}
@@ -3141,7 +3125,7 @@ again:
 				if (header->serial <= search->serial &&
 				    !IGNORE(header))
 				{
-					if (NONEXISTENT(header)) {
+					if (!EXISTS(header)) {
 						header = NULL;
 					}
 					break;
@@ -3286,7 +3270,7 @@ qpzone_check_zonecut(qpznode_t *node, void *arg DNS__DB_FLARG) {
 				if (header->serial <= search->serial &&
 				    !IGNORE(header))
 				{
-					if (NONEXISTENT(header)) {
+					if (!EXISTS(header)) {
 						header = NULL;
 					}
 					break;
@@ -3613,7 +3597,7 @@ found:
 		do {
 			if (header->serial <= search.serial && !IGNORE(header))
 			{
-				if (NONEXISTENT(header)) {
+				if (!EXISTS(header)) {
 					header = NULL;
 				}
 				break;
@@ -4130,7 +4114,7 @@ rdatasetiter_first(dns_rdatasetiter_t *iterator DNS__DB_FLARG) {
 			if (header->serial <= version->serial &&
 			    !IGNORE(header))
 			{
-				if (NONEXISTENT(header)) {
+				if (!EXISTS(header)) {
 					header = NULL;
 				}
 				break;
@@ -4183,7 +4167,7 @@ rdatasetiter_next(dns_rdatasetiter_t *iterator DNS__DB_FLARG) {
 			if (header->serial <= version->serial &&
 			    !IGNORE(header))
 			{
-				if (NONEXISTENT(header)) {
+				if (!EXISTS(header)) {
 					header = NULL;
 				}
 				break;
@@ -4994,7 +4978,7 @@ qpzone_subtractrdataset(dns_db_t *db, dns_dbnode_t *dbnode,
 	while (header != NULL && IGNORE(header)) {
 		header = header->down;
 	}
-	if (header != NULL && !NONEXISTENT(header)) {
+	if (header != NULL && EXISTS(header)) {
 		unsigned int flags = 0;
 		subresult = NULL;
 		result = ISC_R_SUCCESS;

lib/dns/rdataslab.c

@@ -18,6 +18,7 @@
 #include <stdlib.h>
 
 #include <isc/ascii.h>
+#include <isc/atomic.h>
 #include <isc/mem.h>
 #include <isc/region.h>
 #include <isc/result.h>
@@ -30,18 +31,7 @@
 #include <dns/rdataslab.h>
 #include <dns/stats.h>
 
-#define CASESET(header)                                \
+#include "rdataslab_p.h"
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_CASESET) != 0)
-#define CASEFULLYLOWER(header)                         \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_CASEFULLYLOWER) != 0)
-#define NONEXISTENT(header)                            \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_NONEXISTENT) != 0)
-#define NEGATIVE(header)                               \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_NEGATIVE) != 0)
 
 /*
  * The rdataslab structure allows iteration to occur in both load order
@@ -844,36 +834,38 @@ dns_slabheader_raw(dns_slabheader_t *header) {
 
 void
 dns_slabheader_setownercase(dns_slabheader_t *header, const dns_name_t *name) {
-	unsigned int i;
+	REQUIRE(!CASESET(header));
-	bool fully_lower;
+
+	bool casefullylower = true;
 
 	/*
 	 * We do not need to worry about label lengths as they are all
 	 * less than or equal to 63.
 	 */
 	memset(header->upper, 0, sizeof(header->upper));
-	fully_lower = true;
+	for (size_t i = 0; i < name->length; i++) {
-	for (i = 0; i < name->length; i++) {
 		if (isupper(name->ndata[i])) {
 			header->upper[i / 8] |= 1 << (i % 8);
-			fully_lower = false;
+			casefullylower = false;
 		}
 	}
-	DNS_SLABHEADER_SETATTR(header, DNS_SLABHEADERATTR_CASESET);
+	if (casefullylower) {
-	if (fully_lower) {
 		DNS_SLABHEADER_SETATTR(header,
 				       DNS_SLABHEADERATTR_CASEFULLYLOWER);
 	}
+	DNS_SLABHEADER_SETATTR(header, DNS_SLABHEADERATTR_CASESET);
 }
 
 void
 dns_slabheader_copycase(dns_slabheader_t *dest, dns_slabheader_t *src) {
+	REQUIRE(!CASESET(dest));
 	if (CASESET(src)) {
-		uint_least16_t attr = DNS_SLABHEADER_GETATTR(
-			src, DNS_SLABHEADERATTR_CASESET |
-				     DNS_SLABHEADERATTR_CASEFULLYLOWER);
-		DNS_SLABHEADER_SETATTR(dest, attr);
 		memmove(dest->upper, src->upper, sizeof(src->upper));
+		if (CASEFULLYLOWER(src)) {
+			DNS_SLABHEADER_SETATTR(
+				dest, DNS_SLABHEADERATTR_CASEFULLYLOWER);
+		}
+		DNS_SLABHEADER_SETATTR(dest, DNS_SLABHEADERATTR_CASESET);
 	}
 }
 
@@ -915,10 +907,10 @@ dns_slabheader_destroy(dns_slabheader_t **headerp) {
 	isc_mem_t *mctx = header->node->mctx;
 	dns_db_deletedata(header->node, header);
 
-	if (NONEXISTENT(header)) {
+	if (EXISTS(header)) {
-		size = sizeof(*header);
-	} else {
 		size = dns_rdataslab_size(header);
+	} else {
+		size = sizeof(*header);
 	}
 
 	isc_mem_put(mctx, header, size);
@@ -1209,7 +1201,10 @@ static void
 rdataset_setownercase(dns_rdataset_t *rdataset, const dns_name_t *name) {
 	dns_slabheader_t *header = dns_rdataset_getheader(rdataset);
 
-	DNS_SLABHEADER_CLRATTR(header, DNS_SLABHEADERATTR_CASEFULLYLOWER);
+	/* The case could be set just once for the same header */
+	if (CASESET(header)) {
+		return;
+	}
 
 	dns_db_locknode(header->node, isc_rwlocktype_write);
 	dns_slabheader_setownercase(header, name);
@@ -1222,26 +1217,26 @@ rdataset_getownercase(const dns_rdataset_t *rdataset, dns_name_t *name) {
 	uint8_t mask = (1 << 7);
 	uint8_t bits = 0;
 
+	if (!CASESET(header)) {
+		return;
+	}
+
 	if (CASEFULLYLOWER(header)) {
 		isc_ascii_lowercopy(name->ndata, name->ndata, name->length);
 		return;
 	}
 
-	dns_db_locknode(header->node, isc_rwlocktype_read);
+	uint8_t *nd = name->ndata;
-	if (CASESET(header)) {
+	for (size_t i = 0; i < name->length; i++) {
-		uint8_t *nd = name->ndata;
+		if (mask == (1 << 7)) {
-		for (size_t i = 0; i < name->length; i++) {
+			bits = header->upper[i / 8];
-			if (mask == (1 << 7)) {
+			mask = 1;
-				bits = header->upper[i / 8];
+		} else {
-				mask = 1;
+			mask <<= 1;
-			} else {
-				mask <<= 1;
-			}
-			nd[i] = (bits & mask) ? isc_ascii_toupper(nd[i])
-					      : isc_ascii_tolower(nd[i]);
 		}
+		nd[i] = (bits & mask) ? isc_ascii_toupper(nd[i])
+				      : isc_ascii_tolower(nd[i]);
 	}
-	dns_db_unlocknode(header->node, isc_rwlocktype_read);
 }
 
 static dns_slabheader_t *

lib/dns/rdataslab_p.h

@@ -0,0 +1,59 @@
+/*
+ * Copyright (C) Internet Systems Consortium, Inc. ("ISC")
+ *
+ * SPDX-License-Identifier: MPL-2.0
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, you can obtain one at https://mozilla.org/MPL/2.0/.
+ *
+ * See the COPYRIGHT file distributed with this work for additional
+ * information regarding copyright ownership.
+ */
+
+#pragma once
+
+#include <dns/rdataslab.h>
+
+#define ANCIENT(header)                                \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_ANCIENT) != 0)
+#define CASEFULLYLOWER(header)                         \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_CASEFULLYLOWER) != 0)
+#define CASESET(header)                                \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_CASESET) != 0)
+#define EXISTS(header)                                 \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_NONEXISTENT) == 0)
+#define IGNORE(header)                                 \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_IGNORE) != 0)
+#define NEGATIVE(header)                               \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_NEGATIVE) != 0)
+#define NXDOMAIN(header)                               \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_NXDOMAIN) != 0)
+#define OPTOUT(header)                                 \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_OPTOUT) != 0)
+#define PREFETCH(header)                               \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_PREFETCH) != 0)
+#define RESIGN(header)                                 \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_RESIGN) != 0)
+#define STALE(header)                                  \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_STALE) != 0)
+#define STALE_WINDOW(header)                           \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_STALE_WINDOW) != 0)
+#define STATCOUNT(header)                              \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_STATCOUNT) != 0)
+#define ZEROTTL(header)                                \
+	((atomic_load_acquire(&(header)->attributes) & \
+	  DNS_SLABHEADERATTR_ZEROTTL) != 0)

tests/dns/qpzone_test.c

@@ -34,6 +34,8 @@
 #include <dns/rdatastruct.h>
 #define KEEP_BEFORE
 
+#include "rdataslab_p.h"
+
 /* Include the main file */
 
 #pragma GCC diagnostic push
@@ -45,10 +47,6 @@
 #undef CHECK
 #include <tests/dns.h>
 
-#define CASESET(header)                                \
-	((atomic_load_acquire(&(header)->attributes) & \
-	  DNS_SLABHEADERATTR_CASESET) != 0)
-
 const char *ownercase_vectors[12][2] = {
 	{
 		"AaBbCcDdEeFfGgHhIiJjKkLlMmNnOoPpQqRrSsTtUuVvWwXxYyZz",