[ovs-dev,4/5] controller: Add MAC cache I-P node

The node currently keeps track of MAC bindings
that have aging enabled. The purpose of this node
is to have structures that can be used by the
thread to update the timestamp when the MAC
binding is used. The I-P is generic enough
to be used by FDB timestamp refresh in very similar
way to the MAC binding.

This is a preparation for the MAC binding refresh mechanism.

Signed-off-by: Ales Musil <amusil@redhat.com>
---
 controller/automake.mk      |   4 +-
 controller/mac_cache.c      | 254 ++++++++++++++++++++++++++++++++++++
 controller/mac_cache.h      |  74 +++++++++++
 controller/ovn-controller.c | 220 +++++++++++++++++++++++++++++++
 4 files changed, 551 insertions(+), 1 deletion(-)
 create mode 100644 controller/mac_cache.c
 create mode 100644 controller/mac_cache.h

Hi Ales,

I have a couple of comments in-line.

On 7/10/23 07:05, Ales Musil wrote:
> The node currently keeps track of MAC bindings
> that have aging enabled. The purpose of this node
> is to have structures that can be used by the
> thread to update the timestamp when the MAC
> binding is used. The I-P is generic enough
> to be used by FDB timestamp refresh in very similar
> way to the MAC binding.
> 
> This is a preparation for the MAC binding refresh mechanism.
> 
> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---
>   controller/automake.mk      |   4 +-
>   controller/mac_cache.c      | 254 ++++++++++++++++++++++++++++++++++++
>   controller/mac_cache.h      |  74 +++++++++++
>   controller/ovn-controller.c | 220 +++++++++++++++++++++++++++++++
>   4 files changed, 551 insertions(+), 1 deletion(-)
>   create mode 100644 controller/mac_cache.c
>   create mode 100644 controller/mac_cache.h
> 
> diff --git a/controller/automake.mk b/controller/automake.mk
> index 334672b4d..562290359 100644
> --- a/controller/automake.mk
> +++ b/controller/automake.mk
> @@ -43,7 +43,9 @@ controller_ovn_controller_SOURCES = \
>   	controller/vif-plug.h \
>   	controller/vif-plug.c \
>   	controller/mirror.h \
> -	controller/mirror.c
> +	controller/mirror.c \
> +	controller/mac_cache.h \
> +	controller/mac_cache.c
>   
>   controller_ovn_controller_LDADD = lib/libovn.la $(OVS_LIBDIR)/libopenvswitch.la
>   man_MANS += controller/ovn-controller.8
> diff --git a/controller/mac_cache.c b/controller/mac_cache.c
> new file mode 100644
> index 000000000..4663499a1
> --- /dev/null
> +++ b/controller/mac_cache.c
> @@ -0,0 +1,254 @@
> +/* Copyright (c) 2023, 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 <stdbool.h>
> +
> +#include "lport.h"
> +#include "mac_cache.h"
> +#include "openvswitch/hmap.h"
> +#include "openvswitch/vlog.h"
> +#include "ovn/logical-fields.h"
> +#include "ovn-sb-idl.h"
> +
> +VLOG_DEFINE_THIS_MODULE(mac_cache);
> +
> +static uint32_t
> +mac_cache_mb_data_hash(const struct mac_cache_mb_data *mb_data);
> +static inline bool
> +mac_cache_mb_data_equals(const struct mac_cache_mb_data *a,
> +                          const struct mac_cache_mb_data *b);
> +static struct mac_cache_mac_binding *
> +mac_cache_mac_binding_find_by_mb_data(struct mac_cache_data *data,
> +                                      const struct mac_cache_mb_data *mb_data);
> +static bool
> +mac_cache_mb_data_from_sbrec(struct mac_cache_mb_data *data,
> +                              const struct sbrec_mac_binding *mb,
> +                              struct ovsdb_idl_index *sbrec_pb_by_name);
> +static struct mac_cache_threshold *
> +mac_cache_threshold_find(struct hmap *thresholds, const struct uuid *uuid);
> +static void
> +mac_cache_threshold_remove(struct hmap *thresholds,
> +                           struct mac_cache_threshold *threshold);
> +
> +bool
> +mac_cache_threshold_add(struct mac_cache_data *data,
> +                        const struct sbrec_datapath_binding *dp)
> +{
> +    struct mac_cache_threshold *threshold =
> +            mac_cache_threshold_find(&data->mb_thresholds, &dp->header_.uuid);
> +    if (threshold) {
> +        return true;
> +    }
> +
> +    uint64_t mb_threshold = smap_get_uint(&dp->external_ids,
> +                                          "mac_binding_age_threshold", 0);
> +    if (!mb_threshold) {
> +        return false;
> +    }
> +
> +    threshold = xmalloc(sizeof *threshold);
> +    threshold->uuid = dp->header_.uuid;
> +    threshold->value = mb_threshold * 1000;
> +
> +    hmap_insert(&data->mb_thresholds, &threshold->hmap_node,
> +                uuid_hash(&dp->header_.uuid));
> +
> +    return true;
> +}
> +
> +bool
> +mac_cache_threshold_replace(struct mac_cache_data *data,
> +                            const struct sbrec_datapath_binding *dp)
> +{
> +    struct mac_cache_threshold *threshold =
> +            mac_cache_threshold_find(&data->mb_thresholds, &dp->header_.uuid);
> +    if (threshold) {
> +        mac_cache_threshold_remove(&data->mb_thresholds, threshold);
> +    }
> +
> +    return mac_cache_threshold_add(data, dp);
> +}
> +
> +void
> +mac_cache_thresholds_destroy(struct mac_cache_data *data)
> +{
> +    struct mac_cache_threshold *threshold;
> +    HMAP_FOR_EACH_POP (threshold, hmap_node, &data->mb_thresholds) {
> +        free(threshold);
> +    }
> +}
> +
> +void
> +mac_cache_mac_binding_add(struct mac_cache_data *data,
> +                           const struct sbrec_mac_binding *mb,
> +                           struct ovsdb_idl_index *sbrec_pb_by_name)
> +{
> +    struct mac_cache_mb_data mb_data;
> +    if (!mac_cache_mb_data_from_sbrec(&mb_data, mb, sbrec_pb_by_name)) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +        VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s, "
> +                     "logical_port=%s", mb->ip, mb->mac, mb->logical_port);
> +        return;
> +    }
> +
> +    struct mac_cache_mac_binding *mc_mb =
> +            mac_cache_mac_binding_find_by_mb_data(data, &mb_data);
> +
> +    if (!mc_mb) {
> +        mc_mb = xmalloc(sizeof *mc_mb);
> +        hmap_insert(&data->mac_bindings, &mc_mb->hmap_node,
> +                    mac_cache_mb_data_hash(&mb_data));
> +    }
> +
> +    mc_mb->sbrec_mb = mb;
> +    mc_mb->data = mb_data;
> +}
> +
> +void
> +mac_cache_mac_binding_remove(struct mac_cache_data *data,
> +                             const struct sbrec_mac_binding *mb,
> +                             struct ovsdb_idl_index *sbrec_pb_by_name)
> +{
> +    struct mac_cache_mb_data mb_data;
> +    if (!mac_cache_mb_data_from_sbrec(&mb_data, mb, sbrec_pb_by_name)) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +        VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s, "
> +                     "logical_port=%s", mb->ip, mb->mac, mb->logical_port);
> +        return;
> +    }
> +
> +    struct mac_cache_mac_binding *mc_mb =
> +            mac_cache_mac_binding_find_by_mb_data(data, &mb_data);
> +    if (!mc_mb) {
> +        return;
> +    }
> +
> +    hmap_remove(&data->mac_bindings, &mc_mb->hmap_node);
> +    free(mc_mb);
> +}
> +
> +bool
> +mac_cache_sb_mac_binding_updated(const struct sbrec_mac_binding *mb)
> +{
> +    bool updated = false;
> +    for (size_t i = 0; i < SBREC_MAC_BINDING_N_COLUMNS; i++) {
> +        /* Ignore timestamp update as this does not affect the existing nodes
> +         * at all. */
> +        if (i == SBREC_MAC_BINDING_COL_TIMESTAMP) {
> +            continue;
> +        }
> +        updated |= sbrec_mac_binding_is_updated(mb, i);
> +    }
> +
> +    return updated || sbrec_mac_binding_is_deleted(mb);
> +}
> +
> +void
> +mac_cache_mac_bindings_destroy(struct mac_cache_data *data)
> +{
> +    struct mac_cache_mac_binding *mc_mb;
> +    HMAP_FOR_EACH_POP (mc_mb, hmap_node, &data->mac_bindings) {
> +        free(mc_mb);
> +    }
> +}
> +
> +static uint32_t
> +mac_cache_mb_data_hash(const struct mac_cache_mb_data *mb_data)
> +{
> +    uint32_t hash = 0;
> +
> +    hash = hash_add(hash, mb_data->port_key);
> +    hash = hash_add(hash, mb_data->dp_key);
> +    hash = hash_add_in6_addr(hash, &mb_data->ip);
> +    hash = hash_add64(hash, eth_addr_to_uint64(mb_data->mac));
> +
> +    return hash_finish(hash, 32);
> +}
> +
> +static inline bool
> +mac_cache_mb_data_equals(const struct mac_cache_mb_data *a,
> +                          const struct mac_cache_mb_data *b)
> +{
> +    return a->port_key == b->port_key &&
> +           a->dp_key == b->dp_key &&
> +           ipv6_addr_equals(&a->ip, &b->ip) &&
> +           eth_addr_equals(a->mac, b->mac);
> +}
> +
> +static bool
> +mac_cache_mb_data_from_sbrec(struct mac_cache_mb_data *data,
> +                              const struct sbrec_mac_binding *mb,
> +                              struct ovsdb_idl_index *sbrec_pb_by_name)
> +{
> +    const struct sbrec_port_binding *pb =
> +            lport_lookup_by_name(sbrec_pb_by_name, mb->logical_port);
> +
> +    if (!pb || !pb->datapath) {
> +        return false;
> +    }
> +
> +    if (!ip46_parse(mb->ip, &data->ip)) {
> +        return false;
> +    }
> +
> +    if (!eth_addr_from_string(mb->mac, &data->mac)) {
> +        return false;
> +    }
> +
> +    data->dp_key = mb->datapath->tunnel_key;
> +    data->port_key = pb->tunnel_key;
> +
> +    return true;
> +}
> +
> +static struct mac_cache_mac_binding *
> +mac_cache_mac_binding_find_by_mb_data(struct mac_cache_data *data,
> +                                      const struct mac_cache_mb_data *mb_data)
> +{
> +    uint32_t hash = mac_cache_mb_data_hash(mb_data);
> +
> +    struct mac_cache_mac_binding *mb;
> +    HMAP_FOR_EACH_WITH_HASH (mb, hmap_node, hash, &data->mac_bindings) {
> +        if (mac_cache_mb_data_equals(&mb->data, mb_data)) {
> +            return mb;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +static struct mac_cache_threshold *
> +mac_cache_threshold_find(struct hmap *thresholds, const struct uuid *uuid)
> +{
> +    uint32_t hash = uuid_hash(uuid);
> +
> +    struct mac_cache_threshold *threshold;
> +    HMAP_FOR_EACH_WITH_HASH (threshold, hmap_node, hash, thresholds) {
> +        if (uuid_equals(&threshold->uuid, uuid)) {
> +            return threshold;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +static void
> +mac_cache_threshold_remove(struct hmap *thresholds,
> +                           struct mac_cache_threshold *threshold)
> +{
> +    hmap_remove(thresholds, &threshold->hmap_node);
> +    free(threshold);
> +}
> diff --git a/controller/mac_cache.h b/controller/mac_cache.h
> new file mode 100644
> index 000000000..f1f1772c8
> --- /dev/null
> +++ b/controller/mac_cache.h
> @@ -0,0 +1,74 @@
> +/* Copyright (c) 2023, 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 OVN_MAC_CACHE_H
> +#define OVN_MAC_CACHE_H
> +
> +#include <stdint.h>
> +
> +#include "openvswitch/hmap.h"
> +#include "ovn-sb-idl.h"
> +#include "openvswitch/ofp-flow.h"
> +
> +struct mac_cache_data {
> +    /* 'struct threshold_node' by datapath UUID for MAC bindings. */
> +    struct hmap mb_thresholds;
> +    /* 'struct mac_binding_node' by 'mac_binding_data' that are
> +     * local and have threshold > 0. */
> +    struct hmap mac_bindings;
> +};

The comments above reference structs that do not exist. Did the struct 
names change at some point?

> +
> +struct mac_cache_threshold {
> +    struct hmap_node hmap_node;
> +    /* Datapath UUID. */
> +    struct uuid uuid;
> +    /* Aging threshold in ms. */
> +    uint64_t value;
> +};
> +
> +struct mac_cache_mb_data {
> +    uint32_t port_key;
> +    uint32_t dp_key;
> +    struct in6_addr ip;
> +    struct eth_addr mac;
> +};
> +
> +struct mac_cache_mac_binding {
> +    struct hmap_node hmap_node;
> +    /* Common data to identify MAC binding. */
> +    struct mac_cache_mb_data data;
> +    /* Reference to the SB MAC binding record. */
> +    const struct sbrec_mac_binding *sbrec_mb;

I don't think it's safe to be keeping a reference to a southbound row 
between runs of ovn-controller's main loop. I'm fairly certain these 
pointers become invalid after a loop iteration.

> +};
> +
> +bool mac_cache_threshold_add(struct mac_cache_data *data,
> +                             const struct sbrec_datapath_binding *dp);
> +void mac_cache_thresholds_destroy(struct mac_cache_data *data);
> +bool mac_cache_threshold_replace(struct mac_cache_data *data,
> +                                 const struct sbrec_datapath_binding *dp);
> +void mac_cache_mac_binding_add(struct mac_cache_data *data,
> +                                const struct sbrec_mac_binding *mb,
> +                                struct ovsdb_idl_index *sbrec_pb_by_name);
> +struct mac_cache_mac_binding *
> +mac_cachce_mac_binding_find(struct mac_cache_data *data,
> +                            const struct sbrec_mac_binding *mb,
> +                            struct ovsdb_idl_index *sbrec_pb_by_name);
> +void mac_cache_mac_binding_remove(struct mac_cache_data *data,
> +                                  const struct sbrec_mac_binding *mb,
> +                                  struct ovsdb_idl_index *sbrec_pb_by_name);
> +void mac_cache_mac_bindings_destroy(struct mac_cache_data *data);
> +bool mac_cache_sb_mac_binding_updated(const struct sbrec_mac_binding *mb);
> +
> +#endif /* controller/mac_cache.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 96d01219e..abb18647a 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -83,6 +83,7 @@
>   #include "lib/ovn-l7.h"
>   #include "hmapx.h"
>   #include "mirror.h"
> +#include "mac_cache.h"
>   
>   VLOG_DEFINE_THIS_MODULE(main);
>   
> @@ -3158,6 +3159,205 @@ en_lb_data_cleanup(void *data)
>       uuidset_destroy(&lb_data->new);
>   }
>   
> +static void *
> +en_mac_cache_init(struct engine_node *node OVS_UNUSED,
> +                   struct engine_arg *arg OVS_UNUSED)
> +{
> +    struct mac_cache_data *cache_data = xzalloc(sizeof *cache_data);
> +
> +    hmap_init(&cache_data->mb_thresholds);
> +    hmap_init(&cache_data->mac_bindings);
> +
> +    return cache_data;
> +}
> +
> +static void
> +en_mac_cache_run(struct engine_node *node, void *data)
> +{
> +    struct mac_cache_data *cache_data = data;
> +    struct ed_type_runtime_data *rt_data =
> +            engine_get_input_data("runtime_data", node);
> +    const struct sbrec_mac_binding_table *mb_table =
> +            EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
> +    struct ovsdb_idl_index *sbrec_pb_by_name =
> +            engine_ovsdb_node_get_index(
> +                    engine_get_input("SB_port_binding", node),
> +                    "name");
> +
> +    mac_cache_thresholds_destroy(cache_data);
> +    mac_cache_mac_bindings_destroy(cache_data);
> +
> +    const struct sbrec_mac_binding *sbrec_mb;
> +    SBREC_MAC_BINDING_TABLE_FOR_EACH (sbrec_mb, mb_table) {
> +        if (!get_local_datapath(&rt_data->local_datapaths,
> +                                sbrec_mb->datapath->tunnel_key)) {
> +            continue;
> +        }
> +
> +        if (mac_cache_threshold_add(cache_data, sbrec_mb->datapath)) {
> +            mac_cache_mac_binding_add(cache_data, sbrec_mb, sbrec_pb_by_name);
> +        }
> +    }
> +
> +    engine_set_node_state(node, EN_UPDATED);
> +}
> +
> +static bool
> +mac_cache_sb_mac_binding_handler(struct engine_node *node, void *data)
> +{
> +    struct mac_cache_data *cache_data = data;
> +    struct ed_type_runtime_data *rt_data =
> +            engine_get_input_data("runtime_data", node);
> +    const struct sbrec_mac_binding_table *mb_table =
> +            EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
> +    struct ovsdb_idl_index *sbrec_pb_by_name =
> +            engine_ovsdb_node_get_index(
> +                    engine_get_input("SB_port_binding", node),
> +                    "name");
> +
> +    size_t previous_size = hmap_count(&cache_data->mac_bindings);
> +
> +    const struct sbrec_mac_binding *sbrec_mb;
> +    SBREC_MAC_BINDING_TABLE_FOR_EACH_TRACKED (sbrec_mb, mb_table) {
> +        if (!mac_cache_sb_mac_binding_updated(sbrec_mb)) {
> +            continue;
> +        }
> +
> +        if (!sbrec_mac_binding_is_new(sbrec_mb)) {
> +            mac_cache_mac_binding_remove(cache_data, sbrec_mb,
> +                                         sbrec_pb_by_name);
> +        }
> +
> +        if (sbrec_mac_binding_is_deleted(sbrec_mb) ||
> +            !get_local_datapath(&rt_data->local_datapaths,
> +                                sbrec_mb->datapath->tunnel_key)) {
> +            continue;
> +        }
> +
> +        if (mac_cache_threshold_add(cache_data, sbrec_mb->datapath)) {
> +            mac_cache_mac_binding_add(cache_data, sbrec_mb, sbrec_pb_by_name);
> +        }
> +    }
> +
> +    if (hmap_count(&cache_data->mac_bindings) != previous_size) {
> +        engine_set_node_state(node, EN_UPDATED);
> +    }
> +
> +    return true;
> +}
> +
> +static bool
> +mac_cache_runtime_data_handler(struct engine_node *node, void *data OVS_UNUSED)
> +{
> +    struct mac_cache_data *cache_data = data;
> +    struct ed_type_runtime_data *rt_data =
> +            engine_get_input_data("runtime_data", node);
> +    struct ovsdb_idl_index *sbrec_mb_by_dp =
> +            engine_ovsdb_node_get_index(
> +                    engine_get_input("SB_mac_binding", node),
> +                    "datapath");
> +    struct ovsdb_idl_index *sbrec_pb_by_name =
> +            engine_ovsdb_node_get_index(
> +                    engine_get_input("SB_port_binding", node),
> +                    "name");
> +
> +    /* There are no tracked data. Fall back to full recompute. */
> +    if (!rt_data->tracked) {
> +        return false;
> +    }
> +
> +    size_t previous_size = hmap_count(&cache_data->mac_bindings);
> +
> +    struct tracked_datapath *tdp;
> +    HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) {
> +        if (tdp->tracked_type != TRACKED_RESOURCE_NEW) {
> +            continue;
> +        }
> +
> +        struct sbrec_mac_binding *mb_index_row =
> +                sbrec_mac_binding_index_init_row(sbrec_mb_by_dp);
> +        sbrec_mac_binding_index_set_datapath(mb_index_row, tdp->dp);
> +
> +        const struct sbrec_mac_binding *mb;
> +        SBREC_MAC_BINDING_FOR_EACH_EQUAL (mb, mb_index_row, sbrec_mb_by_dp) {
> +            mac_cache_mac_binding_add(data, mb, sbrec_pb_by_name);
> +        }
> +
> +        sbrec_mac_binding_index_destroy_row(mb_index_row);
> +    }
> +
> +    if (hmap_count(&cache_data->mac_bindings) != previous_size) {
> +        engine_set_node_state(node, EN_UPDATED);
> +    }
> +
> +    return true;
> +}
> +
> +static bool
> +mac_cache_sb_datapath_binding_handler(struct engine_node *node, void *data)
> +{
> +    struct mac_cache_data *cache_data = data;
> +    struct ed_type_runtime_data *rt_data =
> +            engine_get_input_data("runtime_data", node);
> +    const struct sbrec_datapath_binding_table *dp_table =
> +            EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node));
> +    struct ovsdb_idl_index *sbrec_mb_by_dp =
> +            engine_ovsdb_node_get_index(
> +                    engine_get_input("SB_mac_binding", node),
> +                    "datapath");
> +    struct ovsdb_idl_index *sbrec_pb_by_name =
> +            engine_ovsdb_node_get_index(
> +                    engine_get_input("SB_port_binding", node),
> +                    "name");
> +
> +    size_t previous_size = hmap_count(&cache_data->mac_bindings);
> +
> +    const struct sbrec_datapath_binding *sbrec_dp;
> +    SBREC_DATAPATH_BINDING_TABLE_FOR_EACH (sbrec_dp, dp_table) {
> +        if (sbrec_datapath_binding_is_new(sbrec_dp) ||
> +            sbrec_datapath_binding_is_deleted(sbrec_dp) ||
> +            !get_local_datapath(&rt_data->local_datapaths,
> +                                sbrec_dp->tunnel_key)) {
> +            continue;
> +        }
> +
> +        bool has_threshold = mac_cache_threshold_replace(cache_data, sbrec_dp);
> +
> +        struct sbrec_mac_binding *mb_index_row =
> +                sbrec_mac_binding_index_init_row(sbrec_mb_by_dp);
> +        sbrec_mac_binding_index_set_datapath(mb_index_row, sbrec_dp);
> +
> +        const struct sbrec_mac_binding *mb;
> +        SBREC_MAC_BINDING_FOR_EACH_EQUAL (mb, mb_index_row, sbrec_mb_by_dp) {
> +            if (has_threshold) {
> +                mac_cache_mac_binding_add(data, mb, sbrec_pb_by_name);
> +            } else {
> +                mac_cache_mac_binding_remove(data, mb, sbrec_pb_by_name);
> +            }
> +        }
> +
> +        sbrec_mac_binding_index_destroy_row(mb_index_row);
> +    }
> +
> +    if (hmap_count(&cache_data->mac_bindings) != previous_size) {
> +        engine_set_node_state(node, EN_UPDATED);
> +    }
> +
> +    return true;
> +}
> +
> +
> +static void
> +en_mac_cache_cleanup(void *data)
> +{
> +    struct mac_cache_data *cache_data = data;
> +
> +    mac_cache_thresholds_destroy(cache_data);
> +    hmap_destroy(&cache_data->mb_thresholds);
> +    mac_cache_mac_bindings_destroy(cache_data);
> +    hmap_destroy(&cache_data->mac_bindings);
> +}
> +
>   /* Engine node which is used to handle the Non VIF data like
>    *   - OVS patch ports
>    *   - Tunnel ports and the related chassis information.
> @@ -4482,6 +4682,14 @@ controller_output_lflow_output_handler(struct engine_node *node,
>       return true;
>   }
>   
> +static bool
> +controller_output_mac_cache_handler(struct engine_node *node,
> +                                       void *data OVS_UNUSED)
> +{
> +    engine_set_node_state(node, EN_UPDATED);
> +    return true;
> +}
> +
>   struct ovn_controller_exit_args {
>       bool *exiting;
>       bool *restart;
> @@ -4758,6 +4966,7 @@ main(int argc, char *argv[])
>       ENGINE_NODE(dhcp_options, "dhcp_options");
>       ENGINE_NODE(if_status_mgr, "if_status_mgr");
>       ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data, "lb_data");
> +    ENGINE_NODE(mac_cache, "mac_cache");
>   
>   #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR);
>       SB_NODES
> @@ -4935,10 +5144,21 @@ main(int argc, char *argv[])
>       engine_add_input(&en_runtime_data, &en_ovs_interface_shadow,
>                        runtime_data_ovs_interface_shadow_handler);
>   
> +    engine_add_input(&en_mac_cache, &en_runtime_data,
> +                     mac_cache_runtime_data_handler);
> +    engine_add_input(&en_mac_cache, &en_sb_mac_binding,
> +                     mac_cache_sb_mac_binding_handler);
> +    engine_add_input(&en_mac_cache, &en_sb_datapath_binding,
> +                     mac_cache_sb_datapath_binding_handler);
> +    engine_add_input(&en_mac_cache, &en_sb_port_binding,
> +                     engine_noop_handler);
> +
>       engine_add_input(&en_controller_output, &en_lflow_output,
>                        controller_output_lflow_output_handler);
>       engine_add_input(&en_controller_output, &en_pflow_output,
>                        controller_output_pflow_output_handler);
> +    engine_add_input(&en_controller_output, &en_mac_cache,
> +                     controller_output_mac_cache_handler);
>   
>       struct engine_arg engine_arg = {
>           .sb_idl = ovnsb_idl_loop.idl,

On Thu, Jul 20, 2023 at 10:58 PM Mark Michelson <mmichels@redhat.com> wrote:

> Hi Ales,
>
> I have a couple of comments in-line.
>

Hi Mark,

thank you for the review.


>
> On 7/10/23 07:05, Ales Musil wrote:
> > The node currently keeps track of MAC bindings
> > that have aging enabled. The purpose of this node
> > is to have structures that can be used by the
> > thread to update the timestamp when the MAC
> > binding is used. The I-P is generic enough
> > to be used by FDB timestamp refresh in very similar
> > way to the MAC binding.
> >
> > This is a preparation for the MAC binding refresh mechanism.
> >
> > Signed-off-by: Ales Musil <amusil@redhat.com>
> > ---
> >   controller/automake.mk      |   4 +-
> >   controller/mac_cache.c      | 254 ++++++++++++++++++++++++++++++++++++
> >   controller/mac_cache.h      |  74 +++++++++++
> >   controller/ovn-controller.c | 220 +++++++++++++++++++++++++++++++
> >   4 files changed, 551 insertions(+), 1 deletion(-)
> >   create mode 100644 controller/mac_cache.c
> >   create mode 100644 controller/mac_cache.h
> >
> > diff --git a/controller/automake.mk b/controller/automake.mk
> > index 334672b4d..562290359 100644
> > --- a/controller/automake.mk
> > +++ b/controller/automake.mk
> > @@ -43,7 +43,9 @@ controller_ovn_controller_SOURCES = \
> >       controller/vif-plug.h \
> >       controller/vif-plug.c \
> >       controller/mirror.h \
> > -     controller/mirror.c
> > +     controller/mirror.c \
> > +     controller/mac_cache.h \
> > +     controller/mac_cache.c
> >
> >   controller_ovn_controller_LDADD = lib/libovn.la $(OVS_LIBDIR)/
> libopenvswitch.la
> >   man_MANS += controller/ovn-controller.8
> > diff --git a/controller/mac_cache.c b/controller/mac_cache.c
> > new file mode 100644
> > index 000000000..4663499a1
> > --- /dev/null
> > +++ b/controller/mac_cache.c
> > @@ -0,0 +1,254 @@
> > +/* Copyright (c) 2023, 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 <stdbool.h>
> > +
> > +#include "lport.h"
> > +#include "mac_cache.h"
> > +#include "openvswitch/hmap.h"
> > +#include "openvswitch/vlog.h"
> > +#include "ovn/logical-fields.h"
> > +#include "ovn-sb-idl.h"
> > +
> > +VLOG_DEFINE_THIS_MODULE(mac_cache);
> > +
> > +static uint32_t
> > +mac_cache_mb_data_hash(const struct mac_cache_mb_data *mb_data);
> > +static inline bool
> > +mac_cache_mb_data_equals(const struct mac_cache_mb_data *a,
> > +                          const struct mac_cache_mb_data *b);
> > +static struct mac_cache_mac_binding *
> > +mac_cache_mac_binding_find_by_mb_data(struct mac_cache_data *data,
> > +                                      const struct mac_cache_mb_data
> *mb_data);
> > +static bool
> > +mac_cache_mb_data_from_sbrec(struct mac_cache_mb_data *data,
> > +                              const struct sbrec_mac_binding *mb,
> > +                              struct ovsdb_idl_index *sbrec_pb_by_name);
> > +static struct mac_cache_threshold *
> > +mac_cache_threshold_find(struct hmap *thresholds, const struct uuid
> *uuid);
> > +static void
> > +mac_cache_threshold_remove(struct hmap *thresholds,
> > +                           struct mac_cache_threshold *threshold);
> > +
> > +bool
> > +mac_cache_threshold_add(struct mac_cache_data *data,
> > +                        const struct sbrec_datapath_binding *dp)
> > +{
> > +    struct mac_cache_threshold *threshold =
> > +            mac_cache_threshold_find(&data->mb_thresholds,
> &dp->header_.uuid);
> > +    if (threshold) {
> > +        return true;
> > +    }
> > +
> > +    uint64_t mb_threshold = smap_get_uint(&dp->external_ids,
> > +                                          "mac_binding_age_threshold",
> 0);
> > +    if (!mb_threshold) {
> > +        return false;
> > +    }
> > +
> > +    threshold = xmalloc(sizeof *threshold);
> > +    threshold->uuid = dp->header_.uuid;
> > +    threshold->value = mb_threshold * 1000;
> > +
> > +    hmap_insert(&data->mb_thresholds, &threshold->hmap_node,
> > +                uuid_hash(&dp->header_.uuid));
> > +
> > +    return true;
> > +}
> > +
> > +bool
> > +mac_cache_threshold_replace(struct mac_cache_data *data,
> > +                            const struct sbrec_datapath_binding *dp)
> > +{
> > +    struct mac_cache_threshold *threshold =
> > +            mac_cache_threshold_find(&data->mb_thresholds,
> &dp->header_.uuid);
> > +    if (threshold) {
> > +        mac_cache_threshold_remove(&data->mb_thresholds, threshold);
> > +    }
> > +
> > +    return mac_cache_threshold_add(data, dp);
> > +}
> > +
> > +void
> > +mac_cache_thresholds_destroy(struct mac_cache_data *data)
> > +{
> > +    struct mac_cache_threshold *threshold;
> > +    HMAP_FOR_EACH_POP (threshold, hmap_node, &data->mb_thresholds) {
> > +        free(threshold);
> > +    }
> > +}
> > +
> > +void
> > +mac_cache_mac_binding_add(struct mac_cache_data *data,
> > +                           const struct sbrec_mac_binding *mb,
> > +                           struct ovsdb_idl_index *sbrec_pb_by_name)
> > +{
> > +    struct mac_cache_mb_data mb_data;
> > +    if (!mac_cache_mb_data_from_sbrec(&mb_data, mb, sbrec_pb_by_name)) {
> > +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > +        VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s, "
> > +                     "logical_port=%s", mb->ip, mb->mac,
> mb->logical_port);
> > +        return;
> > +    }
> > +
> > +    struct mac_cache_mac_binding *mc_mb =
> > +            mac_cache_mac_binding_find_by_mb_data(data, &mb_data);
> > +
> > +    if (!mc_mb) {
> > +        mc_mb = xmalloc(sizeof *mc_mb);
> > +        hmap_insert(&data->mac_bindings, &mc_mb->hmap_node,
> > +                    mac_cache_mb_data_hash(&mb_data));
> > +    }
> > +
> > +    mc_mb->sbrec_mb = mb;
> > +    mc_mb->data = mb_data;
> > +}
> > +
> > +void
> > +mac_cache_mac_binding_remove(struct mac_cache_data *data,
> > +                             const struct sbrec_mac_binding *mb,
> > +                             struct ovsdb_idl_index *sbrec_pb_by_name)
> > +{
> > +    struct mac_cache_mb_data mb_data;
> > +    if (!mac_cache_mb_data_from_sbrec(&mb_data, mb, sbrec_pb_by_name)) {
> > +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > +        VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s, "
> > +                     "logical_port=%s", mb->ip, mb->mac,
> mb->logical_port);
> > +        return;
> > +    }
> > +
> > +    struct mac_cache_mac_binding *mc_mb =
> > +            mac_cache_mac_binding_find_by_mb_data(data, &mb_data);
> > +    if (!mc_mb) {
> > +        return;
> > +    }
> > +
> > +    hmap_remove(&data->mac_bindings, &mc_mb->hmap_node);
> > +    free(mc_mb);
> > +}
> > +
> > +bool
> > +mac_cache_sb_mac_binding_updated(const struct sbrec_mac_binding *mb)
> > +{
> > +    bool updated = false;
> > +    for (size_t i = 0; i < SBREC_MAC_BINDING_N_COLUMNS; i++) {
> > +        /* Ignore timestamp update as this does not affect the existing
> nodes
> > +         * at all. */
> > +        if (i == SBREC_MAC_BINDING_COL_TIMESTAMP) {
> > +            continue;
> > +        }
> > +        updated |= sbrec_mac_binding_is_updated(mb, i);
> > +    }
> > +
> > +    return updated || sbrec_mac_binding_is_deleted(mb);
> > +}
> > +
> > +void
> > +mac_cache_mac_bindings_destroy(struct mac_cache_data *data)
> > +{
> > +    struct mac_cache_mac_binding *mc_mb;
> > +    HMAP_FOR_EACH_POP (mc_mb, hmap_node, &data->mac_bindings) {
> > +        free(mc_mb);
> > +    }
> > +}
> > +
> > +static uint32_t
> > +mac_cache_mb_data_hash(const struct mac_cache_mb_data *mb_data)
> > +{
> > +    uint32_t hash = 0;
> > +
> > +    hash = hash_add(hash, mb_data->port_key);
> > +    hash = hash_add(hash, mb_data->dp_key);
> > +    hash = hash_add_in6_addr(hash, &mb_data->ip);
> > +    hash = hash_add64(hash, eth_addr_to_uint64(mb_data->mac));
> > +
> > +    return hash_finish(hash, 32);
> > +}
> > +
> > +static inline bool
> > +mac_cache_mb_data_equals(const struct mac_cache_mb_data *a,
> > +                          const struct mac_cache_mb_data *b)
> > +{
> > +    return a->port_key == b->port_key &&
> > +           a->dp_key == b->dp_key &&
> > +           ipv6_addr_equals(&a->ip, &b->ip) &&
> > +           eth_addr_equals(a->mac, b->mac);
> > +}
> > +
> > +static bool
> > +mac_cache_mb_data_from_sbrec(struct mac_cache_mb_data *data,
> > +                              const struct sbrec_mac_binding *mb,
> > +                              struct ovsdb_idl_index *sbrec_pb_by_name)
> > +{
> > +    const struct sbrec_port_binding *pb =
> > +            lport_lookup_by_name(sbrec_pb_by_name, mb->logical_port);
> > +
> > +    if (!pb || !pb->datapath) {
> > +        return false;
> > +    }
> > +
> > +    if (!ip46_parse(mb->ip, &data->ip)) {
> > +        return false;
> > +    }
> > +
> > +    if (!eth_addr_from_string(mb->mac, &data->mac)) {
> > +        return false;
> > +    }
> > +
> > +    data->dp_key = mb->datapath->tunnel_key;
> > +    data->port_key = pb->tunnel_key;
> > +
> > +    return true;
> > +}
> > +
> > +static struct mac_cache_mac_binding *
> > +mac_cache_mac_binding_find_by_mb_data(struct mac_cache_data *data,
> > +                                      const struct mac_cache_mb_data
> *mb_data)
> > +{
> > +    uint32_t hash = mac_cache_mb_data_hash(mb_data);
> > +
> > +    struct mac_cache_mac_binding *mb;
> > +    HMAP_FOR_EACH_WITH_HASH (mb, hmap_node, hash, &data->mac_bindings) {
> > +        if (mac_cache_mb_data_equals(&mb->data, mb_data)) {
> > +            return mb;
> > +        }
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +static struct mac_cache_threshold *
> > +mac_cache_threshold_find(struct hmap *thresholds, const struct uuid
> *uuid)
> > +{
> > +    uint32_t hash = uuid_hash(uuid);
> > +
> > +    struct mac_cache_threshold *threshold;
> > +    HMAP_FOR_EACH_WITH_HASH (threshold, hmap_node, hash, thresholds) {
> > +        if (uuid_equals(&threshold->uuid, uuid)) {
> > +            return threshold;
> > +        }
> > +    }
> > +
> > +    return NULL;
> > +}
> > +
> > +static void
> > +mac_cache_threshold_remove(struct hmap *thresholds,
> > +                           struct mac_cache_threshold *threshold)
> > +{
> > +    hmap_remove(thresholds, &threshold->hmap_node);
> > +    free(threshold);
> > +}
> > diff --git a/controller/mac_cache.h b/controller/mac_cache.h
> > new file mode 100644
> > index 000000000..f1f1772c8
> > --- /dev/null
> > +++ b/controller/mac_cache.h
> > @@ -0,0 +1,74 @@
> > +/* Copyright (c) 2023, 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 OVN_MAC_CACHE_H
> > +#define OVN_MAC_CACHE_H
> > +
> > +#include <stdint.h>
> > +
> > +#include "openvswitch/hmap.h"
> > +#include "ovn-sb-idl.h"
> > +#include "openvswitch/ofp-flow.h"
> > +
> > +struct mac_cache_data {
> > +    /* 'struct threshold_node' by datapath UUID for MAC bindings. */
> > +    struct hmap mb_thresholds;
> > +    /* 'struct mac_binding_node' by 'mac_binding_data' that are
> > +     * local and have threshold > 0. */
> > +    struct hmap mac_bindings;
> > +};
>
> The comments above reference structs that do not exist. Did the struct
> names change at some point?
>

Yeah it did, fixed in v2.


> > +
> > +struct mac_cache_threshold {
> > +    struct hmap_node hmap_node;
> > +    /* Datapath UUID. */
> > +    struct uuid uuid;
> > +    /* Aging threshold in ms. */
> > +    uint64_t value;
> > +};
> > +
> > +struct mac_cache_mb_data {
> > +    uint32_t port_key;
> > +    uint32_t dp_key;
> > +    struct in6_addr ip;
> > +    struct eth_addr mac;
> > +};
> > +
> > +struct mac_cache_mac_binding {
> > +    struct hmap_node hmap_node;
> > +    /* Common data to identify MAC binding. */
> > +    struct mac_cache_mb_data data;
> > +    /* Reference to the SB MAC binding record. */
> > +    const struct sbrec_mac_binding *sbrec_mb;
>
> I don't think it's safe to be keeping a reference to a southbound row
> between runs of ovn-controller's main loop. I'm fairly certain these
> pointers become invalid after a loop iteration.
>
>
I'm got bit confused by this comment at first. But this is a common pattern
in I-P we are storing references to basically anything (e.g runtime data and
local datapaths). As long as we make sure that the reference is removed
 when the row is deleted from DB we should be fine.

Or maybe I misunderstood?


> > +};
> > +
> > +bool mac_cache_threshold_add(struct mac_cache_data *data,
> > +                             const struct sbrec_datapath_binding *dp);
> > +void mac_cache_thresholds_destroy(struct mac_cache_data *data);
> > +bool mac_cache_threshold_replace(struct mac_cache_data *data,
> > +                                 const struct sbrec_datapath_binding
> *dp);
> > +void mac_cache_mac_binding_add(struct mac_cache_data *data,
> > +                                const struct sbrec_mac_binding *mb,
> > +                                struct ovsdb_idl_index
> *sbrec_pb_by_name);
> > +struct mac_cache_mac_binding *
> > +mac_cachce_mac_binding_find(struct mac_cache_data *data,
> > +                            const struct sbrec_mac_binding *mb,
> > +                            struct ovsdb_idl_index *sbrec_pb_by_name);
> > +void mac_cache_mac_binding_remove(struct mac_cache_data *data,
> > +                                  const struct sbrec_mac_binding *mb,
> > +                                  struct ovsdb_idl_index
> *sbrec_pb_by_name);
> > +void mac_cache_mac_bindings_destroy(struct mac_cache_data *data);
> > +bool mac_cache_sb_mac_binding_updated(const struct sbrec_mac_binding
> *mb);
> > +
> > +#endif /* controller/mac_cache.h */
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index 96d01219e..abb18647a 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -83,6 +83,7 @@
> >   #include "lib/ovn-l7.h"
> >   #include "hmapx.h"
> >   #include "mirror.h"
> > +#include "mac_cache.h"
> >
> >   VLOG_DEFINE_THIS_MODULE(main);
> >
> > @@ -3158,6 +3159,205 @@ en_lb_data_cleanup(void *data)
> >       uuidset_destroy(&lb_data->new);
> >   }
> >
> > +static void *
> > +en_mac_cache_init(struct engine_node *node OVS_UNUSED,
> > +                   struct engine_arg *arg OVS_UNUSED)
> > +{
> > +    struct mac_cache_data *cache_data = xzalloc(sizeof *cache_data);
> > +
> > +    hmap_init(&cache_data->mb_thresholds);
> > +    hmap_init(&cache_data->mac_bindings);
> > +
> > +    return cache_data;
> > +}
> > +
> > +static void
> > +en_mac_cache_run(struct engine_node *node, void *data)
> > +{
> > +    struct mac_cache_data *cache_data = data;
> > +    struct ed_type_runtime_data *rt_data =
> > +            engine_get_input_data("runtime_data", node);
> > +    const struct sbrec_mac_binding_table *mb_table =
> > +            EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
> > +    struct ovsdb_idl_index *sbrec_pb_by_name =
> > +            engine_ovsdb_node_get_index(
> > +                    engine_get_input("SB_port_binding", node),
> > +                    "name");
> > +
> > +    mac_cache_thresholds_destroy(cache_data);
> > +    mac_cache_mac_bindings_destroy(cache_data);
> > +
> > +    const struct sbrec_mac_binding *sbrec_mb;
> > +    SBREC_MAC_BINDING_TABLE_FOR_EACH (sbrec_mb, mb_table) {
> > +        if (!get_local_datapath(&rt_data->local_datapaths,
> > +                                sbrec_mb->datapath->tunnel_key)) {
> > +            continue;
> > +        }
> > +
> > +        if (mac_cache_threshold_add(cache_data, sbrec_mb->datapath)) {
> > +            mac_cache_mac_binding_add(cache_data, sbrec_mb,
> sbrec_pb_by_name);
> > +        }
> > +    }
> > +
> > +    engine_set_node_state(node, EN_UPDATED);
> > +}
> > +
> > +static bool
> > +mac_cache_sb_mac_binding_handler(struct engine_node *node, void *data)
> > +{
> > +    struct mac_cache_data *cache_data = data;
> > +    struct ed_type_runtime_data *rt_data =
> > +            engine_get_input_data("runtime_data", node);
> > +    const struct sbrec_mac_binding_table *mb_table =
> > +            EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
> > +    struct ovsdb_idl_index *sbrec_pb_by_name =
> > +            engine_ovsdb_node_get_index(
> > +                    engine_get_input("SB_port_binding", node),
> > +                    "name");
> > +
> > +    size_t previous_size = hmap_count(&cache_data->mac_bindings);
> > +
> > +    const struct sbrec_mac_binding *sbrec_mb;
> > +    SBREC_MAC_BINDING_TABLE_FOR_EACH_TRACKED (sbrec_mb, mb_table) {
> > +        if (!mac_cache_sb_mac_binding_updated(sbrec_mb)) {
> > +            continue;
> > +        }
> > +
> > +        if (!sbrec_mac_binding_is_new(sbrec_mb)) {
> > +            mac_cache_mac_binding_remove(cache_data, sbrec_mb,
> > +                                         sbrec_pb_by_name);
> > +        }
> > +
> > +        if (sbrec_mac_binding_is_deleted(sbrec_mb) ||
> > +            !get_local_datapath(&rt_data->local_datapaths,
> > +                                sbrec_mb->datapath->tunnel_key)) {
> > +            continue;
> > +        }
> > +
> > +        if (mac_cache_threshold_add(cache_data, sbrec_mb->datapath)) {
> > +            mac_cache_mac_binding_add(cache_data, sbrec_mb,
> sbrec_pb_by_name);
> > +        }
> > +    }
> > +
> > +    if (hmap_count(&cache_data->mac_bindings) != previous_size) {
> > +        engine_set_node_state(node, EN_UPDATED);
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +static bool
> > +mac_cache_runtime_data_handler(struct engine_node *node, void *data
> OVS_UNUSED)
> > +{
> > +    struct mac_cache_data *cache_data = data;
> > +    struct ed_type_runtime_data *rt_data =
> > +            engine_get_input_data("runtime_data", node);
> > +    struct ovsdb_idl_index *sbrec_mb_by_dp =
> > +            engine_ovsdb_node_get_index(
> > +                    engine_get_input("SB_mac_binding", node),
> > +                    "datapath");
> > +    struct ovsdb_idl_index *sbrec_pb_by_name =
> > +            engine_ovsdb_node_get_index(
> > +                    engine_get_input("SB_port_binding", node),
> > +                    "name");
> > +
> > +    /* There are no tracked data. Fall back to full recompute. */
> > +    if (!rt_data->tracked) {
> > +        return false;
> > +    }
> > +
> > +    size_t previous_size = hmap_count(&cache_data->mac_bindings);
> > +
> > +    struct tracked_datapath *tdp;
> > +    HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) {
> > +        if (tdp->tracked_type != TRACKED_RESOURCE_NEW) {
> > +            continue;
> > +        }
> > +
> > +        struct sbrec_mac_binding *mb_index_row =
> > +                sbrec_mac_binding_index_init_row(sbrec_mb_by_dp);
> > +        sbrec_mac_binding_index_set_datapath(mb_index_row, tdp->dp);
> > +
> > +        const struct sbrec_mac_binding *mb;
> > +        SBREC_MAC_BINDING_FOR_EACH_EQUAL (mb, mb_index_row,
> sbrec_mb_by_dp) {
> > +            mac_cache_mac_binding_add(data, mb, sbrec_pb_by_name);
> > +        }
> > +
> > +        sbrec_mac_binding_index_destroy_row(mb_index_row);
> > +    }
> > +
> > +    if (hmap_count(&cache_data->mac_bindings) != previous_size) {
> > +        engine_set_node_state(node, EN_UPDATED);
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +static bool
> > +mac_cache_sb_datapath_binding_handler(struct engine_node *node, void
> *data)
> > +{
> > +    struct mac_cache_data *cache_data = data;
> > +    struct ed_type_runtime_data *rt_data =
> > +            engine_get_input_data("runtime_data", node);
> > +    const struct sbrec_datapath_binding_table *dp_table =
> > +            EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node));
> > +    struct ovsdb_idl_index *sbrec_mb_by_dp =
> > +            engine_ovsdb_node_get_index(
> > +                    engine_get_input("SB_mac_binding", node),
> > +                    "datapath");
> > +    struct ovsdb_idl_index *sbrec_pb_by_name =
> > +            engine_ovsdb_node_get_index(
> > +                    engine_get_input("SB_port_binding", node),
> > +                    "name");
> > +
> > +    size_t previous_size = hmap_count(&cache_data->mac_bindings);
> > +
> > +    const struct sbrec_datapath_binding *sbrec_dp;
> > +    SBREC_DATAPATH_BINDING_TABLE_FOR_EACH (sbrec_dp, dp_table) {
> > +        if (sbrec_datapath_binding_is_new(sbrec_dp) ||
> > +            sbrec_datapath_binding_is_deleted(sbrec_dp) ||
> > +            !get_local_datapath(&rt_data->local_datapaths,
> > +                                sbrec_dp->tunnel_key)) {
> > +            continue;
> > +        }
> > +
> > +        bool has_threshold = mac_cache_threshold_replace(cache_data,
> sbrec_dp);
> > +
> > +        struct sbrec_mac_binding *mb_index_row =
> > +                sbrec_mac_binding_index_init_row(sbrec_mb_by_dp);
> > +        sbrec_mac_binding_index_set_datapath(mb_index_row, sbrec_dp);
> > +
> > +        const struct sbrec_mac_binding *mb;
> > +        SBREC_MAC_BINDING_FOR_EACH_EQUAL (mb, mb_index_row,
> sbrec_mb_by_dp) {
> > +            if (has_threshold) {
> > +                mac_cache_mac_binding_add(data, mb, sbrec_pb_by_name);
> > +            } else {
> > +                mac_cache_mac_binding_remove(data, mb,
> sbrec_pb_by_name);
> > +            }
> > +        }
> > +
> > +        sbrec_mac_binding_index_destroy_row(mb_index_row);
> > +    }
> > +
> > +    if (hmap_count(&cache_data->mac_bindings) != previous_size) {
> > +        engine_set_node_state(node, EN_UPDATED);
> > +    }
> > +
> > +    return true;
> > +}
> > +
> > +
> > +static void
> > +en_mac_cache_cleanup(void *data)
> > +{
> > +    struct mac_cache_data *cache_data = data;
> > +
> > +    mac_cache_thresholds_destroy(cache_data);
> > +    hmap_destroy(&cache_data->mb_thresholds);
> > +    mac_cache_mac_bindings_destroy(cache_data);
> > +    hmap_destroy(&cache_data->mac_bindings);
> > +}
> > +
> >   /* Engine node which is used to handle the Non VIF data like
> >    *   - OVS patch ports
> >    *   - Tunnel ports and the related chassis information.
> > @@ -4482,6 +4682,14 @@ controller_output_lflow_output_handler(struct
> engine_node *node,
> >       return true;
> >   }
> >
> > +static bool
> > +controller_output_mac_cache_handler(struct engine_node *node,
> > +                                       void *data OVS_UNUSED)
> > +{
> > +    engine_set_node_state(node, EN_UPDATED);
> > +    return true;
> > +}
> > +
> >   struct ovn_controller_exit_args {
> >       bool *exiting;
> >       bool *restart;
> > @@ -4758,6 +4966,7 @@ main(int argc, char *argv[])
> >       ENGINE_NODE(dhcp_options, "dhcp_options");
> >       ENGINE_NODE(if_status_mgr, "if_status_mgr");
> >       ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data, "lb_data");
> > +    ENGINE_NODE(mac_cache, "mac_cache");
> >
> >   #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR);
> >       SB_NODES
> > @@ -4935,10 +5144,21 @@ main(int argc, char *argv[])
> >       engine_add_input(&en_runtime_data, &en_ovs_interface_shadow,
> >                        runtime_data_ovs_interface_shadow_handler);
> >
> > +    engine_add_input(&en_mac_cache, &en_runtime_data,
> > +                     mac_cache_runtime_data_handler);
> > +    engine_add_input(&en_mac_cache, &en_sb_mac_binding,
> > +                     mac_cache_sb_mac_binding_handler);
> > +    engine_add_input(&en_mac_cache, &en_sb_datapath_binding,
> > +                     mac_cache_sb_datapath_binding_handler);
> > +    engine_add_input(&en_mac_cache, &en_sb_port_binding,
> > +                     engine_noop_handler);
> > +
> >       engine_add_input(&en_controller_output, &en_lflow_output,
> >                        controller_output_lflow_output_handler);
> >       engine_add_input(&en_controller_output, &en_pflow_output,
> >                        controller_output_pflow_output_handler);
> > +    engine_add_input(&en_controller_output, &en_mac_cache,
> > +                     controller_output_mac_cache_handler);
> >
> >       struct engine_arg engine_arg = {
> >           .sb_idl = ovnsb_idl_loop.idl,
>
>
Thanks,
Ales

On 7/25/23 03:59, Ales Musil wrote:
> 
> 
> On Thu, Jul 20, 2023 at 10:58 PM Mark Michelson <mmichels@redhat.com 
> <mailto:mmichels@redhat.com>> wrote:
> 
>     Hi Ales,
> 
>     I have a couple of comments in-line.
> 
> 
> Hi Mark,
> 
> thank you for the review.
> 
> 
>     On 7/10/23 07:05, Ales Musil wrote:
>      > The node currently keeps track of MAC bindings
>      > that have aging enabled. The purpose of this node
>      > is to have structures that can be used by the
>      > thread to update the timestamp when the MAC
>      > binding is used. The I-P is generic enough
>      > to be used by FDB timestamp refresh in very similar
>      > way to the MAC binding.
>      >
>      > This is a preparation for the MAC binding refresh mechanism.
>      >
>      > Signed-off-by: Ales Musil <amusil@redhat.com
>     <mailto:amusil@redhat.com>>
>      > ---
>      >   controller/automake.mk <http://automake.mk>      |   4 +-
>      >   controller/mac_cache.c      | 254
>     ++++++++++++++++++++++++++++++++++++
>      >   controller/mac_cache.h      |  74 +++++++++++
>      >   controller/ovn-controller.c | 220 +++++++++++++++++++++++++++++++
>      >   4 files changed, 551 insertions(+), 1 deletion(-)
>      >   create mode 100644 controller/mac_cache.c
>      >   create mode 100644 controller/mac_cache.h
>      >
>      > diff --git a/controller/automake.mk <http://automake.mk>
>     b/controller/automake.mk <http://automake.mk>
>      > index 334672b4d..562290359 100644
>      > --- a/controller/automake.mk <http://automake.mk>
>      > +++ b/controller/automake.mk <http://automake.mk>
>      > @@ -43,7 +43,9 @@ controller_ovn_controller_SOURCES = \
>      >       controller/vif-plug.h \
>      >       controller/vif-plug.c \
>      >       controller/mirror.h \
>      > -     controller/mirror.c
>      > +     controller/mirror.c \
>      > +     controller/mac_cache.h \
>      > +     controller/mac_cache.c
>      >
>      >   controller_ovn_controller_LDADD = lib/libovn.la
>     <http://libovn.la> $(OVS_LIBDIR)/libopenvswitch.la
>     <http://libopenvswitch.la>
>      >   man_MANS += controller/ovn-controller.8
>      > diff --git a/controller/mac_cache.c b/controller/mac_cache.c
>      > new file mode 100644
>      > index 000000000..4663499a1
>      > --- /dev/null
>      > +++ b/controller/mac_cache.c
>      > @@ -0,0 +1,254 @@
>      > +/* Copyright (c) 2023, 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
>     <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 <stdbool.h>
>      > +
>      > +#include "lport.h"
>      > +#include "mac_cache.h"
>      > +#include "openvswitch/hmap.h"
>      > +#include "openvswitch/vlog.h"
>      > +#include "ovn/logical-fields.h"
>      > +#include "ovn-sb-idl.h"
>      > +
>      > +VLOG_DEFINE_THIS_MODULE(mac_cache);
>      > +
>      > +static uint32_t
>      > +mac_cache_mb_data_hash(const struct mac_cache_mb_data *mb_data);
>      > +static inline bool
>      > +mac_cache_mb_data_equals(const struct mac_cache_mb_data *a,
>      > +                          const struct mac_cache_mb_data *b);
>      > +static struct mac_cache_mac_binding *
>      > +mac_cache_mac_binding_find_by_mb_data(struct mac_cache_data *data,
>      > +                                      const struct
>     mac_cache_mb_data *mb_data);
>      > +static bool
>      > +mac_cache_mb_data_from_sbrec(struct mac_cache_mb_data *data,
>      > +                              const struct sbrec_mac_binding *mb,
>      > +                              struct ovsdb_idl_index
>     *sbrec_pb_by_name);
>      > +static struct mac_cache_threshold *
>      > +mac_cache_threshold_find(struct hmap *thresholds, const struct
>     uuid *uuid);
>      > +static void
>      > +mac_cache_threshold_remove(struct hmap *thresholds,
>      > +                           struct mac_cache_threshold *threshold);
>      > +
>      > +bool
>      > +mac_cache_threshold_add(struct mac_cache_data *data,
>      > +                        const struct sbrec_datapath_binding *dp)
>      > +{
>      > +    struct mac_cache_threshold *threshold =
>      > +            mac_cache_threshold_find(&data->mb_thresholds,
>     &dp->header_.uuid);
>      > +    if (threshold) {
>      > +        return true;
>      > +    }
>      > +
>      > +    uint64_t mb_threshold = smap_get_uint(&dp->external_ids,
>      > +                                         
>     "mac_binding_age_threshold", 0);
>      > +    if (!mb_threshold) {
>      > +        return false;
>      > +    }
>      > +
>      > +    threshold = xmalloc(sizeof *threshold);
>      > +    threshold->uuid = dp->header_.uuid;
>      > +    threshold->value = mb_threshold * 1000;
>      > +
>      > +    hmap_insert(&data->mb_thresholds, &threshold->hmap_node,
>      > +                uuid_hash(&dp->header_.uuid));
>      > +
>      > +    return true;
>      > +}
>      > +
>      > +bool
>      > +mac_cache_threshold_replace(struct mac_cache_data *data,
>      > +                            const struct sbrec_datapath_binding *dp)
>      > +{
>      > +    struct mac_cache_threshold *threshold =
>      > +            mac_cache_threshold_find(&data->mb_thresholds,
>     &dp->header_.uuid);
>      > +    if (threshold) {
>      > +        mac_cache_threshold_remove(&data->mb_thresholds, threshold);
>      > +    }
>      > +
>      > +    return mac_cache_threshold_add(data, dp);
>      > +}
>      > +
>      > +void
>      > +mac_cache_thresholds_destroy(struct mac_cache_data *data)
>      > +{
>      > +    struct mac_cache_threshold *threshold;
>      > +    HMAP_FOR_EACH_POP (threshold, hmap_node, &data->mb_thresholds) {
>      > +        free(threshold);
>      > +    }
>      > +}
>      > +
>      > +void
>      > +mac_cache_mac_binding_add(struct mac_cache_data *data,
>      > +                           const struct sbrec_mac_binding *mb,
>      > +                           struct ovsdb_idl_index *sbrec_pb_by_name)
>      > +{
>      > +    struct mac_cache_mb_data mb_data;
>      > +    if (!mac_cache_mb_data_from_sbrec(&mb_data, mb,
>     sbrec_pb_by_name)) {
>      > +        static struct vlog_rate_limit rl =
>     VLOG_RATE_LIMIT_INIT(5, 1);
>      > +        VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s,
>     mac=%s, "
>      > +                     "logical_port=%s", mb->ip, mb->mac,
>     mb->logical_port);
>      > +        return;
>      > +    }
>      > +
>      > +    struct mac_cache_mac_binding *mc_mb =
>      > +            mac_cache_mac_binding_find_by_mb_data(data, &mb_data);
>      > +
>      > +    if (!mc_mb) {
>      > +        mc_mb = xmalloc(sizeof *mc_mb);
>      > +        hmap_insert(&data->mac_bindings, &mc_mb->hmap_node,
>      > +                    mac_cache_mb_data_hash(&mb_data));
>      > +    }
>      > +
>      > +    mc_mb->sbrec_mb = mb;
>      > +    mc_mb->data = mb_data;
>      > +}
>      > +
>      > +void
>      > +mac_cache_mac_binding_remove(struct mac_cache_data *data,
>      > +                             const struct sbrec_mac_binding *mb,
>      > +                             struct ovsdb_idl_index
>     *sbrec_pb_by_name)
>      > +{
>      > +    struct mac_cache_mb_data mb_data;
>      > +    if (!mac_cache_mb_data_from_sbrec(&mb_data, mb,
>     sbrec_pb_by_name)) {
>      > +        static struct vlog_rate_limit rl =
>     VLOG_RATE_LIMIT_INIT(5, 1);
>      > +        VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s,
>     mac=%s, "
>      > +                     "logical_port=%s", mb->ip, mb->mac,
>     mb->logical_port);
>      > +        return;
>      > +    }
>      > +
>      > +    struct mac_cache_mac_binding *mc_mb =
>      > +            mac_cache_mac_binding_find_by_mb_data(data, &mb_data);
>      > +    if (!mc_mb) {
>      > +        return;
>      > +    }
>      > +
>      > +    hmap_remove(&data->mac_bindings, &mc_mb->hmap_node);
>      > +    free(mc_mb);
>      > +}
>      > +
>      > +bool
>      > +mac_cache_sb_mac_binding_updated(const struct sbrec_mac_binding *mb)
>      > +{
>      > +    bool updated = false;
>      > +    for (size_t i = 0; i < SBREC_MAC_BINDING_N_COLUMNS; i++) {
>      > +        /* Ignore timestamp update as this does not affect the
>     existing nodes
>      > +         * at all. */
>      > +        if (i == SBREC_MAC_BINDING_COL_TIMESTAMP) {
>      > +            continue;
>      > +        }
>      > +        updated |= sbrec_mac_binding_is_updated(mb, i);
>      > +    }
>      > +
>      > +    return updated || sbrec_mac_binding_is_deleted(mb);
>      > +}
>      > +
>      > +void
>      > +mac_cache_mac_bindings_destroy(struct mac_cache_data *data)
>      > +{
>      > +    struct mac_cache_mac_binding *mc_mb;
>      > +    HMAP_FOR_EACH_POP (mc_mb, hmap_node, &data->mac_bindings) {
>      > +        free(mc_mb);
>      > +    }
>      > +}
>      > +
>      > +static uint32_t
>      > +mac_cache_mb_data_hash(const struct mac_cache_mb_data *mb_data)
>      > +{
>      > +    uint32_t hash = 0;
>      > +
>      > +    hash = hash_add(hash, mb_data->port_key);
>      > +    hash = hash_add(hash, mb_data->dp_key);
>      > +    hash = hash_add_in6_addr(hash, &mb_data->ip);
>      > +    hash = hash_add64(hash, eth_addr_to_uint64(mb_data->mac));
>      > +
>      > +    return hash_finish(hash, 32);
>      > +}
>      > +
>      > +static inline bool
>      > +mac_cache_mb_data_equals(const struct mac_cache_mb_data *a,
>      > +                          const struct mac_cache_mb_data *b)
>      > +{
>      > +    return a->port_key == b->port_key &&
>      > +           a->dp_key == b->dp_key &&
>      > +           ipv6_addr_equals(&a->ip, &b->ip) &&
>      > +           eth_addr_equals(a->mac, b->mac);
>      > +}
>      > +
>      > +static bool
>      > +mac_cache_mb_data_from_sbrec(struct mac_cache_mb_data *data,
>      > +                              const struct sbrec_mac_binding *mb,
>      > +                              struct ovsdb_idl_index
>     *sbrec_pb_by_name)
>      > +{
>      > +    const struct sbrec_port_binding *pb =
>      > +            lport_lookup_by_name(sbrec_pb_by_name,
>     mb->logical_port);
>      > +
>      > +    if (!pb || !pb->datapath) {
>      > +        return false;
>      > +    }
>      > +
>      > +    if (!ip46_parse(mb->ip, &data->ip)) {
>      > +        return false;
>      > +    }
>      > +
>      > +    if (!eth_addr_from_string(mb->mac, &data->mac)) {
>      > +        return false;
>      > +    }
>      > +
>      > +    data->dp_key = mb->datapath->tunnel_key;
>      > +    data->port_key = pb->tunnel_key;
>      > +
>      > +    return true;
>      > +}
>      > +
>      > +static struct mac_cache_mac_binding *
>      > +mac_cache_mac_binding_find_by_mb_data(struct mac_cache_data *data,
>      > +                                      const struct
>     mac_cache_mb_data *mb_data)
>      > +{
>      > +    uint32_t hash = mac_cache_mb_data_hash(mb_data);
>      > +
>      > +    struct mac_cache_mac_binding *mb;
>      > +    HMAP_FOR_EACH_WITH_HASH (mb, hmap_node, hash,
>     &data->mac_bindings) {
>      > +        if (mac_cache_mb_data_equals(&mb->data, mb_data)) {
>      > +            return mb;
>      > +        }
>      > +    }
>      > +
>      > +    return NULL;
>      > +}
>      > +
>      > +static struct mac_cache_threshold *
>      > +mac_cache_threshold_find(struct hmap *thresholds, const struct
>     uuid *uuid)
>      > +{
>      > +    uint32_t hash = uuid_hash(uuid);
>      > +
>      > +    struct mac_cache_threshold *threshold;
>      > +    HMAP_FOR_EACH_WITH_HASH (threshold, hmap_node, hash,
>     thresholds) {
>      > +        if (uuid_equals(&threshold->uuid, uuid)) {
>      > +            return threshold;
>      > +        }
>      > +    }
>      > +
>      > +    return NULL;
>      > +}
>      > +
>      > +static void
>      > +mac_cache_threshold_remove(struct hmap *thresholds,
>      > +                           struct mac_cache_threshold *threshold)
>      > +{
>      > +    hmap_remove(thresholds, &threshold->hmap_node);
>      > +    free(threshold);
>      > +}
>      > diff --git a/controller/mac_cache.h b/controller/mac_cache.h
>      > new file mode 100644
>      > index 000000000..f1f1772c8
>      > --- /dev/null
>      > +++ b/controller/mac_cache.h
>      > @@ -0,0 +1,74 @@
>      > +/* Copyright (c) 2023, 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
>     <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 OVN_MAC_CACHE_H
>      > +#define OVN_MAC_CACHE_H
>      > +
>      > +#include <stdint.h>
>      > +
>      > +#include "openvswitch/hmap.h"
>      > +#include "ovn-sb-idl.h"
>      > +#include "openvswitch/ofp-flow.h"
>      > +
>      > +struct mac_cache_data {
>      > +    /* 'struct threshold_node' by datapath UUID for MAC bindings. */
>      > +    struct hmap mb_thresholds;
>      > +    /* 'struct mac_binding_node' by 'mac_binding_data' that are
>      > +     * local and have threshold > 0. */
>      > +    struct hmap mac_bindings;
>      > +};
> 
>     The comments above reference structs that do not exist. Did the struct
>     names change at some point?
> 
> 
> Yeah it did, fixed in v2.
> 
> 
>      > +
>      > +struct mac_cache_threshold {
>      > +    struct hmap_node hmap_node;
>      > +    /* Datapath UUID. */
>      > +    struct uuid uuid;
>      > +    /* Aging threshold in ms. */
>      > +    uint64_t value;
>      > +};
>      > +
>      > +struct mac_cache_mb_data {
>      > +    uint32_t port_key;
>      > +    uint32_t dp_key;
>      > +    struct in6_addr ip;
>      > +    struct eth_addr mac;
>      > +};
>      > +
>      > +struct mac_cache_mac_binding {
>      > +    struct hmap_node hmap_node;
>      > +    /* Common data to identify MAC binding. */
>      > +    struct mac_cache_mb_data data;
>      > +    /* Reference to the SB MAC binding record. */
>      > +    const struct sbrec_mac_binding *sbrec_mb;
> 
>     I don't think it's safe to be keeping a reference to a southbound row
>     between runs of ovn-controller's main loop. I'm fairly certain these
>     pointers become invalid after a loop iteration.
> 
> 
> I'm got bit confused by this comment at first. But this is a common pattern
> in I-P we are storing references to basically anything (e.g runtime data and
> local datapaths). As long as we make sure that the reference is removed
>   when the row is deleted from DB we should be fine.
> 
> Or maybe I misunderstood?

Sorry Ales, I was mistaken. For some reason when making the review, I 
thought that sbrec_* pointers could go out of scope after an execution 
of the ovn-controller main loop. However, you're correct that their 
lifetime is tied to the data in the row instead. You can ignore my 
comment here.

> 
> 
>      > +};
>      > +
>      > +bool mac_cache_threshold_add(struct mac_cache_data *data,
>      > +                             const struct sbrec_datapath_binding
>     *dp);
>      > +void mac_cache_thresholds_destroy(struct mac_cache_data *data);
>      > +bool mac_cache_threshold_replace(struct mac_cache_data *data,
>      > +                                 const struct
>     sbrec_datapath_binding *dp);
>      > +void mac_cache_mac_binding_add(struct mac_cache_data *data,
>      > +                                const struct sbrec_mac_binding *mb,
>      > +                                struct ovsdb_idl_index
>     *sbrec_pb_by_name);
>      > +struct mac_cache_mac_binding *
>      > +mac_cachce_mac_binding_find(struct mac_cache_data *data,
>      > +                            const struct sbrec_mac_binding *mb,
>      > +                            struct ovsdb_idl_index
>     *sbrec_pb_by_name);
>      > +void mac_cache_mac_binding_remove(struct mac_cache_data *data,
>      > +                                  const struct sbrec_mac_binding
>     *mb,
>      > +                                  struct ovsdb_idl_index
>     *sbrec_pb_by_name);
>      > +void mac_cache_mac_bindings_destroy(struct mac_cache_data *data);
>      > +bool mac_cache_sb_mac_binding_updated(const struct
>     sbrec_mac_binding *mb);
>      > +
>      > +#endif /* controller/mac_cache.h */
>      > diff --git a/controller/ovn-controller.c
>     b/controller/ovn-controller.c
>      > index 96d01219e..abb18647a 100644
>      > --- a/controller/ovn-controller.c
>      > +++ b/controller/ovn-controller.c
>      > @@ -83,6 +83,7 @@
>      >   #include "lib/ovn-l7.h"
>      >   #include "hmapx.h"
>      >   #include "mirror.h"
>      > +#include "mac_cache.h"
>      >
>      >   VLOG_DEFINE_THIS_MODULE(main);
>      >
>      > @@ -3158,6 +3159,205 @@ en_lb_data_cleanup(void *data)
>      >       uuidset_destroy(&lb_data->new);
>      >   }
>      >
>      > +static void *
>      > +en_mac_cache_init(struct engine_node *node OVS_UNUSED,
>      > +                   struct engine_arg *arg OVS_UNUSED)
>      > +{
>      > +    struct mac_cache_data *cache_data = xzalloc(sizeof *cache_data);
>      > +
>      > +    hmap_init(&cache_data->mb_thresholds);
>      > +    hmap_init(&cache_data->mac_bindings);
>      > +
>      > +    return cache_data;
>      > +}
>      > +
>      > +static void
>      > +en_mac_cache_run(struct engine_node *node, void *data)
>      > +{
>      > +    struct mac_cache_data *cache_data = data;
>      > +    struct ed_type_runtime_data *rt_data =
>      > +            engine_get_input_data("runtime_data", node);
>      > +    const struct sbrec_mac_binding_table *mb_table =
>      > +            EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
>      > +    struct ovsdb_idl_index *sbrec_pb_by_name =
>      > +            engine_ovsdb_node_get_index(
>      > +                    engine_get_input("SB_port_binding", node),
>      > +                    "name");
>      > +
>      > +    mac_cache_thresholds_destroy(cache_data);
>      > +    mac_cache_mac_bindings_destroy(cache_data);
>      > +
>      > +    const struct sbrec_mac_binding *sbrec_mb;
>      > +    SBREC_MAC_BINDING_TABLE_FOR_EACH (sbrec_mb, mb_table) {
>      > +        if (!get_local_datapath(&rt_data->local_datapaths,
>      > +                                sbrec_mb->datapath->tunnel_key)) {
>      > +            continue;
>      > +        }
>      > +
>      > +        if (mac_cache_threshold_add(cache_data,
>     sbrec_mb->datapath)) {
>      > +            mac_cache_mac_binding_add(cache_data, sbrec_mb,
>     sbrec_pb_by_name);
>      > +        }
>      > +    }
>      > +
>      > +    engine_set_node_state(node, EN_UPDATED);
>      > +}
>      > +
>      > +static bool
>      > +mac_cache_sb_mac_binding_handler(struct engine_node *node, void
>     *data)
>      > +{
>      > +    struct mac_cache_data *cache_data = data;
>      > +    struct ed_type_runtime_data *rt_data =
>      > +            engine_get_input_data("runtime_data", node);
>      > +    const struct sbrec_mac_binding_table *mb_table =
>      > +            EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
>      > +    struct ovsdb_idl_index *sbrec_pb_by_name =
>      > +            engine_ovsdb_node_get_index(
>      > +                    engine_get_input("SB_port_binding", node),
>      > +                    "name");
>      > +
>      > +    size_t previous_size = hmap_count(&cache_data->mac_bindings);
>      > +
>      > +    const struct sbrec_mac_binding *sbrec_mb;
>      > +    SBREC_MAC_BINDING_TABLE_FOR_EACH_TRACKED (sbrec_mb, mb_table) {
>      > +        if (!mac_cache_sb_mac_binding_updated(sbrec_mb)) {
>      > +            continue;
>      > +        }
>      > +
>      > +        if (!sbrec_mac_binding_is_new(sbrec_mb)) {
>      > +            mac_cache_mac_binding_remove(cache_data, sbrec_mb,
>      > +                                         sbrec_pb_by_name);
>      > +        }
>      > +
>      > +        if (sbrec_mac_binding_is_deleted(sbrec_mb) ||
>      > +            !get_local_datapath(&rt_data->local_datapaths,
>      > +                                sbrec_mb->datapath->tunnel_key)) {
>      > +            continue;
>      > +        }
>      > +
>      > +        if (mac_cache_threshold_add(cache_data,
>     sbrec_mb->datapath)) {
>      > +            mac_cache_mac_binding_add(cache_data, sbrec_mb,
>     sbrec_pb_by_name);
>      > +        }
>      > +    }
>      > +
>      > +    if (hmap_count(&cache_data->mac_bindings) != previous_size) {
>      > +        engine_set_node_state(node, EN_UPDATED);
>      > +    }
>      > +
>      > +    return true;
>      > +}
>      > +
>      > +static bool
>      > +mac_cache_runtime_data_handler(struct engine_node *node, void
>     *data OVS_UNUSED)
>      > +{
>      > +    struct mac_cache_data *cache_data = data;
>      > +    struct ed_type_runtime_data *rt_data =
>      > +            engine_get_input_data("runtime_data", node);
>      > +    struct ovsdb_idl_index *sbrec_mb_by_dp =
>      > +            engine_ovsdb_node_get_index(
>      > +                    engine_get_input("SB_mac_binding", node),
>      > +                    "datapath");
>      > +    struct ovsdb_idl_index *sbrec_pb_by_name =
>      > +            engine_ovsdb_node_get_index(
>      > +                    engine_get_input("SB_port_binding", node),
>      > +                    "name");
>      > +
>      > +    /* There are no tracked data. Fall back to full recompute. */
>      > +    if (!rt_data->tracked) {
>      > +        return false;
>      > +    }
>      > +
>      > +    size_t previous_size = hmap_count(&cache_data->mac_bindings);
>      > +
>      > +    struct tracked_datapath *tdp;
>      > +    HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) {
>      > +        if (tdp->tracked_type != TRACKED_RESOURCE_NEW) {
>      > +            continue;
>      > +        }
>      > +
>      > +        struct sbrec_mac_binding *mb_index_row =
>      > +                sbrec_mac_binding_index_init_row(sbrec_mb_by_dp);
>      > +        sbrec_mac_binding_index_set_datapath(mb_index_row, tdp->dp);
>      > +
>      > +        const struct sbrec_mac_binding *mb;
>      > +        SBREC_MAC_BINDING_FOR_EACH_EQUAL (mb, mb_index_row,
>     sbrec_mb_by_dp) {
>      > +            mac_cache_mac_binding_add(data, mb, sbrec_pb_by_name);
>      > +        }
>      > +
>      > +        sbrec_mac_binding_index_destroy_row(mb_index_row);
>      > +    }
>      > +
>      > +    if (hmap_count(&cache_data->mac_bindings) != previous_size) {
>      > +        engine_set_node_state(node, EN_UPDATED);
>      > +    }
>      > +
>      > +    return true;
>      > +}
>      > +
>      > +static bool
>      > +mac_cache_sb_datapath_binding_handler(struct engine_node *node,
>     void *data)
>      > +{
>      > +    struct mac_cache_data *cache_data = data;
>      > +    struct ed_type_runtime_data *rt_data =
>      > +            engine_get_input_data("runtime_data", node);
>      > +    const struct sbrec_datapath_binding_table *dp_table =
>      > +            EN_OVSDB_GET(engine_get_input("SB_datapath_binding",
>     node));
>      > +    struct ovsdb_idl_index *sbrec_mb_by_dp =
>      > +            engine_ovsdb_node_get_index(
>      > +                    engine_get_input("SB_mac_binding", node),
>      > +                    "datapath");
>      > +    struct ovsdb_idl_index *sbrec_pb_by_name =
>      > +            engine_ovsdb_node_get_index(
>      > +                    engine_get_input("SB_port_binding", node),
>      > +                    "name");
>      > +
>      > +    size_t previous_size = hmap_count(&cache_data->mac_bindings);
>      > +
>      > +    const struct sbrec_datapath_binding *sbrec_dp;
>      > +    SBREC_DATAPATH_BINDING_TABLE_FOR_EACH (sbrec_dp, dp_table) {
>      > +        if (sbrec_datapath_binding_is_new(sbrec_dp) ||
>      > +            sbrec_datapath_binding_is_deleted(sbrec_dp) ||
>      > +            !get_local_datapath(&rt_data->local_datapaths,
>      > +                                sbrec_dp->tunnel_key)) {
>      > +            continue;
>      > +        }
>      > +
>      > +        bool has_threshold =
>     mac_cache_threshold_replace(cache_data, sbrec_dp);
>      > +
>      > +        struct sbrec_mac_binding *mb_index_row =
>      > +                sbrec_mac_binding_index_init_row(sbrec_mb_by_dp);
>      > +        sbrec_mac_binding_index_set_datapath(mb_index_row,
>     sbrec_dp);
>      > +
>      > +        const struct sbrec_mac_binding *mb;
>      > +        SBREC_MAC_BINDING_FOR_EACH_EQUAL (mb, mb_index_row,
>     sbrec_mb_by_dp) {
>      > +            if (has_threshold) {
>      > +                mac_cache_mac_binding_add(data, mb,
>     sbrec_pb_by_name);
>      > +            } else {
>      > +                mac_cache_mac_binding_remove(data, mb,
>     sbrec_pb_by_name);
>      > +            }
>      > +        }
>      > +
>      > +        sbrec_mac_binding_index_destroy_row(mb_index_row);
>      > +    }
>      > +
>      > +    if (hmap_count(&cache_data->mac_bindings) != previous_size) {
>      > +        engine_set_node_state(node, EN_UPDATED);
>      > +    }
>      > +
>      > +    return true;
>      > +}
>      > +
>      > +
>      > +static void
>      > +en_mac_cache_cleanup(void *data)
>      > +{
>      > +    struct mac_cache_data *cache_data = data;
>      > +
>      > +    mac_cache_thresholds_destroy(cache_data);
>      > +    hmap_destroy(&cache_data->mb_thresholds);
>      > +    mac_cache_mac_bindings_destroy(cache_data);
>      > +    hmap_destroy(&cache_data->mac_bindings);
>      > +}
>      > +
>      >   /* Engine node which is used to handle the Non VIF data like
>      >    *   - OVS patch ports
>      >    *   - Tunnel ports and the related chassis information.
>      > @@ -4482,6 +4682,14 @@
>     controller_output_lflow_output_handler(struct engine_node *node,
>      >       return true;
>      >   }
>      >
>      > +static bool
>      > +controller_output_mac_cache_handler(struct engine_node *node,
>      > +                                       void *data OVS_UNUSED)
>      > +{
>      > +    engine_set_node_state(node, EN_UPDATED);
>      > +    return true;
>      > +}
>      > +
>      >   struct ovn_controller_exit_args {
>      >       bool *exiting;
>      >       bool *restart;
>      > @@ -4758,6 +4966,7 @@ main(int argc, char *argv[])
>      >       ENGINE_NODE(dhcp_options, "dhcp_options");
>      >       ENGINE_NODE(if_status_mgr, "if_status_mgr");
>      >       ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data, "lb_data");
>      > +    ENGINE_NODE(mac_cache, "mac_cache");
>      >
>      >   #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR);
>      >       SB_NODES
>      > @@ -4935,10 +5144,21 @@ main(int argc, char *argv[])
>      >       engine_add_input(&en_runtime_data, &en_ovs_interface_shadow,
>      >                        runtime_data_ovs_interface_shadow_handler);
>      >
>      > +    engine_add_input(&en_mac_cache, &en_runtime_data,
>      > +                     mac_cache_runtime_data_handler);
>      > +    engine_add_input(&en_mac_cache, &en_sb_mac_binding,
>      > +                     mac_cache_sb_mac_binding_handler);
>      > +    engine_add_input(&en_mac_cache, &en_sb_datapath_binding,
>      > +                     mac_cache_sb_datapath_binding_handler);
>      > +    engine_add_input(&en_mac_cache, &en_sb_port_binding,
>      > +                     engine_noop_handler);
>      > +
>      >       engine_add_input(&en_controller_output, &en_lflow_output,
>      >                        controller_output_lflow_output_handler);
>      >       engine_add_input(&en_controller_output, &en_pflow_output,
>      >                        controller_output_pflow_output_handler);
>      > +    engine_add_input(&en_controller_output, &en_mac_cache,
>      > +                     controller_output_mac_cache_handler);
>      >
>      >       struct engine_arg engine_arg = {
>      >           .sb_idl = ovnsb_idl_loop.idl,
> 
> 
> Thanks,
> Ales
> 
> -- 
> 
> Ales Musil
> 
> Senior Software Engineer - OVN Core
> 
> Red Hat EMEA <https://www.redhat.com>
> 
> amusil@redhat.com <mailto:amusil@redhat.com> IM: amusil
> 
> <https://red.ht/sig>
>