On Thu, Feb 4, 2021 at 6:57 PM Dumitru Ceara <[email protected]> wrote: > > Add a new OVS external-id, "ovn-limit-lflow-cache", through which users > can specify the maximum size of the ovn-controller logical flow cache. > > To maintain backwards compatibility the default behavior is to not > enforce any limit on the size of the cache. > > When the cache becomes full, the rule is to prefer more "important" > cache entries over less "important" ones. That is, evict entries of > type LCACHE_T_CONJ_ID if there's no room to add an entry of type > LCACHE_T_EXPR. Similarly, evict entries of type LCACHE_T_CONJ_ID or > LCACHE_T_EXPR if there's no room to add an entry of type > LCACHE_T_MATCHES. > > Signed-off-by: Dumitru Ceara <[email protected]>
Acked-by: Numan Siddique <[email protected]> I think the above rules description can also be added as a comment in the function lflow_cache_make_room__(). Thanks Numan > --- > NEWS | 3 + > controller/chassis.c | 23 ++++++++- > controller/lflow-cache.c | 48 +++++++++++++++++- > controller/lflow-cache.h | 2 - > controller/ovn-controller.8.xml | 16 ++++++ > controller/ovn-controller.c | 8 +++ > controller/test-lflow-cache.c | 12 +++-- > tests/ovn-lflow-cache.at | 104 > +++++++++++++++++++++++++++++++++++++++ > 8 files changed, 206 insertions(+), 10 deletions(-) > > diff --git a/NEWS b/NEWS > index 2044671..6e10557 100644 > --- a/NEWS > +++ b/NEWS > @@ -16,6 +16,9 @@ Post-v20.12.0 > - Add a new option to Load_Balancer.options, "hairpin_snat_ip", to allow > users to explicitly select which source IP should be used for load > balancer hairpin traffic. > + - ovn-controller: Add a configuration knob, through OVS external-id > + "ovn-limit-lflow-cache", to allow enforcing a limit for the size of the > + logical flow cache. > > OVN v20.12.0 - 18 Dec 2020 > -------------------------- > diff --git a/controller/chassis.c b/controller/chassis.c > index b4d4b0e..c66837a 100644 > --- a/controller/chassis.c > +++ b/controller/chassis.c > @@ -49,6 +49,7 @@ struct ovs_chassis_cfg { > const char *monitor_all; > const char *chassis_macs; > const char *enable_lflow_cache; > + const char *limit_lflow_cache; > > /* Set of encap types parsed from the 'ovn-encap-type' external-id. */ > struct sset encap_type_set; > @@ -135,6 +136,12 @@ get_enable_lflow_cache(const struct smap *ext_ids) > } > > static const char * > +get_limit_lflow_cache(const struct smap *ext_ids) > +{ > + return smap_get_def(ext_ids, "ovn-limit-lflow-cache", ""); > +} > + > +static const char * > get_encap_csum(const struct smap *ext_ids) > { > return smap_get_def(ext_ids, "ovn-encap-csum", "true"); > @@ -256,6 +263,7 @@ chassis_parse_ovs_config(const struct > ovsrec_open_vswitch_table *ovs_table, > ovs_cfg->monitor_all = get_monitor_all(&cfg->external_ids); > ovs_cfg->chassis_macs = get_chassis_mac_mappings(&cfg->external_ids); > ovs_cfg->enable_lflow_cache = get_enable_lflow_cache(&cfg->external_ids); > + ovs_cfg->limit_lflow_cache = get_limit_lflow_cache(&cfg->external_ids); > > if (!chassis_parse_ovs_encap_type(encap_type, &ovs_cfg->encap_type_set)) > { > return false; > @@ -283,13 +291,16 @@ chassis_build_other_config(struct smap *config, const > char *bridge_mappings, > const char *datapath_type, const char > *cms_options, > const char *monitor_all, const char *chassis_macs, > const char *iface_types, > - const char *enable_lflow_cache, bool is_interconn) > + const char *enable_lflow_cache, > + const char *limit_lflow_cache, > + bool is_interconn) > { > smap_replace(config, "ovn-bridge-mappings", bridge_mappings); > smap_replace(config, "datapath-type", datapath_type); > smap_replace(config, "ovn-cms-options", cms_options); > smap_replace(config, "ovn-monitor-all", monitor_all); > smap_replace(config, "ovn-enable-lflow-cache", enable_lflow_cache); > + smap_replace(config, "ovn-limit-lflow-cache", limit_lflow_cache); > smap_replace(config, "iface-types", iface_types); > smap_replace(config, "ovn-chassis-mac-mappings", chassis_macs); > smap_replace(config, "is-interconn", is_interconn ? "true" : "false"); > @@ -305,6 +316,7 @@ chassis_other_config_changed(const char *bridge_mappings, > const char *monitor_all, > const char *chassis_macs, > const char *enable_lflow_cache, > + const char *limit_lflow_cache, > const struct ds *iface_types, > bool is_interconn, > const struct sbrec_chassis *chassis_rec) > @@ -344,6 +356,13 @@ chassis_other_config_changed(const char *bridge_mappings, > return true; > } > > + const char *chassis_limit_lflow_cache = > + get_limit_lflow_cache(&chassis_rec->other_config); > + > + if (strcmp(limit_lflow_cache, chassis_limit_lflow_cache)) { > + return true; > + } > + > const char *chassis_mac_mappings = > get_chassis_mac_mappings(&chassis_rec->other_config); > if (strcmp(chassis_macs, chassis_mac_mappings)) { > @@ -523,6 +542,7 @@ chassis_update(const struct sbrec_chassis *chassis_rec, > ovs_cfg->monitor_all, > ovs_cfg->chassis_macs, > ovs_cfg->enable_lflow_cache, > + ovs_cfg->limit_lflow_cache, > &ovs_cfg->iface_types, > ovs_cfg->is_interconn, > chassis_rec)) { > @@ -536,6 +556,7 @@ chassis_update(const struct sbrec_chassis *chassis_rec, > ovs_cfg->chassis_macs, > ds_cstr_ro(&ovs_cfg->iface_types), > ovs_cfg->enable_lflow_cache, > + ovs_cfg->limit_lflow_cache, > ovs_cfg->is_interconn); > sbrec_chassis_verify_other_config(chassis_rec); > sbrec_chassis_set_other_config(chassis_rec, &other_config); > diff --git a/controller/lflow-cache.c b/controller/lflow-cache.c > index 2453b10..342b20a 100644 > --- a/controller/lflow-cache.c > +++ b/controller/lflow-cache.c > @@ -37,6 +37,8 @@ COVERAGE_DEFINE(lflow_cache_add); > COVERAGE_DEFINE(lflow_cache_hit); > COVERAGE_DEFINE(lflow_cache_miss); > COVERAGE_DEFINE(lflow_cache_delete); > +COVERAGE_DEFINE(lflow_cache_full); > +COVERAGE_DEFINE(lflow_cache_made_room); > > const char *lflow_cache_type_names[LCACHE_T_MAX] = { > [LCACHE_T_CONJ_ID] = "cache-conj-id", > @@ -46,6 +48,7 @@ const char *lflow_cache_type_names[LCACHE_T_MAX] = { > > struct lflow_cache { > struct hmap entries[LCACHE_T_MAX]; > + uint32_t capacity; > bool enabled; > }; > > @@ -56,6 +59,9 @@ 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__( > struct lflow_cache *lc, > const struct sbrec_logical_flow *lflow, > @@ -114,16 +120,18 @@ lflow_cache_destroy(struct lflow_cache *lc) > } > > void > -lflow_cache_enable(struct lflow_cache *lc, bool enabled) > +lflow_cache_enable(struct lflow_cache *lc, bool enabled, uint32_t capacity) > { > if (!lc) { > return; > } > > - if (lc->enabled && !enabled) { > + if ((lc->enabled && !enabled) || capacity < lflow_cache_n_entries__(lc)) > { > lflow_cache_flush(lc); > } > + > lc->enabled = enabled; > + lc->capacity = capacity; > } > > bool > @@ -236,6 +244,33 @@ lflow_cache_delete(struct lflow_cache *lc, > } > } > > +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) > +{ > + for (size_t i = 0; i < type; i++) { > + if (hmap_count(&lc->entries[i]) > 0) { > + struct lflow_cache_entry *lce = > + CONTAINER_OF(hmap_first(&lc->entries[i]), > + struct lflow_cache_entry, node); > + > + lflow_cache_delete__(lc, lce); > + return true; > + } > + } > + return false; > +} > + > static struct lflow_cache_value * > lflow_cache_add__(struct lflow_cache *lc, > const struct sbrec_logical_flow *lflow, > @@ -245,6 +280,15 @@ lflow_cache_add__(struct lflow_cache *lc, > return NULL; > } > > + if (lflow_cache_n_entries__(lc) == lc->capacity) { > + if (!lflow_cache_make_room__(lc, type)) { > + COVERAGE_INC(lflow_cache_full); > + return NULL; > + } else { > + COVERAGE_INC(lflow_cache_made_room); > + } > + } > + > struct lflow_cache_entry *lce = xzalloc(sizeof *lce); > > COVERAGE_INC(lflow_cache_add); > diff --git a/controller/lflow-cache.h b/controller/lflow-cache.h > index 54873ee..23c64a0 100644 > --- a/controller/lflow-cache.h > +++ b/controller/lflow-cache.h > @@ -61,7 +61,7 @@ struct lflow_cache_stats { > struct lflow_cache *lflow_cache_create(void); > void lflow_cache_flush(struct lflow_cache *); > void lflow_cache_destroy(struct lflow_cache *); > -void lflow_cache_enable(struct lflow_cache *, bool enabled); > +void lflow_cache_enable(struct lflow_cache *, bool enabled, uint32_t > capacity); > bool lflow_cache_is_enabled(struct lflow_cache *); > struct lflow_cache_stats *lflow_cache_get_stats(const struct lflow_cache *); > > diff --git a/controller/ovn-controller.8.xml b/controller/ovn-controller.8.xml > index 29833c7..e92db6d 100644 > --- a/controller/ovn-controller.8.xml > +++ b/controller/ovn-controller.8.xml > @@ -249,6 +249,22 @@ > processing the southbound and local Open vSwitch database changes. > The default value is considered false if this option is not defined. > </dd> > + > + <dt><code>external_ids:ovn-enable-lflow-cache</code></dt> > + <dd> > + The boolean flag indicates if <code>ovn-controller</code> should > + enable/disable the logical flow in-memory cache it uses when > + processing Southbound database logical flow changes. By default > + caching is enabled. > + </dd> > + > + <dt><code>external_ids:ovn-limit-lflow-cache</code></dt> > + <dd> > + When used, this configuration value determines the maximum number of > + logical flow cache entries <code>ovn-controller</code> may create > + when the logical flow cache is enabled. By default the size of the > + cache is unlimited. > + </dd> > </dl> > > <p> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 6ee6119..dd67d7f 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -96,6 +96,9 @@ static unixctl_cb_func debug_delay_nb_cfg_report; > static char *parse_options(int argc, char *argv[]); > OVS_NO_RETURN static void usage(void); > > +/* By default don't set an upper bound for the lflow cache. */ > +#define DEFAULT_LFLOW_CACHE_MAX_ENTRIES UINT32_MAX > + > struct controller_engine_ctx { > struct lflow_cache *lflow_cache; > }; > @@ -582,7 +585,10 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl > *ovnsb_idl, > lflow_cache_enable(ctx->lflow_cache, > smap_get_bool(&cfg->external_ids, > "ovn-enable-lflow-cache", > - true)); > + true), > + smap_get_uint(&cfg->external_ids, > + "ovn-limit-lflow-cache", > + DEFAULT_LFLOW_CACHE_MAX_ENTRIES)); > } > } > > diff --git a/controller/test-lflow-cache.c b/controller/test-lflow-cache.c > index d6e962e..45262b4 100644 > --- a/controller/test-lflow-cache.c > +++ b/controller/test-lflow-cache.c > @@ -108,7 +108,7 @@ test_lflow_cache_operations(struct ovs_cmdl_context *ctx) > unsigned int shift = 2; > unsigned int n_ops; > > - lflow_cache_enable(lc, enabled); > + lflow_cache_enable(lc, enabled, UINT32_MAX); > test_lflow_cache_stats__(lc); > > if (!test_read_uint_value(ctx, shift++, "n_ops", &n_ops)) { > @@ -156,11 +156,15 @@ test_lflow_cache_operations(struct ovs_cmdl_context > *ctx) > test_lflow_cache_delete__(lc, &lflow); > test_lflow_cache_lookup__(lc, &lflow); > } else if (!strcmp(op, "enable")) { > + unsigned int limit; > + if (!test_read_uint_value(ctx, shift++, "limit", &limit)) { > + goto done; > + } > printf("ENABLE\n"); > - lflow_cache_enable(lc, true); > + lflow_cache_enable(lc, true, limit); > } else if (!strcmp(op, "disable")) { > printf("DISABLE\n"); > - lflow_cache_enable(lc, false); > + lflow_cache_enable(lc, false, UINT32_MAX); > } else if (!strcmp(op, "flush")) { > printf("FLUSH\n"); > lflow_cache_flush(lc); > @@ -179,7 +183,7 @@ test_lflow_cache_negative(struct ovs_cmdl_context *ctx > OVS_UNUSED) > { > lflow_cache_flush(NULL); > lflow_cache_destroy(NULL); > - lflow_cache_enable(NULL, true); > + lflow_cache_enable(NULL, true, UINT32_MAX); > ovs_assert(!lflow_cache_is_enabled(NULL)); > ovs_assert(!lflow_cache_get_stats(NULL)); > > diff --git a/tests/ovn-lflow-cache.at b/tests/ovn-lflow-cache.at > index f7e8959..2cbc1f6 100644 > --- a/tests/ovn-lflow-cache.at > +++ b/tests/ovn-lflow-cache.at > @@ -146,7 +146,7 @@ AT_CHECK( > add conj-id 4 \ > add expr 5 \ > add matches 6 \ > - enable \ > + enable 1000 \ > add conj-id 7 \ > add expr 8 \ > add matches 9 \ > @@ -252,6 +252,108 @@ Enabled: true > ]) > AT_CLEANUP > > +AT_SETUP([ovn -- unit test -- lflow-cache set limit]) > +AT_CHECK( > + [ovstest test-lflow-cache lflow_cache_operations \ > + true 8 \ > + add conj-id 1 \ > + add expr 2 \ > + add matches 3 \ > + enable 1 \ > + add conj-id 4 \ > + add expr 5 \ > + add matches 6 \ > + add conj-id 7], > + [0], [dnl > +Enabled: true > + cache-conj-id: 0 > + cache-expr: 0 > + cache-matches: 0 > +ADD conj-id: > + conj-id-ofs: 1 > +LOOKUP: > + conj_id_ofs: 1 > + type: conj-id > +Enabled: true > + cache-conj-id: 1 > + cache-expr: 0 > + cache-matches: 0 > +ADD expr: > + conj-id-ofs: 2 > +LOOKUP: > + conj_id_ofs: 2 > + type: expr > +Enabled: true > + cache-conj-id: 1 > + cache-expr: 1 > + cache-matches: 0 > +ADD matches: > + conj-id-ofs: 3 > +LOOKUP: > + conj_id_ofs: 0 > + type: matches > +Enabled: true > + cache-conj-id: 1 > + cache-expr: 1 > + cache-matches: 1 > +ENABLE > +dnl > +dnl Max capacity smaller than current usage, cache should be flushed. > +dnl > +Enabled: true > + cache-conj-id: 0 > + cache-expr: 0 > + cache-matches: 0 > +ADD conj-id: > + conj-id-ofs: 4 > +LOOKUP: > + conj_id_ofs: 4 > + type: conj-id > +Enabled: true > + cache-conj-id: 1 > + cache-expr: 0 > + cache-matches: 0 > +ADD expr: > + conj-id-ofs: 5 > +LOOKUP: > + conj_id_ofs: 5 > + type: expr > +dnl > +dnl Cache is full but we can evict the conj-id entry because we're adding > +dnl an expr one. > +dnl > +Enabled: true > + cache-conj-id: 0 > + cache-expr: 1 > + cache-matches: 0 > +ADD matches: > + conj-id-ofs: 6 > +LOOKUP: > + conj_id_ofs: 0 > + type: matches > +dnl > +dnl Cache is full but we can evict the expr entry because we're adding > +dnl a matches one. > +dnl > +Enabled: true > + cache-conj-id: 0 > + cache-expr: 0 > + cache-matches: 1 > +ADD conj-id: > + conj-id-ofs: 7 > +LOOKUP: > + not found > +dnl > +dnl Cache is full and we're adding a conj-id entry so we shouldn't evict > +dnl anything else. > +dnl > +Enabled: true > + cache-conj-id: 0 > + cache-expr: 0 > + cache-matches: 1 > +]) > +AT_CLEANUP > + > AT_SETUP([ovn -- unit test -- lflow-cache negative tests]) > AT_CHECK([ovstest test-lflow-cache lflow_cache_negative], [0], []) > AT_CLEANUP > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
