From: Ondřej Surý Date: Thu, 25 Jun 2020 15:48:34 +0000 (+0200) Subject: Update STALE and ANCIENT header attributes atomically X-Git-Tag: v9.17.4~66^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=81d4230e60fc4c75077b7fe97f2dab9b9e9bb2a9;p=thirdparty%2Fbind9.git Update STALE and ANCIENT header attributes atomically The ThreadSanitizer found a data race when updating the stale header. Instead of trying to acquire the write lock and failing occasionally which would skew the statistics, the dns_rdatasetheader_t.attributes field has been promoted to use stdatomics. Updating the attributes in the mark_header_ancient() and mark_header_stale() now uses the cmpxchg to update the attributes forfeiting the need to hold the write lock on the tree. Please note that mark_header_ancient() still needs to hold the lock because .dirty is being updated in the same go. --- diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index c0b34796841..f5eee327781 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -203,7 +203,7 @@ typedef struct rdatasetheader { rbtdb_serial_t serial; dns_ttl_t rdh_ttl; rbtdb_rdatatype_t type; - uint16_t attributes; + atomic_uint_least16_t attributes; dns_trust_t trust; struct noqname *noqname; struct noqname *closest; @@ -284,22 +284,58 @@ typedef ISC_LIST(dns_rbtnode_t) rbtnodelist_t; #undef IGNORE /* WIN32 winbase.h defines this. */ -#define EXISTS(header) (((header)->attributes & RDATASET_ATTR_NONEXISTENT) == 0) -#define NONEXISTENT(header) \ - (((header)->attributes & RDATASET_ATTR_NONEXISTENT) != 0) -#define IGNORE(header) (((header)->attributes & RDATASET_ATTR_IGNORE) != 0) -#define RETAIN(header) (((header)->attributes & RDATASET_ATTR_RETAIN) != 0) -#define NXDOMAIN(header) (((header)->attributes & RDATASET_ATTR_NXDOMAIN) != 0) -#define STALE(header) (((header)->attributes & RDATASET_ATTR_STALE) != 0) -#define RESIGN(header) (((header)->attributes & RDATASET_ATTR_RESIGN) != 0) -#define OPTOUT(header) (((header)->attributes & RDATASET_ATTR_OPTOUT) != 0) -#define NEGATIVE(header) (((header)->attributes & RDATASET_ATTR_NEGATIVE) != 0) -#define PREFETCH(header) (((header)->attributes & RDATASET_ATTR_PREFETCH) != 0) -#define CASESET(header) (((header)->attributes & RDATASET_ATTR_CASESET) != 0) -#define ZEROTTL(header) (((header)->attributes & RDATASET_ATTR_ZEROTTL) != 0) -#define CASEFULLYLOWER(header) \ - (((header)->attributes & RDATASET_ATTR_CASEFULLYLOWER) != 0) -#define ANCIENT(header) (((header)->attributes & RDATASET_ATTR_ANCIENT) != 0) +#define EXISTS(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_NONEXISTENT) == 0) +#define NONEXISTENT(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_NONEXISTENT) != 0) +#define IGNORE(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_IGNORE) != 0) +#define RETAIN(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_RETAIN) != 0) +#define NXDOMAIN(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_NXDOMAIN) != 0) +#define STALE(header) \ + ((atomic_load_acquire(&(header)->attributes) & RDATASET_ATTR_STALE) != \ + 0) +#define RESIGN(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_RESIGN) != 0) +#define OPTOUT(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_OPTOUT) != 0) +#define NEGATIVE(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_NEGATIVE) != 0) +#define PREFETCH(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_PREFETCH) != 0) +#define CASESET(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_CASESET) != 0) +#define ZEROTTL(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_ZEROTTL) != 0) +#define CASEFULLYLOWER(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_CASEFULLYLOWER) != 0) +#define ANCIENT(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_ANCIENT) != 0) +#define STATCOUNT(header) \ + ((atomic_load_acquire(&(header)->attributes) & \ + RDATASET_ATTR_STATCOUNT) != 0) + +#define RDATASET_ATTR_GET(header, attribute) \ + (atomic_load_acquire(&(header)->attributes) & attribute) +#define RDATASET_ATTR_SET(header, attribute) \ + atomic_fetch_or_release(&(header)->attributes, attribute) +#define RDATASET_ATTR_CLR(header, attribute) \ + atomic_fetch_and_release(&(header)->attributes, ~(attribute)) #define ACTIVE(header, now) \ (((header)->rdh_ttl > (now)) || \ @@ -820,16 +856,18 @@ update_cachestats(dns_rbtdb_t *rbtdb, isc_result_t result) { static bool do_stats(rdatasetheader_t *header) { - return (EXISTS(header) && - (header->attributes & RDATASET_ATTR_STATCOUNT) != 0); + return (EXISTS(header) && STATCOUNT(header)); } static void -update_rrsetstats(dns_rbtdb_t *rbtdb, rdatasetheader_t *header, - bool increment) { +update_rrsetstats(dns_rbtdb_t *rbtdb, const rbtdb_rdatatype_t htype, + const uint_least16_t hattributes, const bool increment) { dns_rdatastatstype_t statattributes = 0; dns_rdatastatstype_t base = 0; dns_rdatastatstype_t type; + rdatasetheader_t *header = &(rdatasetheader_t){ + .type = htype, .attributes = ATOMIC_VAR_INIT(hattributes) + }; if (!do_stats(header)) { return; @@ -1427,6 +1465,13 @@ init_rdataset(dns_rbtdb_t *rbtdb, rdatasetheader_t *h) { h->is_mmapped = 0; h->next_is_relative = 0; h->node_is_relative = 0; + atomic_init(&h->attributes, 0); + +#ifndef ISC_MUTEX_ATOMICS + STATIC_ASSERT((sizeof(h->attributes) == 2), + "The .attributes field of rdatasetheader_t needs to be " + "16-bit int type exactly."); +#endif /* !ISC_MUTEX_ATOMICS */ #if TRACE_HEADER if (IS_CACHE(rbtdb) && rbtdb->common.rdclass == dns_rdataclass_in) { @@ -1455,12 +1500,11 @@ update_newheader(rdatasetheader_t *newh, rdatasetheader_t *old) { newh->node = (dns_rbtnode_t *)p; } if (CASESET(old)) { - uint16_t attr; - + uint_least16_t attr = RDATASET_ATTR_GET( + old, + (RDATASET_ATTR_CASESET | RDATASET_ATTR_CASEFULLYLOWER)); + RDATASET_ATTR_SET(newh, attr); memmove(newh->upper, old->upper, sizeof(old->upper)); - attr = old->attributes & - (RDATASET_ATTR_CASESET | RDATASET_ATTR_CASEFULLYLOWER); - newh->attributes |= attr; } } @@ -1486,7 +1530,8 @@ free_rdataset(dns_rbtdb_t *rbtdb, isc_mem_t *mctx, rdatasetheader_t *rdataset) { unsigned int size; int idx; - update_rrsetstats(rbtdb, rdataset, false); + update_rrsetstats(rbtdb, rdataset->type, + atomic_load_acquire(&rdataset->attributes), false); idx = rdataset->node->locknum; if (ISC_LINK_LINKED(rdataset, link)) { @@ -1536,13 +1581,14 @@ rollback_node(dns_rbtnode_t *node, rbtdb_serial_t serial) { */ for (header = node->data; header != NULL; header = header->next) { if (header->serial == serial) { - header->attributes |= RDATASET_ATTR_IGNORE; + RDATASET_ATTR_SET(header, RDATASET_ATTR_IGNORE); make_dirty = true; } for (dcurrent = header->down; dcurrent != NULL; dcurrent = dcurrent->down) { if (dcurrent->serial == serial) { - dcurrent->attributes |= RDATASET_ATTR_IGNORE; + RDATASET_ATTR_SET(dcurrent, + RDATASET_ATTR_IGNORE); make_dirty = true; } } @@ -1554,12 +1600,19 @@ rollback_node(dns_rbtnode_t *node, rbtdb_serial_t serial) { static inline void mark_header_ancient(dns_rbtdb_t *rbtdb, rdatasetheader_t *header) { + uint_least16_t attributes = atomic_load_acquire(&header->attributes); + uint_least16_t newattributes = 0; + /* * If we are already ancient there is nothing to do. */ - if (ANCIENT(header)) { - return; - } + do { + if ((attributes & RDATASET_ATTR_ANCIENT) != 0) { + return; + } + newattributes = attributes | RDATASET_ATTR_ANCIENT; + } while (!atomic_compare_exchange_weak_acq_rel( + &header->attributes, &attributes, newattributes)); /* * Decrement the stats counter for the appropriate RRtype. @@ -1567,23 +1620,28 @@ mark_header_ancient(dns_rbtdb_t *rbtdb, rdatasetheader_t *header) { * stale type counter, otherwise it decrements the active * stats type counter. */ - update_rrsetstats(rbtdb, header, false); - - header->attributes |= RDATASET_ATTR_ANCIENT; + update_rrsetstats(rbtdb, header->type, attributes, false); header->node->dirty = 1; /* Increment the stats counter for the ancient RRtype. */ - update_rrsetstats(rbtdb, header, true); + update_rrsetstats(rbtdb, header->type, newattributes, true); } static inline void mark_header_stale(dns_rbtdb_t *rbtdb, rdatasetheader_t *header) { + uint_least16_t attributes = atomic_load_acquire(&header->attributes); + uint_least16_t newattributes = 0; + /* * If we are already stale there is nothing to do. */ - if (STALE(header)) { - return; - } + do { + if ((attributes & RDATASET_ATTR_STALE) != 0) { + return; + } + newattributes = attributes | RDATASET_ATTR_STALE; + } while (!atomic_compare_exchange_weak_acq_rel( + &header->attributes, &attributes, newattributes)); /* Decrement the stats counter for the appropriate RRtype. * If the ANCIENT attribute is set (although it is very @@ -1591,11 +1649,9 @@ mark_header_stale(dns_rbtdb_t *rbtdb, rdatasetheader_t *header) { * will decrement the ancient stale type counter, otherwise it * decrements the active stats type counter. */ - update_rrsetstats(rbtdb, header, false); - header->attributes |= RDATASET_ATTR_STALE; - - update_rrsetstats(rbtdb, header, true); + update_rrsetstats(rbtdb, header->type, attributes, false); + update_rrsetstats(rbtdb, header->type, newattributes, true); } static inline void @@ -5530,17 +5586,20 @@ printnode(dns_db_t *db, dns_dbnode_t *node, FILE *out) { first = true; fprintf(out, "\ttype %u", current->type); do { + uint_least16_t attributes = atomic_load_acquire( + ¤t->attributes); if (!first) { fprintf(out, "\t"); } first = false; fprintf(out, "\tserial = %lu, ttl = %u, " - "trust = %u, attributes = %u, " + "trust = %u, attributes = %" PRIuLEAST16 + ", " "resign = %u\n", (unsigned long)current->serial, current->rdh_ttl, current->trust, - current->attributes, + attributes, (current->resign << 1) | current->resign_lsb); current = current->down; @@ -6702,9 +6761,9 @@ addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, set_ttl(rbtdb, newheader, rdataset->ttl + now); newheader->type = RBTDB_RDATATYPE_VALUE(rdataset->type, rdataset->covers); - newheader->attributes = 0; + atomic_init(&newheader->attributes, 0); if (rdataset->ttl == 0U) { - newheader->attributes |= RDATASET_ATTR_ZEROTTL; + RDATASET_ATTR_SET(newheader, RDATASET_ATTR_ZEROTTL); } newheader->noqname = NULL; newheader->closest = NULL; @@ -6718,7 +6777,7 @@ addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, now = 0; if ((rdataset->attributes & DNS_RDATASETATTR_RESIGN) != 0) { - newheader->attributes |= RDATASET_ATTR_RESIGN; + RDATASET_ATTR_SET(newheader, RDATASET_ATTR_RESIGN); newheader->resign = (isc_stdtime_t)( dns_time64_from32(rdataset->resign) >> 1); newheader->resign_lsb = rdataset->resign & 0x1; @@ -6731,16 +6790,16 @@ addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, newheader->resign = 0; newheader->resign_lsb = 0; if ((rdataset->attributes & DNS_RDATASETATTR_PREFETCH) != 0) { - newheader->attributes |= RDATASET_ATTR_PREFETCH; + RDATASET_ATTR_SET(newheader, RDATASET_ATTR_PREFETCH); } if ((rdataset->attributes & DNS_RDATASETATTR_NEGATIVE) != 0) { - newheader->attributes |= RDATASET_ATTR_NEGATIVE; + RDATASET_ATTR_SET(newheader, RDATASET_ATTR_NEGATIVE); } if ((rdataset->attributes & DNS_RDATASETATTR_NXDOMAIN) != 0) { - newheader->attributes |= RDATASET_ATTR_NXDOMAIN; + RDATASET_ATTR_SET(newheader, RDATASET_ATTR_NXDOMAIN); } if ((rdataset->attributes & DNS_RDATASETATTR_OPTOUT) != 0) { - newheader->attributes |= RDATASET_ATTR_OPTOUT; + RDATASET_ATTR_SET(newheader, RDATASET_ATTR_OPTOUT); } if ((rdataset->attributes & DNS_RDATASETATTR_NOQNAME) != 0) { result = addnoqname(rbtdb, newheader, rdataset); @@ -6807,8 +6866,10 @@ addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, isc_rwlocktype_write); if (rbtdb->rrsetstats != NULL) { - newheader->attributes |= RDATASET_ATTR_STATCOUNT; - update_rrsetstats(rbtdb, newheader, true); + RDATASET_ATTR_SET(newheader, RDATASET_ATTR_STATCOUNT); + update_rrsetstats(rbtdb, newheader->type, + atomic_load_acquire(&newheader->attributes), + true); } if (IS_CACHE(rbtdb)) { @@ -6914,7 +6975,7 @@ subtractrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, set_ttl(rbtdb, newheader, rdataset->ttl); newheader->type = RBTDB_RDATATYPE_VALUE(rdataset->type, rdataset->covers); - newheader->attributes = 0; + atomic_init(&newheader->attributes, 0); newheader->serial = rbtversion->serial; newheader->trust = 0; newheader->noqname = NULL; @@ -6924,7 +6985,7 @@ subtractrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, newheader->last_used = 0; newheader->node = rbtnode; if ((rdataset->attributes & DNS_RDATASETATTR_RESIGN) != 0) { - newheader->attributes |= RDATASET_ATTR_RESIGN; + RDATASET_ATTR_SET(newheader, RDATASET_ATTR_RESIGN); newheader->resign = (isc_stdtime_t)( dns_time64_from32(rdataset->resign) >> 1); newheader->resign_lsb = rdataset->resign & 0x1; @@ -6986,7 +7047,8 @@ subtractrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, init_rdataset(rbtdb, newheader); update_newheader(newheader, header); if (RESIGN(header)) { - newheader->attributes |= RDATASET_ATTR_RESIGN; + RDATASET_ATTR_SET(newheader, + RDATASET_ATTR_RESIGN); newheader->resign = header->resign; newheader->resign_lsb = header->resign_lsb; result = resign_insert(rbtdb, rbtnode->locknum, @@ -7024,7 +7086,8 @@ subtractrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, init_rdataset(rbtdb, newheader); set_ttl(rbtdb, newheader, 0); newheader->type = topheader->type; - newheader->attributes = RDATASET_ATTR_NONEXISTENT; + atomic_init(&newheader->attributes, + RDATASET_ATTR_NONEXISTENT); newheader->trust = 0; newheader->serial = rbtversion->serial; newheader->noqname = NULL; @@ -7128,7 +7191,7 @@ deleterdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, init_rdataset(rbtdb, newheader); set_ttl(rbtdb, newheader, 0); newheader->type = RBTDB_RDATATYPE_VALUE(type, covers); - newheader->attributes = RDATASET_ATTR_NONEXISTENT; + atomic_init(&newheader->attributes, RDATASET_ATTR_NONEXISTENT); newheader->trust = 0; newheader->noqname = NULL; newheader->closest = NULL; @@ -7320,7 +7383,7 @@ loading_addrdataset(void *arg, const dns_name_t *name, * check */ newheader->type = RBTDB_RDATATYPE_VALUE(rdataset->type, rdataset->covers); - newheader->attributes = 0; + atomic_init(&newheader->attributes, 0); newheader->trust = rdataset->trust; newheader->serial = 1; newheader->noqname = NULL; @@ -7332,7 +7395,7 @@ loading_addrdataset(void *arg, const dns_name_t *name, setownercase(newheader, name); if ((rdataset->attributes & DNS_RDATASETATTR_RESIGN) != 0) { - newheader->attributes |= RDATASET_ATTR_RESIGN; + RDATASET_ATTR_SET(newheader, RDATASET_ATTR_RESIGN); newheader->resign = (isc_stdtime_t)( dns_time64_from32(rdataset->resign) >> 1); newheader->resign_lsb = rdataset->resign & 0x1; @@ -7381,6 +7444,9 @@ rbt_datafixer(dns_rbtnode_t *rbtnode, void *base, size_t filesize, void *arg, header->is_mmapped = 1; header->node = rbtnode; header->node_is_relative = 0; +#ifdef ISC_MUTEX_ATOMICS + atomic_init(&header->attributes, header->attributes.v); +#endif if (RESIGN(header) && (header->resign != 0 || header->resign_lsb != 0)) { @@ -8082,7 +8148,7 @@ setsigningtime(dns_db_t *db, dns_rdataset_t *rdataset, isc_stdtime_t resign) { header->heap_index); } } else if (resign != 0) { - header->attributes |= RDATASET_ATTR_RESIGN; + RDATASET_ATTR_SET(header, RDATASET_ATTR_RESIGN); result = resign_insert(rbtdb, header->node->locknum, header); } NODE_UNLOCK(&rbtdb->node_locks[header->node->locknum].lock, @@ -8971,7 +9037,7 @@ rdataset_clearprefetch(dns_rdataset_t *rdataset) { header--; NODE_LOCK(&rbtdb->node_locks[rbtnode->locknum].lock, isc_rwlocktype_write); - header->attributes &= ~RDATASET_ATTR_PREFETCH; + RDATASET_ATTR_CLR(header, RDATASET_ATTR_PREFETCH); NODE_UNLOCK(&rbtdb->node_locks[rbtnode->locknum].lock, isc_rwlocktype_write); } @@ -9705,9 +9771,9 @@ setownercase(rdatasetheader_t *header, const dns_name_t *name) { } } } - header->attributes |= RDATASET_ATTR_CASESET; + RDATASET_ATTR_SET(header, RDATASET_ATTR_CASESET); if (ISC_LIKELY(fully_lower)) { - header->attributes |= RDATASET_ATTR_CASEFULLYLOWER; + RDATASET_ATTR_SET(header, RDATASET_ATTR_CASEFULLYLOWER); } } @@ -9781,13 +9847,13 @@ static void rdataset_getownercase(const dns_rdataset_t *rdataset, dns_name_t *name) { dns_rbtdb_t *rbtdb = rdataset->private1; dns_rbtnode_t *rbtnode = rdataset->private2; - const unsigned char *raw = rdataset->private3; /* RDATASLAB */ - const rdatasetheader_t *header; + unsigned char *raw = rdataset->private3; /* RDATASLAB */ + rdatasetheader_t *header; unsigned int i, j; unsigned char bits; unsigned char c, flip; - header = (const struct rdatasetheader *)(raw - sizeof(*header)); + header = (struct rdatasetheader *)(raw - sizeof(*header)); NODE_LOCK(&rbtdb->node_locks[rbtnode->locknum].lock, isc_rwlocktype_read); @@ -10406,9 +10472,9 @@ no_glue: */ static inline bool need_headerupdate(rdatasetheader_t *header, isc_stdtime_t now) { - if ((header->attributes & - (RDATASET_ATTR_NONEXISTENT | RDATASET_ATTR_ANCIENT | - RDATASET_ATTR_ZEROTTL)) != 0) + if (RDATASET_ATTR_GET(header, (RDATASET_ATTR_NONEXISTENT | + RDATASET_ATTR_ANCIENT | + RDATASET_ATTR_ZEROTTL)) != 0) { return (false); }