From: Ondřej Surý Date: Thu, 28 Aug 2025 17:19:01 +0000 (+0200) Subject: Refactor the cyclic ordering to use query ID as offset X-Git-Tag: v9.21.14~57^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2963d1aaf1a82840b81da8bc3857727c07304402;p=thirdparty%2Fbind9.git Refactor the cyclic ordering to use query ID as offset Mimic the Unbound behaviour where the cyclic offset is taken from query ID, and remove recording of the current state. As the incoming query ID should have random distribution, the cyclic ordering should also have uniform distribution of the starting record. --- diff --git a/bin/tests/system/rrsetorder/tests.sh b/bin/tests/system/rrsetorder/tests.sh index 5064d7ebaf1..a1db5dcb397 100644 --- a/bin/tests/system/rrsetorder/tests.sh +++ b/bin/tests/system/rrsetorder/tests.sh @@ -35,7 +35,7 @@ ret=0 matches=0 for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do j=$((i % 4)) - dig_cmd @10.53.0.1 cyclic.example >dig.out.cyclic || ret=1 + dig_cmd +qid=$i @10.53.0.1 cyclic.example >dig.out.cyclic || ret=1 if [ $i -le 4 ]; then cp dig.out.cyclic dig.out.$j else @@ -60,7 +60,7 @@ ret=0 matches=0 for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do j=$((i % 4)) - dig_cmd @10.53.0.1 cyclic2.example >dig.out.cyclic2 || ret=1 + dig_cmd +qid=$i @10.53.0.1 cyclic2.example >dig.out.cyclic2 || ret=1 if [ $i -le 4 ]; then cp dig.out.cyclic2 dig.out.$j else @@ -100,7 +100,7 @@ ret=0 matches=0 for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do j=$((i % 4)) - dig_cmd @10.53.0.2 cyclic.example >dig.out.cyclic || ret=1 + dig_cmd +qid=$i @10.53.0.2 cyclic.example >dig.out.cyclic || ret=1 if [ $i -le 4 ]; then cp dig.out.cyclic dig.out.$j else @@ -125,7 +125,7 @@ ret=0 matches=0 for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do j=$((i % 4)) - dig_cmd @10.53.0.2 cyclic2.example >dig.out.cyclic2 || ret=1 + dig_cmd +qid=$i @10.53.0.2 cyclic2.example >dig.out.cyclic2 || ret=1 if [ $i -le 4 ]; then cp dig.out.cyclic2 dig.out.$j else @@ -180,7 +180,7 @@ ret=0 matches=0 for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do j=$((i % 4)) - dig_cmd @10.53.0.2 cyclic.example >dig.out.cyclic || ret=1 + dig_cmd +qid=$i @10.53.0.2 cyclic.example >dig.out.cyclic || ret=1 if [ $i -le 4 ]; then cp dig.out.cyclic dig.out.$j else @@ -205,7 +205,7 @@ ret=0 matches=0 for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do j=$((i % 4)) - dig_cmd @10.53.0.2 cyclic2.example >dig.out.cyclic2 || ret=1 + dig_cmd +qid=$i @10.53.0.2 cyclic2.example >dig.out.cyclic2 || ret=1 if [ $i -le 4 ]; then cp dig.out.cyclic2 dig.out.$j else @@ -247,7 +247,7 @@ dig_cmd @10.53.0.3 cyclic.example >dig.out.cyclic || ret=1 matches=0 for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do j=$((i % 4)) - dig_cmd @10.53.0.3 cyclic.example >dig.out.cyclic || ret=1 + dig_cmd +qid=$i @10.53.0.3 cyclic.example >dig.out.cyclic || ret=1 if [ $i -le 4 ]; then cp dig.out.cyclic dig.out.$j else @@ -274,7 +274,7 @@ dig_cmd @10.53.0.3 cyclic2.example >dig.out.cyclic2 || ret=1 matches=0 for i in 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20; do j=$((i % 4)) - dig_cmd @10.53.0.3 cyclic2.example >dig.out.cyclic2 || ret=1 + dig_cmd +qid=$i @10.53.0.3 cyclic2.example >dig.out.cyclic2 || ret=1 if [ $i -le 4 ]; then cp dig.out.cyclic2 dig.out.$j else diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 5467a099aff..7cb67f01fe0 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -4054,8 +4054,8 @@ RRset Ordering exactly like ``cyclic``. ``cyclic`` - Records are returned in a cyclic round-robin order, rotating by one - record per query. + Records are returned in a cyclic round-robin order, offset is based + on the query ID, for speed and thread safety. ``none`` Records are returned in the order they were retrieved from the @@ -4066,7 +4066,7 @@ RRset Ordering statements are present in the configuration file used by :iscman:`named`: - If no :any:`rrset-order` statement is present in the configuration - file, the implicit default is to return all records in ``random`` + file, the implicit default is to return all records in ``cyclic`` order. - If any :any:`rrset-order` statements are present in the configuration diff --git a/lib/dns/include/dns/rdataset.h b/lib/dns/include/dns/rdataset.h index 5934d9b0ac3..b227c73f440 100644 --- a/lib/dns/include/dns/rdataset.h +++ b/lib/dns/include/dns/rdataset.h @@ -147,14 +147,6 @@ struct dns_rdataset { dns_orderopt_t order : 2; } attributes; - /*% - * the counter provides the starting point in the "cyclic" order. - * The value UINT32_MAX has a special meaning of "picking up a - * random value." in order to take care of databases that do not - * increment the counter. - */ - uint32_t count; - /* * This RRSIG RRset should be re-generated around this time. * Only valid if 'resign' attribute is set. @@ -228,12 +220,11 @@ struct dns_rdataset { }; }; -#define DNS_RDATASET_COUNT_UNDEFINED UINT32_MAX - -#define DNS_RDATASET_INIT \ - { .magic = DNS_RDATASET_MAGIC, \ - .link = ISC_LINK_INITIALIZER, \ - .count = DNS_RDATASET_COUNT_UNDEFINED } +#define DNS_RDATASET_INIT \ + { \ + .magic = DNS_RDATASET_MAGIC, \ + .link = ISC_LINK_INITIALIZER, \ + } /* clang-format off */ /* @@ -437,8 +428,8 @@ dns_rdataset_totext(dns_rdataset_t *rdataset, const dns_name_t *owner_name, isc_result_t dns_rdataset_towire(dns_rdataset_t *rdataset, const dns_name_t *owner_name, - dns_compress_t *cctx, isc_buffer_t *target, - unsigned int options, unsigned int *countp); + uint16_t id, dns_compress_t *cctx, isc_buffer_t *target, + bool partial, unsigned int options, unsigned int *countp); /*%< * Convert 'rdataset' to wire format, compressing names as specified * in 'cctx', and storing the result in 'target'. @@ -446,6 +437,8 @@ dns_rdataset_towire(dns_rdataset_t *rdataset, const dns_name_t *owner_name, * Notes: *\li The rdata cursor position will be changed. * + *\li If 'partial' is true, a partial rdataset may be written. + * *\li The number of RRs added to target will be added to *countp. * * Requires: @@ -471,28 +464,6 @@ dns_rdataset_towire(dns_rdataset_t *rdataset, const dns_name_t *owner_name, * dns_name_towire(). */ -isc_result_t -dns_rdataset_towirepartial(dns_rdataset_t *rdataset, - const dns_name_t *owner_name, dns_compress_t *cctx, - isc_buffer_t *target, unsigned int options, - unsigned int *countp, void **state); -/*%< - * Like dns_rdataset_towire() except that a partial rdataset may be written. - * - * Requires: - *\li All the requirements of dns_rdataset_towiresorted(). - * If 'state' is non NULL then the current position in the - * rdataset will be remembered if the rdataset in not - * completely written and should be passed on on subsequent - * calls (NOT CURRENTLY IMPLEMENTED). - * - * Returns: - *\li #ISC_R_SUCCESS if all of the records were written. - *\li #ISC_R_NOSPACE if unable to fit in all of the records. *countp - * will be updated to reflect the number of records - * written. - */ - isc_result_t dns_rdataset_additionaldata(dns_rdataset_t *rdataset, const dns_name_t *owner_name, diff --git a/lib/dns/include/dns/rdataslab.h b/lib/dns/include/dns/rdataslab.h index 8b6bd7ab041..da60c8d469e 100644 --- a/lib/dns/include/dns/rdataslab.h +++ b/lib/dns/include/dns/rdataslab.h @@ -96,13 +96,6 @@ struct dns_slabheader { }; dns_typepair_t typepair; - _Atomic(uint16_t) count; - /*%< - * Monotonically increased every time this rdataset is bound so that - * it is used as the base of the starting point in DNS responses - * when the "cyclic" rrset-order is required. - */ - /* resigning (zone) and TTL-cleaning (cache) */ uint16_t resign_lsb : 1; isc_stdtime_t resign; diff --git a/lib/dns/message.c b/lib/dns/message.c index c4727bab51f..e773a419d47 100644 --- a/lib/dns/message.c +++ b/lib/dns/message.c @@ -1823,7 +1823,7 @@ wrong_priority(dns_rdataset_t *rds, int pass, dns_rdatatype_t preferred_glue) { } static isc_result_t -renderset(dns_rdataset_t *rdataset, const dns_name_t *owner_name, +renderset(dns_rdataset_t *rdataset, const dns_name_t *owner_name, uint16_t id, dns_compress_t *cctx, isc_buffer_t *target, unsigned int reserved, unsigned int options, unsigned int *countp) { isc_result_t result; @@ -1836,8 +1836,8 @@ renderset(dns_rdataset_t *rdataset, const dns_name_t *owner_name, } target->length -= reserved; - result = dns_rdataset_towire(rdataset, owner_name, cctx, target, - options, countp); + result = dns_rdataset_towire(rdataset, owner_name, id, cctx, target, + false, options, countp); target->length += reserved; return result; @@ -1932,15 +1932,9 @@ dns_message_rendersection(dns_message_t *msg, dns_section_t sectionid, { st = *(msg->buffer); count = 0; - if (partial) { - result = dns_rdataset_towirepartial( - rdataset, name, msg->cctx, msg->buffer, - rd_options, &count, NULL); - } else { - result = dns_rdataset_towire( - rdataset, name, msg->cctx, msg->buffer, - rd_options, &count); - } + result = dns_rdataset_towire( + rdataset, name, msg->id, msg->cctx, msg->buffer, + partial, rd_options, &count); total += count; if (partial && result == ISC_R_NOSPACE) { msg->flags |= DNS_MESSAGEFLAG_TC; @@ -1990,15 +1984,9 @@ dns_message_rendersection(dns_message_t *msg, dns_section_t sectionid, st = *(msg->buffer); count = 0; - if (partial) { - result = dns_rdataset_towirepartial( - rds, n, msg->cctx, msg->buffer, - rd_options, &count, NULL); - } else { - result = dns_rdataset_towire( - rds, n, msg->cctx, msg->buffer, - rd_options, &count); - } + result = dns_rdataset_towire( + rds, n, msg->id, msg->cctx, msg->buffer, + partial, rd_options, &count); total += count; @@ -2151,7 +2139,7 @@ dns_message_renderend(dns_message_t *msg) { * Render. */ count = 0; - result = renderset(msg->opt, dns_rootname, msg->cctx, + result = renderset(msg->opt, dns_rootname, msg->id, msg->cctx, msg->buffer, msg->reserved, 0, &count); msg->counts[DNS_SECTION_ADDITIONAL] += count; if (result != ISC_R_SUCCESS) { @@ -2220,7 +2208,7 @@ dns_message_renderend(dns_message_t *msg) { return result; } count = 0; - result = renderset(msg->tsig, msg->tsigname, msg->cctx, + result = renderset(msg->tsig, msg->tsigname, msg->id, msg->cctx, msg->buffer, msg->reserved, 0, &count); msg->counts[DNS_SECTION_ADDITIONAL] += count; if (result != ISC_R_SUCCESS) { @@ -2244,7 +2232,7 @@ dns_message_renderend(dns_message_t *msg) { * the owner name of a SIG(0) is irrelevant, and will not * be set in a message being rendered. */ - result = renderset(msg->sig0, dns_rootname, msg->cctx, + result = renderset(msg->sig0, dns_rootname, msg->id, msg->cctx, msg->buffer, msg->reserved, 0, &count); msg->counts[DNS_SECTION_ADDITIONAL] += count; if (result != ISC_R_SUCCESS) { diff --git a/lib/dns/qpcache.c b/lib/dns/qpcache.c index 9763cadf0ef..58699ea7f44 100644 --- a/lib/dns/qpcache.c +++ b/lib/dns/qpcache.c @@ -439,13 +439,6 @@ static dns_dbmethods_t qpdb_cachemethods; static void cleanup_deadnodes_cb(void *arg); -/*% - * 'init_count' is used to initialize 'newheader->count' which in turn - * is used to determine where in the cycle rrset-order cyclic starts. - * We don't lock this as we don't care about simultaneous updates. - */ -static atomic_uint_fast16_t init_count = 0; - /* * Locking * @@ -1068,8 +1061,6 @@ bindrdataset(qpcache_t *qpdb, qpcnode_t *node, dns_slabheader_t *header, rdataset->ttl = 0; } - rdataset->count = atomic_fetch_add_relaxed(&header->count, 1); - rdataset->slab.db = (dns_db_t *)qpdb; rdataset->slab.node = (dns_dbnode_t *)node; rdataset->slab.raw = header->raw; @@ -3004,8 +2995,6 @@ qpcache_addrdataset(dns_db_t *db, dns_dbnode_t *node, dns_dbversion_t *version, DNS_SLABHEADER_SETATTR(newheader, DNS_SLABHEADERATTR_ZEROTTL); } - atomic_init(&newheader->count, - atomic_fetch_add_relaxed(&init_count, 1)); if (rdataset->attributes.prefetch) { DNS_SLABHEADER_SETATTR(newheader, DNS_SLABHEADERATTR_PREFETCH); } diff --git a/lib/dns/qpzone.c b/lib/dns/qpzone.c index 1c28adfb7d4..feac3c2a9a4 100644 --- a/lib/dns/qpzone.c +++ b/lib/dns/qpzone.c @@ -367,13 +367,6 @@ typedef struct qpdb_dbiterator { enum { full, nonsec3, nsec3only } nsec3mode; } qpdb_dbiterator_t; -/*% - * 'init_count' is used to initialize 'newheader->count' which inturn - * is used to determine where in the cycle rrset-order cyclic starts. - * We don't lock this as we don't care about simultaneous updates. - */ -static atomic_uint_fast16_t init_count = 0; - /* * Locking * @@ -991,8 +984,6 @@ bindrdataset(qpzonedb_t *qpdb, qpznode_t *node, dns_slabheader_t *header, rdataset->attributes.optout = true; } - rdataset->count = atomic_fetch_add_relaxed(&header->count, 1); - rdataset->slab.db = (dns_db_t *)qpdb; rdataset->slab.node = (dns_dbnode_t *)node; rdataset->slab.raw = header->raw; @@ -2167,7 +2158,6 @@ loading_addrdataset(void *arg, const dns_name_t *name, newheader->ttl = rdataset->ttl; newheader->trust = rdataset->trust; newheader->serial = 1; - newheader->count = 1; dns_slabheader_setownercase(newheader, name); @@ -4744,9 +4734,6 @@ qpzone_addrdataset(dns_db_t *db, dns_dbnode_t *dbnode, DNS_SLABHEADER_SETATTR(newheader, DNS_SLABHEADERATTR_ZEROTTL); } - atomic_init(&newheader->count, - atomic_fetch_add_relaxed(&init_count, 1)); - newheader->serial = version->serial; if (rdataset->attributes.resign) { DNS_SLABHEADER_SETATTR(newheader, DNS_SLABHEADERATTR_RESIGN); @@ -4855,8 +4842,6 @@ qpzone_subtractrdataset(dns_db_t *db, dns_dbnode_t *dbnode, newheader->ttl = rdataset->ttl; atomic_init(&newheader->attributes, 0); newheader->serial = version->serial; - atomic_init(&newheader->count, - atomic_fetch_add_relaxed(&init_count, 1)); if (rdataset->attributes.resign) { DNS_SLABHEADER_SETATTR(newheader, DNS_SLABHEADERATTR_RESIGN); newheader->resign = diff --git a/lib/dns/rdatalist.c b/lib/dns/rdatalist.c index 3302f3a24f1..eced445df43 100644 --- a/lib/dns/rdatalist.c +++ b/lib/dns/rdatalist.c @@ -80,7 +80,6 @@ dns_rdatalist_tordataset(dns_rdatalist_t *rdatalist, dns_rdataset_t *rdataset) { .rdlist.list = rdatalist, .link = rdataset->link, - .count = rdataset->count, .attributes = rdataset->attributes, .magic = rdataset->magic, }; diff --git a/lib/dns/rdataset.c b/lib/dns/rdataset.c index bb0a2d6edc6..3f411e49160 100644 --- a/lib/dns/rdataset.c +++ b/lib/dns/rdataset.c @@ -62,7 +62,6 @@ dns_rdataset_init(dns_rdataset_t *rdataset) { *rdataset = (dns_rdataset_t){ .magic = DNS_RDATASET_MAGIC, .link = ISC_LINK_INITIALIZER, - .count = DNS_RDATASET_COUNT_UNDEFINED, }; } @@ -78,7 +77,6 @@ dns_rdataset_invalidate(dns_rdataset_t *rdataset) { *rdataset = (dns_rdataset_t){ .magic = 0, .link = ISC_LINK_INITIALIZER, - .count = DNS_RDATASET_COUNT_UNDEFINED, }; } @@ -97,7 +95,6 @@ dns__rdataset_disassociate(dns_rdataset_t *rdataset DNS__DB_FLARG) { *rdataset = (dns_rdataset_t){ .magic = DNS_RDATASET_MAGIC, .link = ISC_LINK_INITIALIZER, - .count = DNS_RDATASET_COUNT_UNDEFINED, }; } @@ -264,8 +261,7 @@ towire_addrdata(dns_rdata_t *rdata, dns_compress_t *cctx, isc_buffer_t *target, static isc_result_t towire_question(dns_rdataset_t *rdataset, const dns_name_t *name, dns_compress_t *cctx, isc_buffer_t *target, - isc_buffer_t *rrbuffer, unsigned int options ISC_ATTR_UNUSED, - unsigned int *countp) { + isc_buffer_t *rrbuffer, unsigned int *countp) { isc_result_t result; result = dns_rdataset_first(rdataset); @@ -284,8 +280,7 @@ towire_question(dns_rdataset_t *rdataset, const dns_name_t *name, static isc_result_t towire_answer(dns_rdataset_t *rdataset, const dns_name_t *name, dns_compress_t *cctx, isc_buffer_t *target, - isc_buffer_t *rrbuffer, unsigned int options ISC_ATTR_UNUSED, - unsigned int *countp) { + isc_buffer_t *rrbuffer, uint16_t id, unsigned int *countp) { isc_result_t result; size_t start = 0, count = 0, added = 0; isc_buffer_t rdlen; @@ -299,10 +294,8 @@ towire_answer(dns_rdataset_t *rdataset, const dns_name_t *name, return result; } - if (WANT_CYCLIC(rdataset) && rdataset->type != dns_rdatatype_rrsig && - rdataset->count != DNS_RDATASET_COUNT_UNDEFINED) - { - start = rdataset->count % count; + if (WANT_CYCLIC(rdataset) && rdataset->type != dns_rdatatype_rrsig) { + start = id % count; /* Do we need larger buffer? */ if (start > ARRAY_SIZE(dns__rdataset_rdatas)) { @@ -380,11 +373,10 @@ cleanup: return result; } -static isc_result_t -towire(dns_rdataset_t *rdataset, const dns_name_t *owner_name, - dns_compress_t *cctx, isc_buffer_t *target, bool partial, - unsigned int options, unsigned int *countp, - void **state ISC_ATTR_UNUSED) { +isc_result_t +dns_rdataset_towire(dns_rdataset_t *rdataset, const dns_name_t *owner_name, + uint16_t id, dns_compress_t *cctx, isc_buffer_t *target, + bool partial, unsigned int options, unsigned int *countp) { isc_result_t result; isc_buffer_t savedbuffer = *target; isc_buffer_t rrbuffer; @@ -422,13 +414,13 @@ towire(dns_rdataset_t *rdataset, const dns_name_t *owner_name, if (rdataset->attributes.question) { result = towire_question(rdataset, name, cctx, target, - &rrbuffer, options, countp); + &rrbuffer, countp); if (result != ISC_R_SUCCESS) { goto rollback; } } else { result = towire_answer(rdataset, name, cctx, target, &rrbuffer, - options, countp); + id, countp); if (result != ISC_R_SUCCESS) { goto rollback; } @@ -449,24 +441,6 @@ rollback: return result; } -isc_result_t -dns_rdataset_towirepartial(dns_rdataset_t *rdataset, - const dns_name_t *owner_name, dns_compress_t *cctx, - isc_buffer_t *target, unsigned int options, - unsigned int *countp, void **state) { - REQUIRE(state == NULL); /* XXX remove when implemented */ - return towire(rdataset, owner_name, cctx, target, true, options, countp, - state); -} - -isc_result_t -dns_rdataset_towire(dns_rdataset_t *rdataset, const dns_name_t *owner_name, - dns_compress_t *cctx, isc_buffer_t *target, - unsigned int options, unsigned int *countp) { - return towire(rdataset, owner_name, cctx, target, false, options, - countp, NULL); -} - isc_result_t dns_rdataset_additionaldata(dns_rdataset_t *rdataset, const dns_name_t *owner_name, diff --git a/lib/dns/rdataslab.c b/lib/dns/rdataslab.c index 6991fe9187c..af7efb3591d 100644 --- a/lib/dns/rdataslab.c +++ b/lib/dns/rdataslab.c @@ -1043,7 +1043,6 @@ rdataset_getnoqname(dns_rdataset_t *rdataset, dns_name_t *name, .slab.node = node, .slab.raw = noqname->neg, .link = nsec->link, - .count = nsec->count, .attributes = nsec->attributes, .magic = nsec->magic, }; @@ -1061,7 +1060,6 @@ rdataset_getnoqname(dns_rdataset_t *rdataset, dns_name_t *name, .slab.node = node, .slab.raw = noqname->negsig, .link = nsecsig->link, - .count = nsecsig->count, .attributes = nsecsig->attributes, .magic = nsecsig->magic, }; @@ -1101,7 +1099,6 @@ rdataset_getclosest(dns_rdataset_t *rdataset, dns_name_t *name, .slab.node = node, .slab.raw = closest->neg, .link = nsec->link, - .count = nsec->count, .attributes = nsec->attributes, .magic = nsec->magic, }; @@ -1119,7 +1116,6 @@ rdataset_getclosest(dns_rdataset_t *rdataset, dns_name_t *name, .slab.node = node, .slab.raw = closest->negsig, .link = nsecsig->link, - .count = nsecsig->count, .attributes = nsecsig->attributes, .magic = nsecsig->magic, }; diff --git a/tests/dns/tsig_test.c b/tests/dns/tsig_test.c index 92d6803b146..0102603d490 100644 --- a/tests/dns/tsig_test.c +++ b/tests/dns/tsig_test.c @@ -129,8 +129,8 @@ add_tsig(dst_context_t *tsigctx, dns_tsigkey_t *key, isc_buffer_t *target, ISC_LIST_APPEND(rdatalist.rdata, &rdata, link); dns_rdataset_init(&rdataset); dns_rdatalist_tordataset(&rdatalist, &rdataset); - CHECK(dns_rdataset_towire(&rdataset, key->name, &cctx, target, 0, - &count)); + CHECK(dns_rdataset_towire(&rdataset, key->name, 0, &cctx, target, false, + 0, &count)); /* * Fixup additional record count.