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

diff --git a/lib/selection.c b/lib/selection.c
index b9997e92d5b493451f328998eaa6a7406e74ccf7..6dad1a29697a387d0204f2c476a52a804a6a8958 100644 (file)
--- 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 f8e2730e55697af3f8e2b647af4fcfb2974e0772..31a677106b893dfa3e5035a98a9cc5a7def7eed4 100644 (file)
--- 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 e0ee43df1544d7c1e75d00c90fb0311ebcca926a..31ff147673dfbe86ce68ddec066d98af0d76d657 100644 (file)
--- 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