[ovs-dev] [PATCH ovn v2 03/10] lflow-cache: Move the lflow cache to its own module.
Numan Siddique
numans at ovn.org
Mon Feb 8 14:13:10 UTC 2021
On Thu, Feb 4, 2021 at 6:56 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> This abstracts the implementation details of the ovn-controller logical
> flow cache and also has refactors how the cache is being used allowing
> further commits to expand its functionality.
>
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
Hi Dumitru,
Acked-by: Numan Siddique <numans at ovn.org>
Please see below for a comment.
Thanks
Numan
> ---
> controller/automake.mk | 2
> controller/lflow-cache.c | 230 +++++++++++++++++++++++++++++
> controller/lflow-cache.h | 77 ++++++++++
> controller/lflow.c | 336 ++++++++++++++-----------------------------
> controller/lflow.h | 8 -
> controller/ovn-controller.c | 57 +++----
> lib/expr.c | 4 +
> 7 files changed, 447 insertions(+), 267 deletions(-)
> create mode 100644 controller/lflow-cache.c
> create mode 100644 controller/lflow-cache.h
>
> diff --git a/controller/automake.mk b/controller/automake.mk
> index 480578e..75119a6 100644
> --- a/controller/automake.mk
> +++ b/controller/automake.mk
> @@ -14,6 +14,8 @@ controller_ovn_controller_SOURCES = \
> controller/ip-mcast.h \
> controller/lflow.c \
> controller/lflow.h \
> + controller/lflow-cache.c \
> + controller/lflow-cache.h \
> controller/lport.c \
> controller/lport.h \
> controller/ofctrl.c \
> diff --git a/controller/lflow-cache.c b/controller/lflow-cache.c
> new file mode 100644
> index 0000000..ce129ac
> --- /dev/null
> +++ b/controller/lflow-cache.c
> @@ -0,0 +1,230 @@
> +/*
> + * Copyright (c) 2015, 2016 Nicira, Inc.
> + * Copyright (c) 2021, Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +
> +#include "lib/ovn-sb-idl.h"
> +#include "lflow-cache.h"
> +#include "ovn/expr.h"
> +
> +struct lflow_cache {
> + struct hmap entries[LCACHE_T_MAX];
> + bool enabled;
> +};
> +
> +struct lflow_cache_entry {
> + struct hmap_node node;
> + struct uuid lflow_uuid; /* key */
> +
> + struct lflow_cache_value value;
> +};
> +
> +static struct lflow_cache_value *lflow_cache_add__(
> + struct lflow_cache *lc,
> + const struct sbrec_logical_flow *lflow,
> + enum lflow_cache_type type);
> +static void lflow_cache_delete__(struct lflow_cache *lc,
> + struct lflow_cache_entry *lce);
> +
> +struct lflow_cache *
> +lflow_cache_create(void)
> +{
> + struct lflow_cache *lc = xmalloc(sizeof *lc);
> +
> + for (size_t i = 0; i < LCACHE_T_MAX; i++) {
> + hmap_init(&lc->entries[i]);
> + }
> +
> + lc->enabled = true;
> + return lc;
> +}
> +
> +void
> +lflow_cache_flush(struct lflow_cache *lc)
> +{
> + if (!lc) {
> + return;
> + }
> +
> + for (size_t i = 0; i < LCACHE_T_MAX; i++) {
> + struct lflow_cache_entry *lce;
> + struct lflow_cache_entry *lce_next;
> +
> + HMAP_FOR_EACH_SAFE (lce, lce_next, node, &lc->entries[i]) {
> + lflow_cache_delete__(lc, lce);
> + }
> + }
> +}
> +
> +void
> +lflow_cache_destroy(struct lflow_cache *lc)
> +{
> + if (!lc) {
> + return;
> + }
> +
> + lflow_cache_flush(lc);
> + for (size_t i = 0; i < LCACHE_T_MAX; i++) {
> + hmap_destroy(&lc->entries[i]);
> + }
> + free(lc);
> +}
> +
> +void
> +lflow_cache_enable(struct lflow_cache *lc, bool enabled)
> +{
> + if (!lc) {
> + return;
> + }
> +
> + if (lc->enabled && !enabled) {
> + lflow_cache_flush(lc);
> + }
> + lc->enabled = enabled;
> +}
> +
> +bool
> +lflow_cache_is_enabled(struct lflow_cache *lc)
> +{
> + return lc && lc->enabled;
> +}
> +
> +void
> +lflow_cache_add_conj_id(struct lflow_cache *lc,
> + const struct sbrec_logical_flow *lflow,
> + uint32_t conj_id_ofs)
'struct sbrec_logical_flow' is used in the entire file for the lflow uuid.
I'd suggest for all these functions which take 'sbrec_logical_flow' as
an argument
to instead take - 'struct uuid lflow_uuid'.
Since we are planning to refactor the code base to separate the IDL access,
it would help in that regard.
Numan
> +{
> + struct lflow_cache_value *lcv =
> + lflow_cache_add__(lc, lflow, LCACHE_T_CONJ_ID);
> +
> + if (!lcv) {
> + return;
> + }
> + lcv->conj_id_ofs = conj_id_ofs;
> +}
> +
> +void
> +lflow_cache_add_expr(struct lflow_cache *lc,
> + const struct sbrec_logical_flow *lflow,
> + uint32_t conj_id_ofs,
> + struct expr *expr)
> +{
> + struct lflow_cache_value *lcv =
> + lflow_cache_add__(lc, lflow, LCACHE_T_EXPR);
> +
> + if (!lcv) {
> + expr_destroy(expr);
> + return;
> + }
> + lcv->conj_id_ofs = conj_id_ofs;
> + lcv->expr = expr;
> +}
> +
> +void
> +lflow_cache_add_matches(struct lflow_cache *lc,
> + const struct sbrec_logical_flow *lflow,
> + struct hmap *matches)
> +{
> + struct lflow_cache_value *lcv =
> + lflow_cache_add__(lc, lflow, LCACHE_T_MATCHES);
> +
> + if (!lcv) {
> + expr_matches_destroy(matches);
> + free(matches);
> + return;
> + }
> + lcv->expr_matches = matches;
> +}
> +
> +struct lflow_cache_value *
> +lflow_cache_get(struct lflow_cache *lc, const struct sbrec_logical_flow *lflow)
> +{
> + if (!lflow_cache_is_enabled(lc)) {
> + return NULL;
> + }
> +
> + size_t hash = uuid_hash(&lflow->header_.uuid);
> +
> + for (size_t i = 0; i < LCACHE_T_MAX; i++) {
> + struct lflow_cache_entry *lce;
> +
> + HMAP_FOR_EACH_WITH_HASH (lce, node, hash, &lc->entries[i]) {
> + if (uuid_equals(&lce->lflow_uuid, &lflow->header_.uuid)) {
> + return &lce->value;
> + }
> + }
> + }
> + return NULL;
> +}
> +
> +void
> +lflow_cache_delete(struct lflow_cache *lc,
> + const struct sbrec_logical_flow *lflow)
> +{
> + if (!lc) {
> + return;
> + }
> +
> + struct lflow_cache_value *lcv = lflow_cache_get(lc, lflow);
> + if (lcv) {
> + lflow_cache_delete__(lc, CONTAINER_OF(lcv, struct lflow_cache_entry,
> + value));
> + }
> +}
> +
> +static struct lflow_cache_value *
> +lflow_cache_add__(struct lflow_cache *lc,
> + const struct sbrec_logical_flow *lflow,
> + enum lflow_cache_type type)
> +{
> + if (!lflow_cache_is_enabled(lc) || !lflow) {
> + return NULL;
> + }
> +
> + struct lflow_cache_entry *lce = xzalloc(sizeof *lce);
> +
> + lce->lflow_uuid = lflow->header_.uuid;
> + lce->value.type = type;
> + hmap_insert(&lc->entries[type], &lce->node,
> + uuid_hash(&lflow->header_.uuid));
> + return &lce->value;
> +}
> +
> +static void
> +lflow_cache_delete__(struct lflow_cache *lc, struct lflow_cache_entry *lce)
> +{
> + if (!lce) {
> + return;
> + }
> +
> + hmap_remove(&lc->entries[lce->value.type], &lce->node);
> + switch (lce->value.type) {
> + case LCACHE_T_NONE:
> + OVS_NOT_REACHED();
> + break;
> + case LCACHE_T_CONJ_ID:
> + break;
> + case LCACHE_T_EXPR:
> + expr_destroy(lce->value.expr);
> + break;
> + case LCACHE_T_MATCHES:
> + expr_matches_destroy(lce->value.expr_matches);
> + free(lce->value.expr_matches);
> + break;
> + }
> + free(lce);
> +}
> diff --git a/controller/lflow-cache.h b/controller/lflow-cache.h
> new file mode 100644
> index 0000000..74a5b81
> --- /dev/null
> +++ b/controller/lflow-cache.h
> @@ -0,0 +1,77 @@
> +/*
> + * Copyright (c) 2015, 2016 Nicira, Inc.
> + * Copyright (c) 2021, Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef LFLOW_CACHE_H
> +#define LFLOW_CACHE_H 1
> +
> +#include "openvswitch/hmap.h"
> +#include "openvswitch/uuid.h"
> +
> +struct sbrec_logical_flow;
> +struct lflow_cache;
> +
> +/* Various lflow cache types which
> + * - store the conjunction id offset if the lflow matches
> + * results in conjunctive OpenvSwitch flows.
> + *
> + * - Caches
> + * (1) Conjunction ID offset if the logical flow has port group/address
> + * set references.
> + * (2) expr tree if the logical flow has is_chassis_resident() match.
> + * (3) expr matches if (1) and (2) are false.
> + */
> +enum lflow_cache_type {
> + LCACHE_T_CONJ_ID, /* Only conjunction id offset is cached. */
> + LCACHE_T_EXPR, /* Expr tree of the logical flow is cached. */
> + LCACHE_T_MATCHES, /* Expression matches are cached. */
> + LCACHE_T_MAX,
> + LCACHE_T_NONE = LCACHE_T_MAX, /* Not found in cache. */
> +};
> +
> +struct lflow_cache_value {
> + enum lflow_cache_type type;
> + uint32_t conj_id_ofs;
> +
> + union {
> + struct hmap *expr_matches;
> + struct expr *expr;
> + };
> +};
> +
> +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);
> +bool lflow_cache_is_enabled(struct lflow_cache *);
> +
> +void lflow_cache_add_conj_id(struct lflow_cache *,
> + const struct sbrec_logical_flow *,
> + uint32_t conj_id_ofs);
> +void lflow_cache_add_expr(struct lflow_cache *,
> + const struct sbrec_logical_flow *,
> + uint32_t conj_id_ofs,
> + struct expr *expr);
> +void lflow_cache_add_matches(struct lflow_cache *,
> + const struct sbrec_logical_flow *,
> + struct hmap *matches);
> +
> +struct lflow_cache_value *lflow_cache_get(struct lflow_cache *,
> + const struct sbrec_logical_flow *);
> +void lflow_cache_delete(struct lflow_cache *,
> + const struct sbrec_logical_flow *);
> +
> +#endif /* controller/lflow-cache.h */
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 495653f..3e3605a 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -17,6 +17,7 @@
> #include "lflow.h"
> #include "coverage.h"
> #include "ha-chassis.h"
> +#include "lflow-cache.h"
> #include "lport.h"
> #include "ofctrl.h"
> #include "openvswitch/dynamic-string.h"
> @@ -306,103 +307,6 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr,
> free(lfrn);
> }
>
> -enum lflow_cache_type {
> - LCACHE_T_NO_CACHE,
> - LCACHE_T_MATCHES,
> - LCACHE_T_EXPR,
> -};
> -
> -/* Represents an lflow cache which
> - * - stores the conjunction id offset if the lflow matches
> - * results in conjunctive OpenvSwitch flows.
> - *
> - * - Caches
> - * (1) Nothing if the logical flow has port group/address set references.
> - * (2) expr tree if the logical flow has is_chassis_resident() match.
> - * (3) expr matches if (1) and (2) are false.
> - */
> -struct lflow_cache {
> - struct hmap_node node;
> - struct uuid lflow_uuid; /* key */
> - uint32_t conj_id_ofs;
> -
> - enum lflow_cache_type type;
> - union {
> - struct {
> - struct hmap *expr_matches;
> - size_t n_conjs;
> - };
> - struct expr *expr;
> - };
> -};
> -
> -static struct lflow_cache *
> -lflow_cache_add(struct hmap *lflow_cache_map,
> - const struct sbrec_logical_flow *lflow)
> -{
> - struct lflow_cache *lc = xmalloc(sizeof *lc);
> - lc->lflow_uuid = lflow->header_.uuid;
> - lc->conj_id_ofs = 0;
> - lc->type = LCACHE_T_NO_CACHE;
> - hmap_insert(lflow_cache_map, &lc->node, uuid_hash(&lc->lflow_uuid));
> - return lc;
> -}
> -
> -static struct lflow_cache *
> -lflow_cache_get(struct hmap *lflow_cache_map,
> - const struct sbrec_logical_flow *lflow)
> -{
> - struct lflow_cache *lc;
> - size_t hash = uuid_hash(&lflow->header_.uuid);
> - HMAP_FOR_EACH_WITH_HASH (lc, node, hash, lflow_cache_map) {
> - if (uuid_equals(&lc->lflow_uuid, &lflow->header_.uuid)) {
> - return lc;
> - }
> - }
> -
> - return NULL;
> -}
> -
> -static void
> -lflow_cache_delete(struct hmap *lflow_cache_map,
> - const struct sbrec_logical_flow *lflow)
> -{
> - struct lflow_cache *lc = lflow_cache_get(lflow_cache_map, lflow);
> - if (lc) {
> - hmap_remove(lflow_cache_map, &lc->node);
> - if (lc->type == LCACHE_T_MATCHES) {
> - expr_matches_destroy(lc->expr_matches);
> - free(lc->expr_matches);
> - } else if (lc->type == LCACHE_T_EXPR) {
> - expr_destroy(lc->expr);
> - }
> - free(lc);
> - }
> -}
> -
> -void
> -lflow_cache_init(struct hmap *lflow_cache_map)
> -{
> - hmap_init(lflow_cache_map);
> -}
> -
> -void
> -lflow_cache_destroy(struct hmap *lflow_cache_map)
> -{
> - struct lflow_cache *lc;
> - HMAP_FOR_EACH_POP (lc, node, lflow_cache_map) {
> - if (lc->type == LCACHE_T_MATCHES) {
> - expr_matches_destroy(lc->expr_matches);
> - free(lc->expr_matches);
> - } else if (lc->type == LCACHE_T_EXPR) {
> - expr_destroy(lc->expr);
> - }
> - free(lc);
> - }
> -
> - hmap_destroy(lflow_cache_map);
> -}
> -
> /* Adds the logical flows from the Logical_Flow table to flow tables. */
> static void
> add_logical_flows(struct lflow_ctx_in *l_ctx_in,
> @@ -496,8 +400,8 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
> ofrn->sb_uuid = lflow->header_.uuid;
> hmap_insert(&flood_remove_nodes, &ofrn->hmap_node,
> uuid_hash(&ofrn->sb_uuid));
> - if (l_ctx_out->lflow_cache_map) {
> - lflow_cache_delete(l_ctx_out->lflow_cache_map, lflow);
> + if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
> + lflow_cache_delete(l_ctx_out->lflow_cache, lflow);
> }
> }
> }
> @@ -659,10 +563,10 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
> {
> if (*conj_id_ofs + n_conjs < *conj_id_ofs) {
> /* overflow */
> - return false;
> + return true;
> }
> *conj_id_ofs += n_conjs;
> - return true;
> + return false;
> }
>
> static void
> @@ -884,152 +788,126 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
> .lfrr = l_ctx_out->lfrr,
> };
>
> - struct expr *expr = NULL;
> - if (!l_ctx_out->lflow_cache_map) {
> - /* Caching is disabled. */
> - expr = convert_match_to_expr(lflow, dp, &prereqs, l_ctx_in->addr_sets,
> - l_ctx_in->port_groups, l_ctx_out->lfrr,
> - NULL);
> - if (!expr) {
> - expr_destroy(prereqs);
> - ovnacts_free(ovnacts.data, ovnacts.size);
> - ofpbuf_uninit(&ovnacts);
> - return true;
> - }
> + struct lflow_cache_value *lcv =
> + lflow_cache_get(l_ctx_out->lflow_cache, lflow);
> + uint32_t conj_id_ofs =
> + lcv ? lcv->conj_id_ofs : *l_ctx_out->conj_id_ofs;
> + enum lflow_cache_type lcv_type =
> + lcv ? lcv->type : LCACHE_T_NONE;
>
> - expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux,
> - NULL);
> - expr = expr_normalize(expr);
> - struct hmap matches = HMAP_INITIALIZER(&matches);
> - uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux,
> - &matches);
> - expr_destroy(expr);
> - if (hmap_is_empty(&matches)) {
> - VLOG_DBG("lflow "UUID_FMT" matches are empty, skip",
> - UUID_ARGS(&lflow->header_.uuid));
> - ovnacts_free(ovnacts.data, ovnacts.size);
> - ofpbuf_uninit(&ovnacts);
> - expr_matches_destroy(&matches);
> - return true;
> - }
> -
> - expr_matches_prepare(&matches, *l_ctx_out->conj_id_ofs);
> - add_matches_to_flow_table(lflow, dp, &matches, ptable, output_ptable,
> - &ovnacts, ingress, l_ctx_in, l_ctx_out);
> -
> - ovnacts_free(ovnacts.data, ovnacts.size);
> - ofpbuf_uninit(&ovnacts);
> - expr_matches_destroy(&matches);
> - return update_conj_id_ofs(l_ctx_out->conj_id_ofs, n_conjs);
> - }
> -
> - /* Caching is enabled. */
> - struct lflow_cache *lc =
> - lflow_cache_get(l_ctx_out->lflow_cache_map, lflow);
> -
> - if (lc && lc->type == LCACHE_T_MATCHES) {
> - /* 'matches' is cached. No need to do expr parsing and no need
> - * to call expr_matches_prepare() to update the conj ids.
> - * Add matches to flow table and return. */
> - add_matches_to_flow_table(lflow, dp, lc->expr_matches, ptable,
> - output_ptable, &ovnacts, ingress,
> - l_ctx_in, l_ctx_out);
> - ovnacts_free(ovnacts.data, ovnacts.size);
> - ofpbuf_uninit(&ovnacts);
> - expr_destroy(prereqs);
> - return true;
> - }
> + struct expr *cached_expr = NULL, *expr = NULL;
> + struct hmap *matches = NULL;
>
> - if (!lc) {
> - /* Create the lflow_cache for the lflow. */
> - lc = lflow_cache_add(l_ctx_out->lflow_cache_map, lflow);
> - }
> + bool is_cr_cond_present = false;
> + bool pg_addr_set_ref = false;
> + uint32_t n_conjs = 0;
>
> - if (lc && lc->type == LCACHE_T_EXPR) {
> - expr = lc->expr;
> - }
> + bool conj_id_overflow = false;
>
> - bool pg_addr_set_ref = false;
> - if (!expr) {
> + /* Get match expr, either from cache or from lflow match. */
> + switch (lcv_type) {
> + case LCACHE_T_NONE:
> + case LCACHE_T_CONJ_ID:
> expr = convert_match_to_expr(lflow, dp, &prereqs, l_ctx_in->addr_sets,
> l_ctx_in->port_groups, l_ctx_out->lfrr,
> &pg_addr_set_ref);
> if (!expr) {
> - expr_destroy(prereqs);
> - ovnacts_free(ovnacts.data, ovnacts.size);
> - ofpbuf_uninit(&ovnacts);
> - return true;
> + goto done;
> }
> - } else {
> - expr_destroy(prereqs);
> - }
> -
> - ovs_assert(lc && expr);
> -
> - /* Cache the 'expr' only if the lflow doesn't reference a port group and
> - * address set. */
> - if (!pg_addr_set_ref) {
> - /* Note: If the expr doesn't have 'is_chassis_resident, then the
> - * type will be set to LCACHE_T_MATCHES and 'matches' will be
> - * cached instead. See below. */
> - lc->type = LCACHE_T_EXPR;
> - lc->expr = expr;
> - }
> -
> - if (lc->type == LCACHE_T_EXPR) {
> - expr = expr_clone(lc->expr);
> + break;
> + case LCACHE_T_EXPR:
> + expr = expr_clone(lcv->expr);
> + break;
> + case LCACHE_T_MATCHES:
> + break;
> }
>
> - bool is_cr_cond_present = false;
> - expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux,
> - &is_cr_cond_present);
> - expr = expr_normalize(expr);
> - struct hmap *matches = xmalloc(sizeof *matches);
> - uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux,
> - matches);
> - expr_destroy(expr);
> - if (hmap_is_empty(matches)) {
> - VLOG_DBG("lflow "UUID_FMT" matches are empty, skip",
> - UUID_ARGS(&lflow->header_.uuid));
> - ovnacts_free(ovnacts.data, ovnacts.size);
> - ofpbuf_uninit(&ovnacts);
> - expr_matches_destroy(matches);
> - free(matches);
> - return true;
> + /* If caching is enabled and this is a not cached expr that doesn't refer
> + * to address sets or port groups, save it to potentially cache it later.
> + */
> + if (lcv_type == LCACHE_T_NONE
> + && lflow_cache_is_enabled(l_ctx_out->lflow_cache)
> + && !pg_addr_set_ref) {
> + cached_expr = expr_clone(expr);
> }
>
> - if (n_conjs && !lc->conj_id_ofs) {
> - lc->conj_id_ofs = *l_ctx_out->conj_id_ofs;
> - if (!update_conj_id_ofs(l_ctx_out->conj_id_ofs, n_conjs)) {
> - lc->conj_id_ofs = 0;
> - expr_matches_destroy(matches);
> - free(matches);
> - return false;
> + /* Normalize expression if needed. */
> + switch (lcv_type) {
> + case LCACHE_T_NONE:
> + case LCACHE_T_CONJ_ID:
> + case LCACHE_T_EXPR:
> + expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux,
> + &is_cr_cond_present);
> + expr = expr_normalize(expr);
> + break;
> + case LCACHE_T_MATCHES:
> + break;
> + }
> +
> + /* Get matches, either from cache or from expr computed above. */
> + switch (lcv_type) {
> + case LCACHE_T_NONE:
> + case LCACHE_T_CONJ_ID:
> + case LCACHE_T_EXPR:
> + matches = xmalloc(sizeof *matches);
> + n_conjs = expr_to_matches(expr, lookup_port_cb, &aux, matches);
> + expr_matches_prepare(matches, conj_id_ofs);
> + if (hmap_is_empty(matches)) {
> + VLOG_DBG("lflow "UUID_FMT" matches are empty, skip",
> + UUID_ARGS(&lflow->header_.uuid));
> + goto done;
> }
> + break;
> + case LCACHE_T_MATCHES:
> + matches = lcv->expr_matches;
> + break;
> }
>
> - expr_matches_prepare(matches, lc->conj_id_ofs);
> -
> - /* Encode OVN logical actions into OpenFlow. */
> add_matches_to_flow_table(lflow, dp, matches, ptable, output_ptable,
> &ovnacts, ingress, l_ctx_in, l_ctx_out);
> +
> + /* Update cache if needed. */
> + switch (lcv_type) {
> + case LCACHE_T_NONE:
> + /* Entry not already in cache, update conjunction id offset and
> + * add the entry to the cache.
> + */
> + conj_id_overflow = update_conj_id_ofs(l_ctx_out->conj_id_ofs, n_conjs);
> +
> + /* Cache new entry if caching is enabled. */
> + if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
> + if (cached_expr && !is_cr_cond_present) {
> + lflow_cache_add_matches(l_ctx_out->lflow_cache, lflow,
> + matches);
> + matches = NULL;
> + } else if (cached_expr) {
> + lflow_cache_add_expr(l_ctx_out->lflow_cache, lflow,
> + conj_id_ofs, cached_expr);
> + cached_expr = NULL;
> + } else {
> + lflow_cache_add_conj_id(l_ctx_out->lflow_cache, lflow,
> + conj_id_ofs);
> + }
> + }
> + break;
> + case LCACHE_T_CONJ_ID:
> + case LCACHE_T_EXPR:
> + break;
> + case LCACHE_T_MATCHES:
> + /* Cached matches were used, don't destroy them. */
> + matches = NULL;
> + break;
> + }
> +
> +done:
> + expr_destroy(prereqs);
> ovnacts_free(ovnacts.data, ovnacts.size);
> ofpbuf_uninit(&ovnacts);
> -
> - if (!is_cr_cond_present && lc->type == LCACHE_T_EXPR) {
> - /* If 'is_chassis_resident' match is not present, then cache
> - * 'matches'. */
> - expr_destroy(lc->expr);
> - lc->type = LCACHE_T_MATCHES;
> - lc->expr_matches = matches;
> - }
> -
> - if (lc->type != LCACHE_T_MATCHES) {
> - expr_matches_destroy(matches);
> - free(matches);
> - }
> -
> - return true;
> + expr_destroy(expr);
> + expr_destroy(cached_expr);
> + expr_matches_destroy(matches);
> + free(matches);
> + return !conj_id_overflow;
> }
>
> static bool
> @@ -1440,14 +1318,14 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out)
> * don't exist anymore.
> */
> void
> -lflow_handle_cached_flows(struct hmap *lflow_cache_map,
> +lflow_handle_cached_flows(struct lflow_cache *lc,
> const struct sbrec_logical_flow_table *flow_table)
> {
> const struct sbrec_logical_flow *lflow;
>
> SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow, flow_table) {
> if (sbrec_logical_flow_is_deleted(lflow)) {
> - lflow_cache_delete(lflow_cache_map, lflow);
> + lflow_cache_delete(lc, lflow);
> }
> }
> }
> diff --git a/controller/lflow.h b/controller/lflow.h
> index cf4f0e8..833b42f 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -34,6 +34,7 @@
> */
>
> #include <stdint.h>
> +#include "lflow-cache.h"
> #include "openvswitch/hmap.h"
> #include "openvswitch/uuid.h"
> #include "openvswitch/list.h"
> @@ -150,14 +151,14 @@ struct lflow_ctx_out {
> struct ovn_extend_table *group_table;
> struct ovn_extend_table *meter_table;
> struct lflow_resource_ref *lfrr;
> - struct hmap *lflow_cache_map;
> + struct lflow_cache *lflow_cache;
> uint32_t *conj_id_ofs;
> bool conj_id_overflow;
> };
>
> void lflow_init(void);
> void lflow_run(struct lflow_ctx_in *, struct lflow_ctx_out *);
> -void lflow_handle_cached_flows(struct hmap *lflow_cache,
> +void lflow_handle_cached_flows(struct lflow_cache *,
> const struct sbrec_logical_flow_table *);
> bool lflow_handle_changed_flows(struct lflow_ctx_in *, struct lflow_ctx_out *);
> bool lflow_handle_changed_ref(enum ref_type, const char *ref_name,
> @@ -171,9 +172,6 @@ void lflow_handle_changed_neighbors(
> bool lflow_handle_changed_lbs(struct lflow_ctx_in *, struct lflow_ctx_out *);
> void lflow_destroy(void);
>
> -void lflow_cache_init(struct hmap *);
> -void lflow_cache_destroy(struct hmap *);
> -
> bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *,
> struct lflow_ctx_in *,
> struct lflow_ctx_out *);
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 9014674..7d247fd 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -36,6 +36,7 @@
> #include "ip-mcast.h"
> #include "openvswitch/hmap.h"
> #include "lflow.h"
> +#include "lflow-cache.h"
> #include "lib/vswitch-idl.h"
> #include "lport.h"
> #include "memory.h"
> @@ -94,6 +95,10 @@ static unixctl_cb_func debug_delay_nb_cfg_report;
> static char *parse_options(int argc, char *argv[]);
> OVS_NO_RETURN static void usage(void);
>
> +struct controller_engine_ctx {
> + struct lflow_cache *lflow_cache;
> +};
> +
> /* Pending packet to be injected into connected OVS. */
> struct pending_pkt {
> /* Setting 'conn' indicates that a request is pending. */
> @@ -530,7 +535,8 @@ get_ofctrl_probe_interval(struct ovsdb_idl *ovs_idl)
> static void
> update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl,
> bool *monitor_all_p, bool *reset_ovnsb_idl_min_index,
> - bool *enable_lflow_cache, unsigned int *sb_cond_seqno)
> + struct controller_engine_ctx *ctx,
> + unsigned int *sb_cond_seqno)
> {
> const struct ovsrec_open_vswitch *cfg = ovsrec_open_vswitch_first(ovs_idl);
> if (!cfg) {
> @@ -571,9 +577,11 @@ update_sb_db(struct ovsdb_idl *ovs_idl, struct ovsdb_idl *ovnsb_idl,
> *reset_ovnsb_idl_min_index = false;
> }
>
> - if (enable_lflow_cache != NULL) {
> - *enable_lflow_cache =
> - smap_get_bool(&cfg->external_ids, "ovn-enable-lflow-cache", true);
> + if (ctx) {
> + lflow_cache_enable(ctx->lflow_cache,
> + smap_get_bool(&cfg->external_ids,
> + "ovn-enable-lflow-cache",
> + true));
> }
> }
>
> @@ -952,10 +960,6 @@ enum ovs_engine_node {
> OVS_NODES
> #undef OVS_NODE
>
> -struct controller_engine_ctx {
> - bool enable_lflow_cache;
> -};
> -
> struct ed_type_ofctrl_is_connected {
> bool connected;
> };
> @@ -1714,8 +1718,7 @@ physical_flow_changes_ovs_iface_handler(struct engine_node *node, void *data)
>
> struct flow_output_persistent_data {
> uint32_t conj_id_ofs;
> - struct hmap lflow_cache_map;
> - bool lflow_cache_enabled;
> + struct lflow_cache *lflow_cache;
> };
>
> struct ed_type_flow_output {
> @@ -1904,11 +1907,7 @@ static void init_lflow_ctx(struct engine_node *node,
> l_ctx_out->meter_table = &fo->meter_table;
> l_ctx_out->lfrr = &fo->lflow_resource_ref;
> l_ctx_out->conj_id_ofs = &fo->pd.conj_id_ofs;
> - if (fo->pd.lflow_cache_enabled) {
> - l_ctx_out->lflow_cache_map = &fo->pd.lflow_cache_map;
> - } else {
> - l_ctx_out->lflow_cache_map = NULL;
> - }
> + l_ctx_out->lflow_cache = fo->pd.lflow_cache;
> l_ctx_out->conj_id_overflow = false;
> }
>
> @@ -1923,8 +1922,6 @@ en_flow_output_init(struct engine_node *node OVS_UNUSED,
> ovn_extend_table_init(&data->meter_table);
> data->pd.conj_id_ofs = 1;
> lflow_resource_init(&data->lflow_resource_ref);
> - lflow_cache_init(&data->pd.lflow_cache_map);
> - data->pd.lflow_cache_enabled = true;
> return data;
> }
>
> @@ -1936,7 +1933,7 @@ en_flow_output_cleanup(void *data)
> ovn_extend_table_destroy(&flow_output_data->group_table);
> ovn_extend_table_destroy(&flow_output_data->meter_table);
> lflow_resource_destroy(&flow_output_data->lflow_resource_ref);
> - lflow_cache_destroy(&flow_output_data->pd.lflow_cache_map);
> + lflow_cache_destroy(flow_output_data->pd.lflow_cache);
> }
>
> static void
> @@ -1983,13 +1980,10 @@ en_flow_output_run(struct engine_node *node, void *data)
> }
>
> struct controller_engine_ctx *ctrl_ctx = engine_get_context()->client_ctx;
> - if (fo->pd.lflow_cache_enabled && !ctrl_ctx->enable_lflow_cache) {
> - lflow_cache_destroy(&fo->pd.lflow_cache_map);
> - lflow_cache_init(&fo->pd.lflow_cache_map);
> - }
> - fo->pd.lflow_cache_enabled = ctrl_ctx->enable_lflow_cache;
>
> - if (!fo->pd.lflow_cache_enabled) {
> + fo->pd.lflow_cache = ctrl_ctx->lflow_cache;
> +
> + if (!lflow_cache_is_enabled(fo->pd.lflow_cache)) {
> fo->pd.conj_id_ofs = 1;
> }
>
> @@ -2006,8 +2000,7 @@ en_flow_output_run(struct engine_node *node, void *data)
> ovn_extend_table_clear(meter_table, false /* desired */);
> lflow_resource_clear(lfrr);
> fo->pd.conj_id_ofs = 1;
> - lflow_cache_destroy(&fo->pd.lflow_cache_map);
> - lflow_cache_init(&fo->pd.lflow_cache_map);
> + lflow_cache_flush(fo->pd.lflow_cache);
> l_ctx_out.conj_id_overflow = false;
> lflow_run(&l_ctx_in, &l_ctx_out);
> if (l_ctx_out.conj_id_overflow) {
> @@ -2692,7 +2685,7 @@ main(int argc, char *argv[])
> unsigned int ovnsb_expected_cond_seqno = UINT_MAX;
>
> struct controller_engine_ctx ctrl_engine_ctx = {
> - .enable_lflow_cache = true
> + .lflow_cache = lflow_cache_create(),
> };
>
> char *ovn_version = ovn_get_internal_version();
> @@ -2736,8 +2729,7 @@ main(int argc, char *argv[])
>
> update_sb_db(ovs_idl_loop.idl, ovnsb_idl_loop.idl, &sb_monitor_all,
> &reset_ovnsb_idl_min_index,
> - &ctrl_engine_ctx.enable_lflow_cache,
> - &ovnsb_expected_cond_seqno);
> + &ctrl_engine_ctx, &ovnsb_expected_cond_seqno);
> update_ssl_config(ovsrec_ssl_table_get(ovs_idl_loop.idl));
> ofctrl_set_probe_interval(get_ofctrl_probe_interval(ovs_idl_loop.idl));
>
> @@ -2778,9 +2770,9 @@ main(int argc, char *argv[])
> /* Unconditionally remove all deleted lflows from the lflow
> * cache.
> */
> - if (flow_output_data && flow_output_data->pd.lflow_cache_enabled) {
> + if (lflow_cache_is_enabled(ctrl_engine_ctx.lflow_cache)) {
> lflow_handle_cached_flows(
> - &flow_output_data->pd.lflow_cache_map,
> + ctrl_engine_ctx.lflow_cache,
> sbrec_logical_flow_table_get(ovnsb_idl_loop.idl));
> }
>
> @@ -3270,8 +3262,7 @@ lflow_cache_flush_cmd(struct unixctl_conn *conn OVS_UNUSED,
> {
> VLOG_INFO("User triggered lflow cache flush.");
> struct flow_output_persistent_data *fo_pd = arg_;
> - lflow_cache_destroy(&fo_pd->lflow_cache_map);
> - lflow_cache_init(&fo_pd->lflow_cache_map);
> + lflow_cache_flush(fo_pd->lflow_cache);
> fo_pd->conj_id_ofs = 1;
> engine_set_force_recompute(true);
> poll_immediate_wake();
> diff --git a/lib/expr.c b/lib/expr.c
> index 796e88a..356e6fe 100644
> --- a/lib/expr.c
> +++ b/lib/expr.c
> @@ -3151,6 +3151,10 @@ expr_matches_destroy(struct hmap *matches)
> {
> struct expr_match *m;
>
> + if (!matches) {
> + return;
> + }
> +
> HMAP_FOR_EACH_POP (m, hmap_node, matches) {
> free(m->conjunctions);
> free(m);
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list