From cdf8700c3f16b2825f7b9ac318959ba8c6e54333 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Vladim=C3=ADr=20=C4=8Cun=C3=A1t?= Date: Mon, 28 Dec 2020 10:09:18 +0100 Subject: [PATCH] lib/selection: tweak how cache is used - standardize cache key choice and ensure impossibility of collisions - comment on interaction with GC; it would be better to give RTT priority over most of other records - be more robust wrt. value in cache --- lib/selection.c | 36 ++++++++++++++++++------------------ lib/selection.h | 4 ++++ utils/cache_gc/db.c | 3 +++ 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/lib/selection.c b/lib/selection.c index b9997e92d..6dad1a296 100644 --- a/lib/selection.c +++ b/lib/selection.c @@ -40,18 +40,21 @@ /* Simple cache interface follows */ -#define KEY_PREFIX 'S' - -void *prefix_key(const uint8_t *ip, size_t len) +static knot_db_val_t cache_key(const uint8_t *ip, size_t len) { - void *key = malloc(len + 1); - *(char *)key = KEY_PREFIX; - memcpy((uint8_t *)key + 1, ip, len); + // CACHE_KEY_DEF: '\0' + 'S' + raw IP + const size_t key_len = len + 2; + uint8_t *key_data = malloc(key_len); + key_data[0] = '\0'; + key_data[1] = 'S'; + memcpy(key_data + 2, ip, len); + knot_db_val_t key = { + .len = key_len, + .data = key_data, + }; return key; } -#undef PREFIX - /* First value of timeout will be calculated as SRTT+4*DEFAULT_TIMEOUT * by calc_timeout(), so it'll be equal to DEFAULT_TIMEOUT. */ static const struct rtt_state default_rtt_state = { .srtt = 0, @@ -60,9 +63,6 @@ static const struct rtt_state default_rtt_state = { .srtt = 0, .consecutive_timeouts = 0, .dead_since = 0 }; -/* Note that this opens a cace transaction, which is usually closed by calling - * `put_rtt_state` i.e. callee is responsible for its closing - * (e.g. calling kr_cache_commit). */ struct rtt_state get_rtt_state(const uint8_t *ip, size_t len, struct kr_cache *cache) { @@ -70,18 +70,19 @@ struct rtt_state get_rtt_state(const uint8_t *ip, size_t len, knot_db_val_t value; knot_db_t *db = cache->db; struct kr_cdb_stats *stats = &cache->stats; - uint8_t *prefixed_ip = prefix_key(ip, len); - knot_db_val_t key = { .len = len + 1, .data = prefixed_ip }; + knot_db_val_t key = cache_key(ip, len); if (cache->api->read(db, stats, &key, &value, 1)) { state = default_rtt_state; + } else if (value.len != sizeof(struct rtt_state)) { + assert(false); // shouldn't happen but let's be more robust + state = default_rtt_state; } else { - assert(value.len == sizeof(struct rtt_state)); state = *(struct rtt_state *)value.data; } - free(prefixed_ip); + free(key.data); return state; } @@ -90,16 +91,15 @@ int put_rtt_state(const uint8_t *ip, size_t len, struct rtt_state state, { knot_db_t *db = cache->db; struct kr_cdb_stats *stats = &cache->stats; - uint8_t *prefixed_ip = prefix_key(ip, len); - knot_db_val_t key = { .len = len + 1, .data = prefixed_ip }; + knot_db_val_t key = cache_key(ip, len); knot_db_val_t value = { .len = sizeof(struct rtt_state), .data = &state }; int ret = cache->api->write(db, stats, &key, &value, 1); cache->api->commit(db, stats); - free(prefixed_ip); + free(key.data); return ret; } diff --git a/lib/selection.h b/lib/selection.h index f8e2730e5..31a677106 100644 --- a/lib/selection.h +++ b/lib/selection.h @@ -209,6 +209,10 @@ void error(struct kr_query *qry, struct address_state *addr_state, /** * Get RTT state from cache. Returns `default_rtt_state` on unknown addresses. + * + * Note that this opens a cache transaction which is usually closed by calling + * `put_rtt_state`, i.e. callee is responsible for its closing + * (e.g. calling kr_cache_commit). */ struct rtt_state get_rtt_state(const uint8_t *ip, size_t len, struct kr_cache *cache); diff --git a/utils/cache_gc/db.c b/utils/cache_gc/db.c index e0ee43df1..31ff14767 100644 --- a/utils/cache_gc/db.c +++ b/utils/cache_gc/db.c @@ -94,6 +94,7 @@ const uint16_t *kr_gc_key_consistent(knot_db_val_t key) return &NSEC1; case '3': return &NSEC3; + case 'S': // we don't GC the rtt_state entries, at least for now default: return NULL; } @@ -206,6 +207,8 @@ int kr_gc_cache_iter(knot_db_t * knot_db, const kr_cache_gc_cfg_t *cfg, entry = val2entry(val, *entry_type); } /* TODO: perhaps improve some details around here: + * - rtt_state entries are considered gc_inconsistent; + * therefore they'll be the first to get freed (see kr_gc_categorize()) * - xNAME have .rrtype NS * - DNAME hidden on NS name will not be considered here * - if zone has NSEC* meta-data but no NS, it will be seen -- 2.47.3