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 <[email protected]> >> --- >> 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
