[ovs-dev] [PATCH ovn] lflow-cache: Auto trim when cache size is reduced significantly.

Mark Gray mark.d.gray at redhat.com
Fri Jun 11 09:22:40 UTC 2021


On 04/06/2021 11:00, Dumitru Ceara wrote:
> Due to a bug in glibc [0], M_TRIM_THRESHOLD (default 128KB) is not
> honored.  The lflow cache is one of the largest memory consumers in
> ovn-controller and it used to trim memory whenever the cache was
> flushed.  However, that required periodic intervention from the CMS
> side.
> 
> Instead, we now automatically trim memory every time the lflow cache
> utilization decreases by 50%, with a threshold of at least
> LFLOW_CACHE_TRIM_LIMIT (10000) elements in the cache.
> 
> [0] https://sourceware.org/bugzilla/show_bug.cgi?id=14827
> 
> Reported-at: https://bugzilla.redhat.com/1967882
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> ---
>  controller/lflow-cache.c | 68 ++++++++++++++++++++++-------------
>  tests/ovn-lflow-cache.at | 76 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 119 insertions(+), 25 deletions(-)
> 
> diff --git a/controller/lflow-cache.c b/controller/lflow-cache.c
> index 56ddf1075..11935e7ae 100644
> --- a/controller/lflow-cache.c
> +++ b/controller/lflow-cache.c
> @@ -40,6 +40,7 @@ COVERAGE_DEFINE(lflow_cache_delete);
>  COVERAGE_DEFINE(lflow_cache_full);
>  COVERAGE_DEFINE(lflow_cache_mem_full);
>  COVERAGE_DEFINE(lflow_cache_made_room);
> +COVERAGE_DEFINE(lflow_cache_trim);
>  
>  static const char *lflow_cache_type_names[LCACHE_T_MAX] = {
>      [LCACHE_T_CONJ_ID] = "cache-conj-id",
> @@ -49,6 +50,8 @@ static const char *lflow_cache_type_names[LCACHE_T_MAX] = {
>  
>  struct lflow_cache {
>      struct hmap entries[LCACHE_T_MAX];
> +    uint32_t n_entries;
> +    uint32_t high_watermark;
>      uint32_t capacity;
>      uint64_t mem_usage;
>      uint64_t max_mem_usage;
> @@ -63,7 +66,8 @@ struct lflow_cache_entry {
>      struct lflow_cache_value value;
>  };
>  
> -static size_t lflow_cache_n_entries__(const struct lflow_cache *lc);
> +#define LFLOW_CACHE_TRIM_LIMIT 10000

This seems to be an arbitrary value? Was there a reason for selecting
this value? Perhaps it could be a runtime tunable parameter? wdyt?
> +
>  static bool lflow_cache_make_room__(struct lflow_cache *lc,
>                                      enum lflow_cache_type type);
>  static struct lflow_cache_value *lflow_cache_add__(
> @@ -71,18 +75,18 @@ static struct lflow_cache_value *lflow_cache_add__(
>      enum lflow_cache_type type, uint64_t value_size);
>  static void lflow_cache_delete__(struct lflow_cache *lc,
>                                   struct lflow_cache_entry *lce);
> +static void lflow_cache_trim__(struct lflow_cache *lc, bool force);
>  
>  struct lflow_cache *
>  lflow_cache_create(void)
>  {
> -    struct lflow_cache *lc = xmalloc(sizeof *lc);
> +    struct lflow_cache *lc = xzalloc(sizeof *lc);
>  
>      for (size_t i = 0; i < LCACHE_T_MAX; i++) {
>          hmap_init(&lc->entries[i]);
>      }
>  
>      lc->enabled = true;
> -    lc->mem_usage = 0;
>      return lc;
>  }
>  
> @@ -101,12 +105,8 @@ lflow_cache_flush(struct lflow_cache *lc)
>          HMAP_FOR_EACH_SAFE (lce, lce_next, node, &lc->entries[i]) {
>              lflow_cache_delete__(lc, lce);
>          }
> -        hmap_shrink(&lc->entries[i]);
>      }
> -
> -#if HAVE_DECL_MALLOC_TRIM
> -    malloc_trim(0);
> -#endif
> +    lflow_cache_trim__(lc, true);
>  }
>  
>  void
> @@ -134,7 +134,7 @@ lflow_cache_enable(struct lflow_cache *lc, bool enabled, uint32_t capacity,
>      uint64_t max_mem_usage = max_mem_usage_kb * 1024;
>  
>      if ((lc->enabled && !enabled)
> -            || capacity < lflow_cache_n_entries__(lc)
> +            || capacity < lc->n_entries
>              || max_mem_usage < lc->mem_usage) {
>          lflow_cache_flush(lc);
>      }
> @@ -164,6 +164,9 @@ lflow_cache_get_stats(const struct lflow_cache *lc, struct ds *output)
>  
>      ds_put_format(output, "Enabled: %s\n",
>                    lflow_cache_is_enabled(lc) ? "true" : "false");
> +    ds_put_format(output, "%-16s: %"PRIu32"\n", "high-watermark",
> +                  lc->high_watermark);
> +    ds_put_format(output, "%-16s: %"PRIu32"\n", "total", lc->n_entries);
>      for (size_t i = 0; i < LCACHE_T_MAX; i++) {
>          ds_put_format(output, "%-16s: %"PRIuSIZE"\n",
>                        lflow_cache_type_names[i],
> @@ -254,20 +257,10 @@ lflow_cache_delete(struct lflow_cache *lc, const struct uuid *lflow_uuid)
>          COVERAGE_INC(lflow_cache_delete);
>          lflow_cache_delete__(lc, CONTAINER_OF(lcv, struct lflow_cache_entry,
>                                                value));
> +        lflow_cache_trim__(lc, false);

Should we have any concerns about the impact of doing this more
frequently now? Before, we only trimmed on flush? However, I do think
the impact will be reduced due to the watermarking.
>      }
>  }
>  
> -static size_t
> -lflow_cache_n_entries__(const struct lflow_cache *lc)
> -{
> -    size_t n_entries = 0;
> -
> -    for (size_t i = 0; i < LCACHE_T_MAX; i++) {
> -        n_entries += hmap_count(&lc->entries[i]);
> -    }
> -    return n_entries;
> -}
> -
>  static bool
>  lflow_cache_make_room__(struct lflow_cache *lc, enum lflow_cache_type type)
>  {
> @@ -319,7 +312,7 @@ lflow_cache_add__(struct lflow_cache *lc, const struct uuid *lflow_uuid,
>          return NULL;
>      }
>  
> -    if (lflow_cache_n_entries__(lc) == lc->capacity) {
> +    if (lc->n_entries == lc->capacity) {
>          if (!lflow_cache_make_room__(lc, type)) {
>              COVERAGE_INC(lflow_cache_full);
>              return NULL;
> @@ -336,17 +329,17 @@ lflow_cache_add__(struct lflow_cache *lc, const struct uuid *lflow_uuid,
>      lce->size = size;
>      lce->value.type = type;
>      hmap_insert(&lc->entries[type], &lce->node, uuid_hash(lflow_uuid));
> +    lc->n_entries++;
> +    lc->high_watermark = MAX(lc->high_watermark, lc->n_entries);
>      return &lce->value;
>  }
>  
>  static void
>  lflow_cache_delete__(struct lflow_cache *lc, struct lflow_cache_entry *lce)
>  {
> -    if (!lce) {
> -        return;
> -    }
> -
> +    ovs_assert(lc->n_entries > 0);
>      hmap_remove(&lc->entries[lce->value.type], &lce->node);
> +    lc->n_entries--;
>      switch (lce->value.type) {
>      case LCACHE_T_NONE:
>          OVS_NOT_REACHED();
> @@ -369,3 +362,28 @@ lflow_cache_delete__(struct lflow_cache *lc, struct lflow_cache_entry *lce)
>      lc->mem_usage -= lce->size;
>      free(lce);
>  }
> +
> +static void
> +lflow_cache_trim__(struct lflow_cache *lc, bool force)
> +{
> +    /* Trim if we had at least 'TRIM_LIMIT' elements at some point and if the
> +     * current usage is less than half of 'high_watermark'.
> +     */
> +    ovs_assert(lc->high_watermark >= lc->n_entries);
> +    if (!force
> +            && (lc->high_watermark <= LFLOW_CACHE_TRIM_LIMIT
> +                || lc->n_entries > lc->high_watermark / 2)) {
> +        return;
> +    }
> +
> +    COVERAGE_INC(lflow_cache_trim);
> +    for (size_t i = 0; i < LCACHE_T_MAX; i++) {
> +        hmap_shrink(&lc->entries[i]);
> +    }
> +
> +#if HAVE_DECL_MALLOC_TRIM
> +    malloc_trim(0);
> +#endif
> +
> +    lc->high_watermark = lc->n_entries;
> +}
> diff --git a/tests/ovn-lflow-cache.at b/tests/ovn-lflow-cache.at
> index e5e9ed1e8..df483c9d7 100644
> --- a/tests/ovn-lflow-cache.at
> +++ b/tests/ovn-lflow-cache.at
> @@ -12,6 +12,8 @@ AT_CHECK(
>          add matches 3 | grep -v 'Mem usage (KB)'],
>      [0], [dnl
>  Enabled: true
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -21,6 +23,8 @@ LOOKUP:
>    conj_id_ofs: 1
>    type: conj-id
>  Enabled: true
> +high-watermark  : 1
> +total           : 1
>  cache-conj-id   : 1
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -30,6 +34,8 @@ LOOKUP:
>    conj_id_ofs: 2
>    type: expr
>  Enabled: true
> +high-watermark  : 2
> +total           : 2
>  cache-conj-id   : 1
>  cache-expr      : 1
>  cache-matches   : 0
> @@ -39,6 +45,8 @@ LOOKUP:
>    conj_id_ofs: 0
>    type: matches
>  Enabled: true
> +high-watermark  : 3
> +total           : 3
>  cache-conj-id   : 1
>  cache-expr      : 1
>  cache-matches   : 1
> @@ -54,6 +62,8 @@ AT_CHECK(
>          add-del matches 3 | grep -v 'Mem usage (KB)'],
>      [0], [dnl
>  Enabled: true
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -66,6 +76,8 @@ DELETE
>  LOOKUP:
>    not found
>  Enabled: true
> +high-watermark  : 1
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -78,6 +90,8 @@ DELETE
>  LOOKUP:
>    not found
>  Enabled: true
> +high-watermark  : 1
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -90,6 +104,8 @@ DELETE
>  LOOKUP:
>    not found
>  Enabled: true
> +high-watermark  : 1
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -105,6 +121,8 @@ AT_CHECK(
>          add matches 3 | grep -v 'Mem usage (KB)'],
>      [0], [dnl
>  Enabled: false
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -113,6 +131,8 @@ ADD conj-id:
>  LOOKUP:
>    not found
>  Enabled: false
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -121,6 +141,8 @@ ADD expr:
>  LOOKUP:
>    not found
>  Enabled: false
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -129,6 +151,8 @@ ADD matches:
>  LOOKUP:
>    not found
>  Enabled: false
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -153,6 +177,8 @@ AT_CHECK(
>          flush | grep -v 'Mem usage (KB)'],
>      [0], [dnl
>  Enabled: true
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -162,6 +188,8 @@ LOOKUP:
>    conj_id_ofs: 1
>    type: conj-id
>  Enabled: true
> +high-watermark  : 1
> +total           : 1
>  cache-conj-id   : 1
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -171,6 +199,8 @@ LOOKUP:
>    conj_id_ofs: 2
>    type: expr
>  Enabled: true
> +high-watermark  : 2
> +total           : 2
>  cache-conj-id   : 1
>  cache-expr      : 1
>  cache-matches   : 0
> @@ -180,11 +210,15 @@ LOOKUP:
>    conj_id_ofs: 0
>    type: matches
>  Enabled: true
> +high-watermark  : 3
> +total           : 3
>  cache-conj-id   : 1
>  cache-expr      : 1
>  cache-matches   : 1
>  DISABLE
>  Enabled: false
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -193,6 +227,8 @@ ADD conj-id:
>  LOOKUP:
>    not found
>  Enabled: false
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -201,6 +237,8 @@ ADD expr:
>  LOOKUP:
>    not found
>  Enabled: false
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -209,11 +247,15 @@ ADD matches:
>  LOOKUP:
>    not found
>  Enabled: false
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
>  ENABLE
>  Enabled: true
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -223,6 +265,8 @@ LOOKUP:
>    conj_id_ofs: 7
>    type: conj-id
>  Enabled: true
> +high-watermark  : 1
> +total           : 1
>  cache-conj-id   : 1
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -232,6 +276,8 @@ LOOKUP:
>    conj_id_ofs: 8
>    type: expr
>  Enabled: true
> +high-watermark  : 2
> +total           : 2
>  cache-conj-id   : 1
>  cache-expr      : 1
>  cache-matches   : 0
> @@ -241,11 +287,15 @@ LOOKUP:
>    conj_id_ofs: 0
>    type: matches
>  Enabled: true
> +high-watermark  : 3
> +total           : 3
>  cache-conj-id   : 1
>  cache-expr      : 1
>  cache-matches   : 1
>  FLUSH
>  Enabled: true
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -270,6 +320,8 @@ AT_CHECK(
>          add matches 10 | grep -v 'Mem usage (KB)'],
>      [0], [dnl
>  Enabled: true
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -279,6 +331,8 @@ LOOKUP:
>    conj_id_ofs: 1
>    type: conj-id
>  Enabled: true
> +high-watermark  : 1
> +total           : 1
>  cache-conj-id   : 1
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -288,6 +342,8 @@ LOOKUP:
>    conj_id_ofs: 2
>    type: expr
>  Enabled: true
> +high-watermark  : 2
> +total           : 2
>  cache-conj-id   : 1
>  cache-expr      : 1
>  cache-matches   : 0
> @@ -297,6 +353,8 @@ LOOKUP:
>    conj_id_ofs: 0
>    type: matches
>  Enabled: true
> +high-watermark  : 3
> +total           : 3
>  cache-conj-id   : 1
>  cache-expr      : 1
>  cache-matches   : 1
> @@ -305,6 +363,8 @@ dnl
>  dnl Max capacity smaller than current usage, cache should be flushed.
>  dnl
>  Enabled: true
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -314,6 +374,8 @@ LOOKUP:
>    conj_id_ofs: 4
>    type: conj-id
>  Enabled: true
> +high-watermark  : 1
> +total           : 1
>  cache-conj-id   : 1
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -327,6 +389,8 @@ dnl Cache is full but we can evict the conj-id entry because we're adding
>  dnl an expr one.
>  dnl
>  Enabled: true
> +high-watermark  : 1
> +total           : 1
>  cache-conj-id   : 0
>  cache-expr      : 1
>  cache-matches   : 0
> @@ -340,6 +404,8 @@ dnl Cache is full but we can evict the expr entry because we're adding
>  dnl a matches one.
>  dnl
>  Enabled: true
> +high-watermark  : 1
> +total           : 1
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 1
> @@ -352,6 +418,8 @@ dnl Cache is full and we're adding a conj-id entry so we shouldn't evict
>  dnl anything else.
>  dnl
>  Enabled: true
> +high-watermark  : 1
> +total           : 1
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 1
> @@ -361,6 +429,8 @@ dnl Max memory usage smaller than current memory usage, cache should be
>  dnl flushed.
>  dnl
>  Enabled: true
> +high-watermark  : 0
> +total           : 0
>  cache-conj-id   : 0
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -370,6 +440,8 @@ LOOKUP:
>    conj_id_ofs: 8
>    type: conj-id
>  Enabled: true
> +high-watermark  : 1
> +total           : 1
>  cache-conj-id   : 1
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -382,6 +454,8 @@ dnl Cache is full and we're adding a cache entry that would go over the max
>  dnl memory limit so adding should fail.
>  dnl
>  Enabled: true
> +high-watermark  : 1
> +total           : 1
>  cache-conj-id   : 1
>  cache-expr      : 0
>  cache-matches   : 0
> @@ -394,6 +468,8 @@ dnl Cache is full and we're adding a cache entry that would go over the max
>  dnl memory limit so adding should fail.
>  dnl
>  Enabled: true
> +high-watermark  : 1
> +total           : 1
>  cache-conj-id   : 1
>  cache-expr      : 0
>  cache-matches   : 0
> 

You could add a test for the LIMIT?



More information about the dev mailing list