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

Dumitru Ceara dceara at redhat.com
Fri Jun 18 19:14:55 UTC 2021


On 6/18/21 9:11 PM, Mark Michelson wrote:
> Hi Dumitru,
> 
> I only had one finding down below.
> 
> On 6/15/21 9:06 AM, 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.
>>
>> The percentage of the high watermark under which memory is trimmed
>> automatically as well as the lflow-cache minimum number of elements
>> above which trimming is performed are configurable through OVS external
>> IDs.
>>
>> [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>
>> ---
>>   NEWS                            |    4 +
>>   controller/chassis.c            |   38 +++++
>>   controller/lflow-cache.c        |  104 +++++++++++----
>>   controller/lflow-cache.h        |    3
>>   controller/ovn-controller.8.xml |   17 ++
>>   controller/ovn-controller.c     |   14 ++
>>   controller/test-lflow-cache.c   |   61 +++++++--
>>   tests/ovn-lflow-cache.at        |  273
>> +++++++++++++++++++++++++++++++++++++++
>>   8 files changed, 474 insertions(+), 40 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index 0da7d8f97..79f562f1e 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -25,6 +25,10 @@ OVN v21.06.0 - 11 May 2021
>>       * ovn-sbctl now also supports daemon mode.
>>     - Added support in native DNS to respond to PTR request types.
>>     - New --dry-run option for ovn-northd and ovn-northd-ddlog.
>> +  - ovn-controller: Add configuration knobs, through OVS external-id
>> +    "ovn-trim-limit-lflow-cache" and
>> "ovn-trim-wmark-perc-lflow-cache", to
>> +    allow enforcing a lflow cache size limit and high watermark
>> percentage
>> +    for which automatic memory trimming is performed.
>>     OVN v21.03.0 - 12 Mar 2021
>>   -------------------------
>> diff --git a/controller/chassis.c b/controller/chassis.c
>> index 80d516f49..c11c10a34 100644
>> --- a/controller/chassis.c
>> +++ b/controller/chassis.c
>> @@ -52,6 +52,8 @@ struct ovs_chassis_cfg {
>>       const char *enable_lflow_cache;
>>       const char *limit_lflow_cache;
>>       const char *memlimit_lflow_cache;
>> +    const char *trim_limit_lflow_cache;
>> +    const char *trim_wmark_perc_lflow_cache;
>>         /* Set of encap types parsed from the 'ovn-encap-type'
>> external-id. */
>>       struct sset encap_type_set;
>> @@ -149,6 +151,18 @@ get_memlimit_lflow_cache(const struct smap *ext_ids)
>>       return smap_get_def(ext_ids, "ovn-memlimit-lflow-cache-kb", "");
>>   }
>>   +static const char *
>> +get_trim_limit_lflow_cache(const struct smap *ext_ids)
>> +{
>> +    return smap_get_def(ext_ids, "ovn-trim-limit-lflow-cache", "");
>> +}
>> +
>> +static const char *
>> +get_trim_wmark_perc_lflow_cache(const struct smap *ext_ids)
>> +{
>> +    return smap_get_def(ext_ids, "ovn-trim-wmark-perc-lflow-cache", "");
>> +}
>> +
>>   static const char *
>>   get_encap_csum(const struct smap *ext_ids)
>>   {
>> @@ -274,6 +288,10 @@ chassis_parse_ovs_config(const struct
>> ovsrec_open_vswitch_table *ovs_table,
>>       ovs_cfg->limit_lflow_cache =
>> get_limit_lflow_cache(&cfg->external_ids);
>>       ovs_cfg->memlimit_lflow_cache =
>>           get_memlimit_lflow_cache(&cfg->external_ids);
>> +    ovs_cfg->trim_limit_lflow_cache =
>> +        get_trim_limit_lflow_cache(&cfg->external_ids);
>> +    ovs_cfg->trim_wmark_perc_lflow_cache =
>> +        get_trim_wmark_perc_lflow_cache(&cfg->external_ids);
>>         if (!chassis_parse_ovs_encap_type(encap_type,
>> &ovs_cfg->encap_type_set)) {
>>           return false;
>> @@ -314,6 +332,10 @@ chassis_build_other_config(const struct
>> ovs_chassis_cfg *ovs_cfg,
>>       smap_replace(config, "ovn-limit-lflow-cache",
>> ovs_cfg->limit_lflow_cache);
>>       smap_replace(config, "ovn-memlimit-lflow-cache-kb",
>>                    ovs_cfg->memlimit_lflow_cache);
>> +    smap_replace(config, "ovn-trim-limit-lflow-cache",
>> +                 ovs_cfg->trim_limit_lflow_cache);
>> +    smap_replace(config, "ovn-trim-wmark-perc-lflow-cache",
>> +                 ovs_cfg->trim_wmark_perc_lflow_cache);
>>       smap_replace(config, "iface-types",
>> ds_cstr_ro(&ovs_cfg->iface_types));
>>       smap_replace(config, "ovn-chassis-mac-mappings",
>> ovs_cfg->chassis_macs);
>>       smap_replace(config, "is-interconn",
>> @@ -377,6 +399,22 @@ chassis_other_config_changed(const struct
>> ovs_chassis_cfg *ovs_cfg,
>>           return true;
>>       }
>>   +    const char *chassis_trim_limit_lflow_cache =
>> +        get_trim_limit_lflow_cache(&chassis_rec->other_config);
>> +
>> +    if (strcmp(ovs_cfg->trim_limit_lflow_cache,
>> +               chassis_trim_limit_lflow_cache)) {
>> +        return true;
>> +    }
>> +
>> +    const char *chassis_trim_wmark_perc_lflow_cache =
>> +        get_trim_wmark_perc_lflow_cache(&chassis_rec->other_config);
>> +
>> +    if (strcmp(ovs_cfg->trim_wmark_perc_lflow_cache,
>> +               chassis_trim_wmark_perc_lflow_cache)) {
>> +        return true;
>> +    }
>> +
>>       const char *chassis_mac_mappings =
>>           get_chassis_mac_mappings(&chassis_rec->other_config);
>>       if (strcmp(ovs_cfg->chassis_macs, chassis_mac_mappings)) {
>> diff --git a/controller/lflow-cache.c b/controller/lflow-cache.c
>> index 56ddf1075..30369b962 100644
>> --- a/controller/lflow-cache.c
>> +++ b/controller/lflow-cache.c
>> @@ -24,8 +24,11 @@
>>   #include "coverage.h"
>>   #include "lflow-cache.h"
>>   #include "lib/uuid.h"
>> +#include "openvswitch/vlog.h"
>>   #include "ovn/expr.h"
>>   +VLOG_DEFINE_THIS_MODULE(lflow_cache);
>> +
>>   COVERAGE_DEFINE(lflow_cache_flush);
>>   COVERAGE_DEFINE(lflow_cache_add_conj_id);
>>   COVERAGE_DEFINE(lflow_cache_add_expr);
>> @@ -40,6 +43,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",
>> @@ -47,11 +51,18 @@ static const char
>> *lflow_cache_type_names[LCACHE_T_MAX] = {
>>       [LCACHE_T_MATCHES] = "cache-matches",
>>   };
>>   +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> +
>>   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;
>> +    uint32_t trim_limit;
>> +    uint32_t trim_wmark_perc;
>> +    uint64_t trim_count;
>>       bool enabled;
>>   };
>>   @@ -63,7 +74,6 @@ struct lflow_cache_entry {
>>       struct lflow_cache_value value;
>>   };
>>   -static size_t lflow_cache_n_entries__(const struct lflow_cache *lc);
>>   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 +81,17 @@ 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 +110,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
>> @@ -125,23 +130,45 @@ lflow_cache_destroy(struct lflow_cache *lc)
>>     void
>>   lflow_cache_enable(struct lflow_cache *lc, bool enabled, uint32_t
>> capacity,
>> -                   uint64_t max_mem_usage_kb)
>> +                   uint64_t max_mem_usage_kb, uint32_t lflow_trim_limit,
>> +                   uint32_t trim_wmark_perc)
>>   {
>>       if (!lc) {
>>           return;
>>       }
>>   +    if (trim_wmark_perc > 100) {
>> +        VLOG_WARN_RL(&rl, "Invalid requested trim watermark
>> percentage: "
>> +                     "requested %"PRIu32", using 100 instead",
>> +                     trim_wmark_perc);
>> +        trim_wmark_perc = 100;
>> +    }
>> +
>>       uint64_t max_mem_usage = max_mem_usage_kb * 1024;
>> +    bool need_flush = false;
>> +    bool need_trim = false;
>>         if ((lc->enabled && !enabled)
>> -            || capacity < lflow_cache_n_entries__(lc)
>> +            || capacity < lc->n_entries
>>               || max_mem_usage < lc->mem_usage) {
>> -        lflow_cache_flush(lc);
>> +        need_flush = true;
>> +    } else if (lc->enabled
>> +                    && (lc->trim_limit != lflow_trim_limit
>> +                        || lc->trim_wmark_perc != trim_wmark_perc)) {
>> +        need_trim = false;
> 
> Should this be "need_trim = true;"? Currently, it's impossible for
> need_trim to ever be true in this function.
> 

Ugh, you're right, sorry about that.  I'll also add a test to exercise
this.  Thanks for the review!

Regards,
Dumitru



More information about the dev mailing list