Hi Ales, Dumitru

Thanks for the patch
I only have one nit below
Acked-by: Xavier Simonart <xsimo...@redhat.com>
Thanks
Xavier

On Wed, Aug 6, 2025 at 8:29 PM Ales Musil via dev <ovs-dev@openvswitch.org>
wrote:

> On Wed, Aug 6, 2025 at 12:53 PM Dumitru Ceara <dce...@redhat.com> wrote:
>
> > Hi Ales,
> >
>
> Hi Dumitru,
>
> thank you for the review.
>
> >
> > On 7/25/25 8:03 AM, Ales Musil wrote:
> > > Pair learned remote VTEPs with their respective datapaths and tunnel
> > > ports. This creates binding that can be used for physical flows. In
> > > order to accommodate for multicast groups we need to also create
> > > specific EVPN multicast groups.
> >
> > Maybe just add a small note here too (like in 05/11) that the
> > "dynamic-routing-vni" cannot be set through the NB yet and will be part
> > of a later commit?
> >
> > Otherwise, I only have a few tiny comments below, the rest looks good to
> > me.
> >
> > >
> > > All data are ovn-controller specific due to the nature of EVPN and
> > > also to avoid possible scalability issues within SB database.
> > >
> > > Those bindings and multicast groups are used in later commits to
> > > create physical flows responsible for the delivery.
> > >
> > > Signed-off-by: Ales Musil <amu...@redhat.com>
> > > ---
> > >  controller/automake.mk      |   2 +
> > >  controller/evpn-binding.c   | 427 ++++++++++++++++++++++++++++++++++++
> > >  controller/evpn-binding.h   |  88 ++++++++
> > >  controller/ovn-controller.c | 200 ++++++++++++++++-
> > >  lib/ovn-util.c              |  16 ++
> > >  lib/ovn-util.h              |   5 +
> > >  6 files changed, 727 insertions(+), 11 deletions(-)
> > >  create mode 100644 controller/evpn-binding.c
> > >  create mode 100644 controller/evpn-binding.h
> > >
> > > diff --git a/controller/automake.mk b/controller/automake.mk
> > > index 37cfd4396..8d94fb646 100644
> > > --- a/controller/automake.mk
> > > +++ b/controller/automake.mk
> > > @@ -10,6 +10,8 @@ controller_ovn_controller_SOURCES = \
> > >       controller/chassis.h \
> > >       controller/encaps.c \
> > >       controller/encaps.h \
> > > +     controller/evpn-binding.c \
> > > +     controller/evpn-binding.h \
> > >       controller/ha-chassis.c \
> > >       controller/ha-chassis.h \
> > >       controller/if-status.c \
> > > diff --git a/controller/evpn-binding.c b/controller/evpn-binding.c
> > > new file mode 100644
> > > index 000000000..86cb7df27
> > > --- /dev/null
> > > +++ b/controller/evpn-binding.c
> > > @@ -0,0 +1,427 @@
> > > +/* Copyright (c) 2025, 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 "flow.h"
> > > +#include "local_data.h"
> > > +#include "neighbor-exchange.h"
> > > +#include "openvswitch/vlog.h"
> > > +#include "ovn-sb-idl.h"
> > > +#include "unixctl.h"
> > > +#include "vec.h"
> > > +#include "vswitch-idl.h"
> > > +
> > > +#include "evpn-binding.h"
> > > +
> > > +VLOG_DEFINE_THIS_MODULE(evpn_binding);
> > > +
> > > +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> > > +
> > > +struct evpn_datapath {
> > > +    uint32_t vni;
> > > +    uint32_t dp_key;
> > > +};
> > > +
> > > +static struct vector collect_evpn_datapaths(
> > > +    const struct hmap *local_datapaths);
> > > +static const struct evpn_datapath *evpn_datapath_find(
> > > +    const struct vector *evpn_datapaths, uint32_t vni);
> > > +
> > > +struct evpn_tunnel {
> > > +    uint16_t dst_port;
> > > +    ofp_port_t ofport;
> > > +};
> > > +
> > > +static struct vector collect_evpn_tunnel_interfaces(
> > > +    const struct ovsrec_bridge *br_int);
> > > +static bool is_evpn_tunnel_port(const struct ovsrec_port *port);
> >
> > Nit: 'port' can be omitted.
> >
> > > +static const struct evpn_tunnel *evpn_tunnel_find(
> > > +    const struct vector *evpn_tunnels, uint16_t dst_port);
> > > +
> > > +static struct evpn_binding *evpn_binding_add(
> > > +    struct hmap *evpn_bindings, const struct evpn_remote_vtep *vtep,
> > > +    uint32_t tunnel_key);
> > > +static uint32_t evpn_binding_hash(const struct in6_addr *remote_ip,
> > > +                                  uint32_t vni);
> > > +static struct evpn_multicast_group *evpn_multicast_group_find(
> > > +    const struct hmap *evpn_mc_groups, uint32_t vni);
> > > +static struct evpn_multicast_group *evpn_multicast_group_add(
> > > +    struct hmap *evpn_mc_groups, uint32_t vni);
> > > +
> > > +void
> > > +evpn_binding_run(const struct evpn_binding_ctx_in *b_ctx_in,
> > > +                 struct evpn_binding_ctx_out *b_ctx_out)
> > > +{
> > > +    struct vector datapaths =
> > > +        collect_evpn_datapaths(b_ctx_in->local_datapaths);
> > > +    struct vector tunnels =
> > collect_evpn_tunnel_interfaces(b_ctx_in->br_int);
> > > +    struct hmapx stale_bindings = HMAPX_INITIALIZER(&stale_bindings);
> > > +    struct hmapx stale_mc_groups =
> HMAPX_INITIALIZER(&stale_mc_groups);
> > > +    uint32_t hint = OVN_MIN_EVPN_KEY;
> > > +
> > > +    struct evpn_binding *binding;
> > > +    HMAP_FOR_EACH (binding, hmap_node, b_ctx_out->bindings) {
> > > +        hmapx_add(&stale_bindings, binding);
> > > +    }
> > > +
> > > +    struct evpn_multicast_group *mc_group;
> > > +    HMAP_FOR_EACH (mc_group, hmap_node, b_ctx_out->multicast_groups) {
> > > +        hmapx_add(&stale_mc_groups, mc_group);
> > > +    }
> > > +
> > > +    const struct evpn_remote_vtep *vtep;
> > > +    HMAP_FOR_EACH (vtep, hmap_node, b_ctx_in->remote_vteps) {
> > > +        const struct evpn_tunnel *tun = evpn_tunnel_find(&tunnels,
> > vtep->port);
> > > +        if (!tun) {
> > > +            VLOG_WARN_RL(&rl, "Couldn't find EVPN tunnel for %"PRIu16
> > > +                         " destination port.", vtep->port);
> > > +            continue;
> > > +        }
> > > +
> > > +        const struct evpn_datapath *edp =
> evpn_datapath_find(&datapaths,
> > > +
>  vtep->vni);
> > > +        if (!edp) {
> > > +            VLOG_WARN_RL(&rl, "Couldn't find EVPN datapath for
> > %"PRIu16" VNI",
> > > +                         vtep->vni);
> > > +            continue;
> > > +        }
> > > +
> > > +        binding = evpn_binding_find(b_ctx_out->bindings, &vtep->ip,
> > vtep->vni);
> > > +        if (!binding) {
> > > +            uint32_t tunnel_key =
> > > +                ovn_allocate_tnlid(b_ctx_out->tunnel_keys,
> > "evpn-binding",
> > > +                                   OVN_MIN_EVPN_KEY, OVN_MAX_EVPN_KEY,
> > &hint);
> > > +            if (!tunnel_key) {
> > > +                continue;
> > > +            }
> > > +
> > > +            binding = evpn_binding_add(b_ctx_out->bindings, vtep,
> > tunnel_key);
> > > +        }
> > > +
> > > +        mc_group =
> > evpn_multicast_group_find(b_ctx_out->multicast_groups,
> > > +                                             vtep->vni);
> > > +        if (!mc_group) {
> > > +            mc_group =
> > evpn_multicast_group_add(b_ctx_out->multicast_groups,
> > > +                                                vtep->vni);
> > > +        }
> > > +
> > > +        bool updated = false;
> > > +        if (binding->tunnel_ofport != tun->ofport) {
> > > +            binding->tunnel_ofport = tun->ofport;
> > > +            updated = true;
> > > +        }
> > > +
> > > +        if (binding->dp_key != edp->dp_key) {
> > > +            binding->dp_key = edp->dp_key;
> > > +            updated = true;
> > > +        }
> > > +
> > > +        if (updated) {
> > > +            hmapx_add(b_ctx_out->updated_bindings, binding);
> > > +
> > > +            hmapx_add(&mc_group->bindings, binding);
> > > +            hmapx_add(b_ctx_out->updated_multicast_groups, mc_group);
> > > +        }
> > > +
> > > +        hmapx_find_and_delete(&stale_bindings, binding);
> > > +        hmapx_find_and_delete(&stale_mc_groups, mc_group);
> > > +    }
> > > +
> > > +    struct hmapx_node *node;
> > > +    HMAPX_FOR_EACH (node, &stale_mc_groups) {
> > > +        mc_group = node->data;
> > > +
> > > +        uuidset_insert(b_ctx_out->removed_multicast_groups,
> > > +                       &mc_group->flow_uuid);
> > > +        hmap_remove(b_ctx_out->multicast_groups,
> &mc_group->hmap_node);
> > > +        free(mc_group);
> > > +    }
> > > +
> > > +    HMAPX_FOR_EACH (node, &stale_bindings) {
> > > +        binding = node->data;
> > > +
> > > +        mc_group =
> > evpn_multicast_group_find(b_ctx_out->multicast_groups,
> > > +                                             binding->vni);
> > > +        if (mc_group) {
> > > +            hmapx_find_and_delete(&mc_group->bindings, binding);
> > > +            hmapx_add(b_ctx_out->updated_multicast_groups, mc_group);
> > > +        }
> > > +        uuidset_insert(b_ctx_out->removed_bindings,
> > &binding->flow_uuid);
> > > +        hmap_remove(b_ctx_out->bindings, &binding->hmap_node);
> > > +        free(binding);
> > > +    }
> > > +
> > > +    vector_destroy(&datapaths);
> > > +    vector_destroy(&tunnels);
> > > +    hmapx_destroy(&stale_bindings);
> > > +    hmapx_destroy(&stale_mc_groups);
> > > +}
> > > +
> > > +struct evpn_binding *
> > > +evpn_binding_find(const struct hmap *evpn_bindings,
> > > +                  const struct in6_addr *remote_ip, uint32_t vni)
> > > +{
> > > +    uint32_t hash = evpn_binding_hash(remote_ip, vni);
> > > +
> > > +    struct evpn_binding *binding;
> > > +    HMAP_FOR_EACH_WITH_HASH (binding, hmap_node, hash, evpn_bindings)
> {
> > > +        if (ipv6_addr_equals(&binding->remote_ip, remote_ip) &&
> > > +            binding->vni == vni) {
> > > +            return binding;
> > > +        }
> > > +    }
> > > +
> > > +    return NULL;
> > > +}
> > > +
> > > +void
> > > +evpn_bindings_destroy(struct hmap *bindings)
> > > +{
> > > +    struct evpn_binding *binding;
> > > +    HMAP_FOR_EACH_POP (binding, hmap_node, bindings) {
> > > +        free(binding);
> > > +    }
> > > +    hmap_destroy(bindings);
> > > +}
> > > +
> > > +void
> > > +evpn_vtep_binding_list(struct unixctl_conn *conn, int argc OVS_UNUSED,
> > > +                        const char *argv[] OVS_UNUSED, void *data_)
> > > +{
> > > +    struct hmap *bindings = data_;
> > > +    struct ds ds = DS_EMPTY_INITIALIZER;
> > > +
> > > +    const struct evpn_binding *binding;
> > > +    HMAP_FOR_EACH (binding, hmap_node, bindings) {
> > > +        ds_put_format(&ds, "UUID: "UUID_FMT", Remote IP: ",
> > > +                      UUID_ARGS(&binding->flow_uuid));
> > > +        ipv6_format_mapped(&binding->remote_ip, &ds);
> > > +        ds_put_format(&ds, ", vni: %"PRIu32", tunnel_key: %#"PRIx32",
> "
> > > +                      "ofport: %"PRIu32", dp_key: %"PRIu32,
> > binding->vni,
> > > +                      binding->tunnel_key, binding->tunnel_ofport,
> > > +                      binding->dp_key);
> > > +        ds_put_char(&ds, '\n');
> > > +    }
> > > +
> > > +    unixctl_command_reply(conn, ds_cstr_ro(&ds));
> > > +    ds_destroy(&ds);
> > > +}
> > > +
> > > +void
> > > +evpn_multicast_groups_destroy(struct hmap *multicast_groups)
> > > +{
> > > +    struct evpn_multicast_group *mc_group;
> > > +    HMAP_FOR_EACH_POP (mc_group, hmap_node, multicast_groups) {
> > > +        free(mc_group);
> > > +    }
> > > +    hmap_destroy(multicast_groups);
> > > +}
> > > +
> > > +
> > > +void
> > > +evpn_multicast_group_list(struct unixctl_conn *conn, int argc
> > OVS_UNUSED,
> > > +                           const char *argv[] OVS_UNUSED, void *data_)
> > > +{
> > > +    struct hmap *mc_groups = data_;
> > > +    struct ds ds = DS_EMPTY_INITIALIZER;
> > > +
> > > +    const struct evpn_multicast_group *mc_group;
> > > +    HMAP_FOR_EACH (mc_group, hmap_node, mc_groups) {
> > > +        ds_put_format(&ds, "UUID: "UUID_FMT", Remote IPs: ",
> > > +                      UUID_ARGS(&mc_group->flow_uuid));
> > > +
> > > +        struct hmapx_node *node;
> > > +        HMAPX_FOR_EACH (node, &mc_group->bindings) {
> > > +            const struct evpn_binding *binding = node->data;
> > > +            ipv6_format_mapped(&binding->remote_ip, &ds);
> > > +            ds_put_cstr(&ds, ", ");
> > > +        }
> > > +
> > > +        ds_put_format(&ds, "vni: %"PRIu32, mc_group->vni);
> > > +        ds_put_char(&ds, '\n');
> > > +    }
> > > +
> > > +    unixctl_command_reply(conn, ds_cstr_ro(&ds));
> > > +    ds_destroy(&ds);
> > > +}
> > > +
> > > +static struct vector
> > > +collect_evpn_datapaths(const struct hmap *local_datapaths)
> > > +{
> > > +    struct vector evpn_datapaths =
> > > +        VECTOR_EMPTY_INITIALIZER(struct evpn_datapath);
> > > +
> > > +    struct local_datapath *ld;
> > > +    HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
> > > +        if (!ld->is_switch) {
> > > +            continue;
> > > +        }
> > > +
> > > +        int64_t vni = ovn_smap_get_llong(&ld->datapath->external_ids,
> > > +                                         "dynamic-routing-vni", -1);
> > > +        if (!ovn_is_valid_vni(vni)) {
> > > +            continue;
> > > +        }
> > > +
> > > +        if (evpn_datapath_find(&evpn_datapaths, vni)) {
> > > +            VLOG_WARN_RL(&rl, "Datapath "UUID_FMT" with duplicate VNI
> > %"PRIi64,
> > > +                         UUID_ARGS(&ld->datapath->header_.uuid), vni);
> > > +            continue;
> > > +        }
> > > +
> > > +        struct evpn_datapath edp = {
> > > +            .vni = vni,
> > > +            .dp_key = ld->datapath->tunnel_key,
> > > +        };
> > > +        vector_push(&evpn_datapaths, &edp);
> > > +    }
> > > +
> > > +    return evpn_datapaths;
> > > +}
> > > +
> > > +static const struct evpn_datapath *
> > > +evpn_datapath_find(const struct vector *evpn_datapaths, uint32_t vni)
> > > +{
> > > +    const struct evpn_datapath *edp;
> > > +    VECTOR_FOR_EACH_PTR (evpn_datapaths, edp) {
> > > +        if (edp->vni == vni) {
> > > +            return edp;
> > > +        }
> > > +    }
> > > +
> > > +    return NULL;
> > > +}
> > > +
> > > +static struct vector
> > > +collect_evpn_tunnel_interfaces(const struct ovsrec_bridge *br_int)
> > > +{
> > > +    struct vector evpn_tunnels = VECTOR_EMPTY_INITIALIZER(struct
> > evpn_tunnel);
> > > +
> > > +    for (size_t i = 0; i < br_int->n_ports; i++) {
> > > +        const struct ovsrec_port *port = br_int->ports[i];
> > > +
> > > +        if (!is_evpn_tunnel_port(port)) {
> > > +            continue;
> > > +        }
> > > +
> > > +        const struct ovsrec_interface *iface = port->interfaces[0];
> > > +        if (iface->n_ofport != 1) {
> > > +            continue;
> > > +        }
> > > +
> > > +        const char *dst_port_str =
> > > +            smap_get_def(&iface->options, "dst_port",
> > > +                         OVS_STRINGIZE(DEFAULT_VXLAN_PORT));
> > > +        unsigned int dst_port;
> > > +        if (!str_to_uint(dst_port_str, 10, &dst_port)) {
> > > +            VLOG_WARN_RL(&rl, "Couldn't parse \"dst_port\" %s for
> > tunnel"
> > > +                         " interface %s", dst_port_str, iface->name);
> > > +            continue;
> > > +        }
> > > +
> > > +        struct evpn_tunnel tun = {
> > > +            .dst_port = dst_port,
> > > +            .ofport = u16_to_ofp(iface->ofport[0]),
> > > +        };
> > > +        vector_push(&evpn_tunnels, &tun);
> > > +    }
> > > +
> > > +    return evpn_tunnels;
> > > +}
> > > +
> > > +static bool
> > > +is_evpn_tunnel_port(const struct ovsrec_port *port)
> > > +{
> > > +    if (port->n_interfaces != 1) {
> > > +        return false;
> > > +    }
> > > +
> > > +    return ovn_is_evpn_tunnel_interface(port->interfaces[0]);
> > > +}
> > > +
> > > +static const struct evpn_tunnel *
> > > +evpn_tunnel_find(const struct vector *evpn_tunnels, uint16_t dst_port)
> > > +{
> > > +    const struct evpn_tunnel *tun;
> > > +    VECTOR_FOR_EACH_PTR (evpn_tunnels, tun) {
> > > +        if (tun->dst_port == dst_port) {
> > > +            return tun;
> > > +        }
> > > +    }
> > > +
> > > +    return NULL;
> > > +}
> > > +
> > > +static uint32_t
> > > +evpn_binding_hash(const struct in6_addr *remote_ip, uint32_t vni)
> > > +{
> > > +    uint32_t hash = 0;
> > > +    hash = hash_add_in6_addr(hash, remote_ip);
> > > +    hash = hash_add(hash, vni);
> > > +
> > > +    return hash;
> > > +}
> > > +
> > > +static struct evpn_binding *
> > > +evpn_binding_add(struct hmap *evpn_bindings,
> > > +                 const struct evpn_remote_vtep *vtep, uint32_t
> > tunnel_key)
> > > +{
> > > +    struct evpn_binding *binding = xmalloc(sizeof *binding);
> > > +    *binding = (struct evpn_binding) {
> > > +        .flow_uuid = uuid_random(),
> > > +        .remote_ip = vtep->ip,
> > > +        .vni = vtep->vni,
> > > +        .tunnel_key = tunnel_key,
> > > +        .tunnel_ofport = OFPP_NONE,
> > > +        .dp_key = 0,
> > > +    };
> > > +
> > > +    uint32_t hash = evpn_binding_hash(&vtep->ip, vtep->vni);
> > > +    hmap_insert(evpn_bindings, &binding->hmap_node, hash);
> > > +
> > > +    return binding;
> > > +}
> > > +
> > > +static struct evpn_multicast_group *
> > > +evpn_multicast_group_find(const struct hmap *evpn_mc_groups, uint32_t
> > vni)
> > > +{
> > > +    uint32_t hash = hash_add(0, vni);
> > > +
> > > +    struct evpn_multicast_group *mc_group;
> > > +    HMAP_FOR_EACH_WITH_HASH (mc_group, hmap_node, hash,
> evpn_mc_groups)
> > {
> > > +        if (mc_group->vni == vni) {
> > > +            return mc_group;
> > > +        }
> > > +    }
> > > +
> > > +    return NULL;
> > > +}
> > > +
> > > +static struct evpn_multicast_group *
> > > +evpn_multicast_group_add(struct hmap *evpn_mc_groups, uint32_t vni)
> > > +{
> > > +    struct evpn_multicast_group *mc_group = xmalloc(sizeof *mc_group);
> > > +    *mc_group = (struct evpn_multicast_group) {
> > > +        .flow_uuid = uuid_random(),
> > > +        .bindings = HMAPX_INITIALIZER(&mc_group->bindings),
> > > +        .vni = vni,
> > > +    };
> > > +
> > > +    uint32_t hash = hash_add(0, vni);
> > > +    hmap_insert(evpn_mc_groups, &mc_group->hmap_node, hash);
> > > +
> > > +    return mc_group;
> > > +}
> > > diff --git a/controller/evpn-binding.h b/controller/evpn-binding.h
> > > new file mode 100644
> > > index 000000000..88d8c202c
> > > --- /dev/null
> > > +++ b/controller/evpn-binding.h
> > > @@ -0,0 +1,88 @@
> > > +/* Copyright (c) 2025, 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 EVPN_BINDING_H
> > > +#define EVPN_BINDING_H 1
> > > +
> > > +#include <stdint.h>
> > > +
> > > +#include "hmapx.h"
> > > +#include "openvswitch/hmap.h"
> > > +#include "uuidset.h"
> > > +
> > > +struct ovsrec_bridge;
> > > +struct unixctl_conn;
> > > +
> > > +struct evpn_binding_ctx_in {
> > > +    const struct ovsrec_bridge *br_int;
> > > +    /* Contains 'struct local_datapath'. */
> > > +    const struct hmap *local_datapaths;
> > > +    /* Contains 'struct evpn_remote_vtep'. */
> > > +    const struct hmap *remote_vteps;
> > > +};
> > > +
> > > +struct evpn_binding_ctx_out {
> > > +    /* Contains 'struct evpn_binding'. */
> > > +    struct hmap *bindings;
> > > +    /* Contains pointers to 'struct evpn_binding'. */
> > > +    struct hmapx *updated_bindings;
> > > +    /* Contains 'flow_uuid' from removed 'struct evpn_binding'. */
> > > +    struct uuidset *removed_bindings;
> > > +    /* Contains 'struct evpn_multicast_group'. */
> > > +    struct hmap *multicast_groups;
> > > +    /* Contains pointers to 'struct evpn_multicast_group'. */
> > > +    struct hmapx *updated_multicast_groups;
> > > +    /* Contains 'flow_uuid' from removed 'struct
> evpn_multicast_group'.
> > */
> > > +    struct uuidset *removed_multicast_groups;
> > > +    /* Contains 'struct tnlid_node". */
> > > +    struct hmap *tunnel_keys;
> > > +};
> > > +
> > > +struct evpn_binding {
> > > +    struct hmap_node hmap_node;
> > > +    /* UUID used to identify physical flows related to this binding.
> */
> > > +    struct uuid flow_uuid;
> > > +    /* IP address of the remote VTEP. */
> > > +    struct in6_addr remote_ip;
> > > +    uint32_t vni;
> > > +    /* Local tunnel key to identify the binding. */
> > > +    uint32_t tunnel_key;
> > > +
> > > +    ofp_port_t tunnel_ofport;
> > > +    uint32_t dp_key;
> > > +};
> > > +
> > > +struct evpn_multicast_group {
> > > +    struct hmap_node hmap_node;
> > > +    /* UUID used to identify physical flows related to this mutlicast
> > group. */
> > > +    struct uuid flow_uuid;
> > > +    /* Contains pointers to 'struct evpn_bindings'. */
> > > +    struct hmapx bindings;
> > > +    uint32_t vni;
> > > +};
> > > +
> > > +void evpn_binding_run(const struct evpn_binding_ctx_in *,
> > > +                      struct evpn_binding_ctx_out *);
> > > +struct evpn_binding *evpn_binding_find(const struct hmap
> *evpn_bindings,
> > > +                                       const struct in6_addr
> *remote_ip,
> > > +                                       uint32_t vni);
> > > +void evpn_bindings_destroy(struct hmap *bindings);
> > > +void evpn_vtep_binding_list(struct unixctl_conn *conn, int argc,
> > > +                             const char *argv[], void *data_);
> > > +void evpn_multicast_groups_destroy(struct hmap *multicast_groups);
> > > +void evpn_multicast_group_list(struct unixctl_conn *conn, int argc,
> > > +                                const char *argv[], void *data_);
> > > +
> > > +#endif /* EVPN_BINDING_H */
> > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > > index 90ef6011a..996dcc021 100644
> > > --- a/controller/ovn-controller.c
> > > +++ b/controller/ovn-controller.c
> > > @@ -98,6 +98,7 @@
> > >  #include "neighbor.h"
> > >  #include "neighbor-exchange.h"
> > >  #include "neighbor-table-notify.h"
> > > +#include "evpn-binding.h"
> > >
> > >  VLOG_DEFINE_THIS_MODULE(main);
> > >
> > > @@ -4558,6 +4559,23 @@ parse_encap_ips(const struct
> > ovsrec_open_vswitch_table *ovs_table,
> > >      }
> > >  }
> > >
> > > +struct ed_type_evpn_vtep_binding {
> > > +    /* Contains 'struct evpn_binding'. */
> > > +    struct hmap bindings;
> > > +    /* Contains pointers to 'struct evpn_binding'. */
> > > +    struct hmapx updated_bindings;
> > > +    /* Contains 'flow_uuid' from removed 'struct evpn_binding'. */
> > > +    struct uuidset removed_bindings;
> > > +    /* Contains 'struct evpn_multicast_group'. */
> > > +    struct hmap multicast_groups;
> > > +    /* Contains pointers to 'struct evpn_multicast_group'. */
> > > +    struct hmapx updated_multicast_groups;
> > > +    /* Contains 'flow_uuid' from removed 'struct
> evpn_multicast_group'.
> > */
> > > +    struct uuidset removed_multicast_groups;
> > > +    /* Contains 'struct tnlid_node". */
> > > +    struct hmap tunnel_keys;
> > > +};
> >
> > Nit: I'd add some empty lines between the fields here.  I think it would
> > be a bit more readable.
> >
>
>
> I kinda prefer them as is, but if you feel like I should
> add empty lines. I can do it in v3.
>
>
> >
> > > +
> > >  static void init_physical_ctx(struct engine_node *node,
> > >                                struct ed_type_runtime_data *rt_data,
> > >                                struct ed_type_non_vif_data
> *non_vif_data,
> > > @@ -4988,14 +5006,6 @@ controller_output_garp_rarp_handler(struct
> > engine_node *node OVS_UNUSED,
> > >      return EN_HANDLED_UPDATED;
> > >  }
> > >
> > > -static enum engine_input_handler_result
> > > -controller_output_neighbor_exchange_handler(
> > > -    struct engine_node *node OVS_UNUSED,
> > > -    void *data OVS_UNUSED)
> > > -{
> > > -    return EN_HANDLED_UPDATED;
> > > -}
> > > -
> > >  /* Handles sbrec_chassis changes.
> > >   * If a new chassis is added or removed return false, so that
> > >   * flows are recomputed.  For any updates, there is no need for
> > > @@ -5943,6 +5953,152 @@ en_neighbor_exchange_status_run(struct
> > engine_node *node OVS_UNUSED,
> > >      return state;
> > >  }
> > >
> > > +static void *
> > > +en_evpn_vtep_binding_init(struct engine_node *node OVS_UNUSED,
> > > +                          struct engine_arg *arg OVS_UNUSED)
> > > +{
> > > +    struct ed_type_evpn_vtep_binding *data = xmalloc(sizeof *data);
> > > +    *data = (struct ed_type_evpn_vtep_binding) {
> > > +        .bindings = HMAP_INITIALIZER(&data->bindings),
> > > +        .updated_bindings =
> HMAPX_INITIALIZER(&data->updated_bindings),
> > > +        .removed_bindings =
> > UUIDSET_INITIALIZER(&data->removed_bindings),
> > > +        .multicast_groups = HMAP_INITIALIZER(&data->multicast_groups),
> > > +        .updated_multicast_groups =
> > > +            HMAPX_INITIALIZER(&data->updated_multicast_groups),
> > > +        .removed_multicast_groups =
> > > +            UUIDSET_INITIALIZER(&data->removed_multicast_groups),
> > > +        .tunnel_keys = HMAP_INITIALIZER(&data->tunnel_keys),
> > > +    };
> > > +
> > > +    return data;
> > > +}
> > > +
> > > +static void
> > > +en_evpn_vtep_binding_clear_tracked_data(void *data_)
> > > +{
> > > +    struct ed_type_evpn_vtep_binding *data = data_;
> > > +    hmapx_clear(&data->updated_bindings);
> > > +    uuidset_clear(&data->removed_bindings);
> > > +    hmapx_clear(&data->updated_multicast_groups);
> > > +    uuidset_clear(&data->removed_multicast_groups);
> > > +}
> > > +
> > > +static void
> > > +en_evpn_vtep_binding_cleanup(void *data_)
> > > +{
> > > +    struct ed_type_evpn_vtep_binding *data = data_;
> > > +    evpn_bindings_destroy(&data->bindings);
> > > +    hmapx_destroy(&data->updated_bindings);
> > > +    uuidset_destroy(&data->removed_bindings);
> > > +    evpn_multicast_groups_destroy(&data->multicast_groups);
> > > +    hmapx_clear(&data->updated_multicast_groups);
> > > +    uuidset_clear(&data->removed_multicast_groups);
> > > +    ovn_destroy_tnlids(&data->tunnel_keys);
> > > +}
> > > +
> > > +static enum engine_node_state
> > > +en_evpn_vtep_binding_run(struct engine_node *node, void *data_)
> > > +{
> > > +    struct ed_type_evpn_vtep_binding *data = data_;
> > > +    const struct ed_type_neighbor_exchange *ne_data =
> > > +        engine_get_input_data("neighbor_exchange", node);
> > > +    const struct ed_type_runtime_data *rt_data =
> > > +        engine_get_input_data("runtime_data", node);
> > > +    const struct ovsrec_open_vswitch_table *ovs_table =
> > > +        EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node));
> > > +    const struct ovsrec_bridge_table *bridge_table =
> > > +        EN_OVSDB_GET(engine_get_input("OVS_bridge", node));
> > > +    const struct ovsrec_bridge *br_int = get_br_int(bridge_table,
> > ovs_table);
> > > +
> > > +    struct evpn_binding_ctx_in b_ctx_in = {
> > > +        .br_int = br_int,
> > > +        .local_datapaths = &rt_data->local_datapaths,
> > > +        .remote_vteps = &ne_data->remote_vteps,
> > > +    };
> > > +
> > > +    struct evpn_binding_ctx_out b_ctx_out = {
> > > +        .bindings = &data->bindings,
> > > +        .updated_bindings = &data->updated_bindings,
> > > +        .removed_bindings = &data->removed_bindings,
> > > +        .multicast_groups = &data->multicast_groups,
> > > +        .updated_multicast_groups = &data->updated_multicast_groups,
> > > +        .removed_multicast_groups = &data->removed_multicast_groups,
> > > +        .tunnel_keys = &data->tunnel_keys,
> > > +    };
> > > +
> > > +    evpn_binding_run(&b_ctx_in, &b_ctx_out);
> > > +
> > > +    if (hmapx_count(&data->updated_bindings) ||
> > > +        uuidset_count(&data->removed_bindings) ||
> > > +        hmapx_count(&data->updated_multicast_groups) ||
> > > +        uuidset_count(&data->removed_multicast_groups)) {
> > > +        return EN_UPDATED;
> > > +    }
> > > +
> > > +    return EN_UNCHANGED;
> > > +}
> > > +
> > > +static enum engine_input_handler_result
> > > +evpn_vtep_binding_ovs_interface_handler(struct engine_node *node,
> > > +                                        void *data OVS_UNUSED)
> > > +{
> > > +    const struct ovsrec_interface_table *iface_table =
> > > +        EN_OVSDB_GET(engine_get_input("OVS_interface", node));
> > > +
> > > +    const struct ovsrec_interface *iface;
> > > +    OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface, iface_table) {
> > > +        if (!ovn_is_evpn_tunnel_interface(iface)) {
> > > +            continue;
> > > +        }
> > > +
> > > +        if (ovsrec_interface_is_new(iface) ||
> > > +            ovsrec_interface_is_deleted(iface) ||
> > > +            ovsrec_interface_is_updated(iface,
> > OVSREC_INTERFACE_COL_OFPORT)) {
>
nit: I would at least add a comment on why handling ovs interface option
change (e.g. handling setting "flow" on local_ip)
is not needed. I think that it is not needed as (among other potential
recomputes) ovs interface changes for vxlan
port w/o iface-id would result in runtime_data recompute (we might also
wonder whether runtime_data recompute
on vxlan changes is really needed, but this can be done in a separate/later
optimization patch).

> > > +            return EN_UNHANDLED;
> > > +        }
> > > +    }
> > > +
> > > +    return EN_HANDLED_UNCHANGED;
> > > +}
> > > +
> > > +static enum engine_input_handler_result
> > > +evpn_vtep_binding_datapath_binding_handler(struct engine_node *node,
> > > +                                           void *data OVS_UNUSED)
> > > +{
> > > +    const struct sbrec_datapath_binding_table *dp_table =
> > > +        EN_OVSDB_GET(engine_get_input("SB_datapath_binding", node));
> > > +    struct ed_type_runtime_data *rt_data =
> > > +        engine_get_input_data("runtime_data", node);
> > > +
> > > +    const struct sbrec_datapath_binding *dp;
> > > +    SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (dp, dp_table) {
> > > +        if (sbrec_datapath_binding_is_new(dp) ||
> > > +            sbrec_datapath_binding_is_deleted(dp)) {
> > > +            /* The removal and addition is handled via the neighbor.
> */
> >
> > Nit: maybe the more explicit "via the en_neighbor_exchange I-P node."?
> >
> > > +            return EN_HANDLED_UNCHANGED;
> > > +        }
> > > +
> > > +        struct local_datapath *ld =
> > > +            get_local_datapath(&rt_data->local_datapaths,
> > dp->tunnel_key);
> > > +        if (!ld || !ld->is_switch) {
> > > +            continue;
> > > +        }
> > > +
> > > +        int64_t vni = ovn_smap_get_llong(&dp->external_ids,
> > > +                                         "dynamic-routing-vni", -1);
> > > +        if (!ovn_is_valid_vni(vni)) {
> > > +            continue;
> > > +        }
> > > +
> > > +        if (sbrec_datapath_binding_is_updated(
> > > +                dp, SBREC_DATAPATH_BINDING_COL_TUNNEL_KEY)) {
> > > +            return EN_UNHANDLED;
> > > +        }
> > > +    }
> > > +
> > > +    return EN_HANDLED_UNCHANGED;
> > > +}
> > > +
> > >  /* Returns false if the northd internal version stored in SB_Global
> > >   * and ovn-controller internal version don't match.
> > >   */
> > > @@ -6257,6 +6413,7 @@ main(int argc, char *argv[])
> > >      ENGINE_NODE(neighbor_table_notify);
> > >      ENGINE_NODE(neighbor_exchange);
> > >      ENGINE_NODE(neighbor_exchange_status);
> > > +    ENGINE_NODE(evpn_vtep_binding, CLEAR_TRACKED_DATA);
> > >
> > >  #define SB_NODE(NAME) ENGINE_NODE_SB(NAME);
> > >      SB_NODES
> > > @@ -6492,6 +6649,22 @@ main(int argc, char *argv[])
> > >      engine_add_input(&en_neighbor_exchange,
> > &en_neighbor_exchange_status,
> > >                       NULL);
> > >
> > > +    engine_add_input(&en_evpn_vtep_binding, &en_ovs_open_vswitch,
> NULL);
> > > +    engine_add_input(&en_evpn_vtep_binding, &en_ovs_bridge, NULL);
> > > +    engine_add_input(&en_evpn_vtep_binding, &en_neighbor_exchange,
> > NULL);
> > > +    /* The runtime_data are needed only for local datapaths, any
> update
> > of
> > > +     * local datapath will be reflected via en_neighbor_exchange. */
> > > +    engine_add_input(&en_evpn_vtep_binding, &en_runtime_data,
> > > +                     engine_noop_handler);
> > > +    engine_add_input(&en_evpn_vtep_binding, &en_ovs_interface,
> > > +                     evpn_vtep_binding_ovs_interface_handler);
> > > +    engine_add_input(&en_evpn_vtep_binding, &en_sb_datapath_binding,
> > > +                     evpn_vtep_binding_datapath_binding_handler);
> > > +
> > > +    /* TODO Add real handler to process all bindings. */
> > > +    engine_add_input(&en_pflow_output, &en_evpn_vtep_binding,
> > > +                     engine_noop_handler);
> > > +
> > >      engine_add_input(&en_controller_output, &en_dns_cache,
> > >                       NULL);
> > >      engine_add_input(&en_controller_output, &en_lflow_output,
> > > @@ -6511,9 +6684,6 @@ main(int argc, char *argv[])
> > >      engine_add_input(&en_controller_output, &en_acl_id,
> > >                       controller_output_acl_id_handler);
> > >
> > > -    engine_add_input(&en_controller_output, &en_neighbor_exchange,
> > > -                     controller_output_neighbor_exchange_handler);
> > > -
> > >      struct engine_arg engine_arg = {
> > >          .sb_idl = ovnsb_idl_loop.idl,
> > >          .ovs_idl = ovs_idl_loop.idl,
> > > @@ -6573,6 +6743,8 @@ main(int argc, char *argv[])
> > >              engine_get_internal_data(&en_mac_cache);
> > >      struct ed_type_neighbor_exchange *ne_data =
> > >          engine_get_internal_data(&en_neighbor_exchange);
> > > +    struct ed_type_evpn_vtep_binding *eb_data =
> > > +        engine_get_internal_data(&en_evpn_vtep_binding);
> > >
> > >      ofctrl_init(&lflow_output_data->group_table,
> > >                  &lflow_output_data->meter_table);
> > > @@ -6592,6 +6764,12 @@ main(int argc, char *argv[])
> > >      unixctl_command_register("evpn/remote-vtep-list", "", 0, 0,
> > >                               evpn_remote_vtep_list,
> > >                               &ne_data->remote_vteps);
> > > +    unixctl_command_register("evpn/vtep-binding-list", "", 0, 0,
> > > +                             evpn_vtep_binding_list,
> > > +                             &eb_data->bindings);
> > > +    unixctl_command_register("evpn/vtep-multicast-group-list", "", 0,
> 0,
> > > +                             evpn_multicast_group_list,
> > > +                             &eb_data->multicast_groups);
> > >
> > >      struct pending_pkt pending_pkt = { .conn = NULL };
> > >      unixctl_command_register("inject-pkt", "MICROFLOW", 1, 1,
> > inject_pkt,
> > > diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> > > index c12de4da9..e8a32c96a 100644
> > > --- a/lib/ovn-util.c
> > > +++ b/lib/ovn-util.c
> > > @@ -1490,3 +1490,19 @@ ovn_is_valid_vni(int64_t vni)
> > >  {
> > >      return vni >= 0 && (vni <= (1 << 24) - 1);
> > >  }
> > > +
> > > +bool
> > > +ovn_is_evpn_tunnel_interface(const struct ovsrec_interface *iface)
> > > +{
> > > +    if (strcmp(iface->type, "vxlan")) {
> > > +        return false;
> > > +    }
> > > +
> > > +    if (strcmp(smap_get_def(&iface->options, "local_ip", ""), "flow")
> ||
> > > +        strcmp(smap_get_def(&iface->options, "remote_ip", ""), "flow")
> > ||
> > > +        strcmp(smap_get_def(&iface->options, "key", ""), "flow")) {
> > > +        return false;
> > > +    }
> > > +
> > > +    return true;
> > > +}
> > > diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> > > index f31eba344..1bdd98675 100644
> > > --- a/lib/ovn-util.h
> > > +++ b/lib/ovn-util.h
> > > @@ -49,6 +49,7 @@ struct svec;
> > >  struct uuid;
> > >  struct unixctl_conn;
> > >  struct rconn;
> > > +struct ovsrec_interface;
> > >
> > >  struct ipv4_netaddr {
> > >      ovs_be32 addr;            /* 192.168.10.123 */
> > > @@ -167,6 +168,9 @@ void set_idl_probe_interval(struct ovsdb_idl *idl,
> > const char *remote,
> > >  #define OVN_MIN_DP_VXLAN_KEY_GLOBAL (OVN_MAX_DP_VXLAN_KEY_LOCAL + 1)
> > >  #define OVN_MAX_DP_VXLAN_KEY_GLOBAL ((1u << 12) - 1)
> > >
> > > +#define OVN_MIN_EVPN_KEY (1u << 31)
> > > +#define OVN_MAX_EVPN_KEY (OVN_MAX_DP_GLOBAL_NUM | OVN_MIN_EVPN_KEY)
> > > +
> > >  struct hmap;
> > >  void ovn_destroy_tnlids(struct hmap *tnlids);
> > >  bool ovn_add_tnlid(struct hmap *set, uint32_t tnlid);
> > > @@ -501,6 +505,7 @@ bool find_prefix_in_set(const struct in6_addr
> > *prefix, unsigned int plen,
> > >  void ovn_debug_commands_register(void);
> > >
> > >  bool ovn_is_valid_vni(int64_t vni);
> > > +bool ovn_is_evpn_tunnel_interface(const struct ovsrec_interface
> *iface);
> > >
> > >  const struct sbrec_port_binding *lport_lookup_by_name(
> > >      struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >
> > Regards,
> > Dumitru
> >
> >
> Everything else should be addressed in v2.
>
> Thanks,
> Ales
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to