| Message ID | 20210204132621.9958.27237.stgit@dceara.remote.csb |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ovn-controller: Make lflow cache size configurable. | expand |
On Thu, Feb 4, 2021 at 6:57 PM Dumitru Ceara <dceara@redhat.com> 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 <dceara@redhat.com> Acked-by: Numan Siddique <numans@ovn.org> 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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
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 <dceara@redhat.com> --- 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(-)