Hi Ales, Dumitru
Thanks for the patch
I have one questions below
Thanks
Xavier
On Wed, Aug 6, 2025 at 8:27 PM Ales Musil via dev <ovs-dev@openvswitch.org>
wrote:
> From: Dumitru Ceara <dce...@redhat.com>
>
> For each (Linux) interface that's interesting for the OVN control plane,
> watch for neighbor (fdb/arp) changes. This allows us to reconcile on
> changed neighbor entries from outside routing agents (e.g., FRR).
>
> This is the neighbor equivalent of the support added for route tables in
> 673d90f1173f ("controller: Watch for route changes.").
>
> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> ---
> Changes in V2:
> - fixed up log messages
> - changed code to make OVN only manage neighbor entries with VLAN 0
> - added advertise_neigh_find() helper
> ---
> controller/automake.mk | 5 +-
> controller/neighbor-exchange-netlink.c | 27 +--
> controller/neighbor-table-notify-stub.c | 57 ++++++
> controller/neighbor-table-notify.c | 244 ++++++++++++++++++++++++
> controller/neighbor-table-notify.h | 45 +++++
> controller/neighbor.c | 19 +-
> controller/neighbor.h | 5 +-
> 7 files changed, 386 insertions(+), 16 deletions(-)
> create mode 100644 controller/neighbor-table-notify-stub.c
> create mode 100644 controller/neighbor-table-notify.c
> create mode 100644 controller/neighbor-table-notify.h
>
> diff --git a/controller/automake.mk b/controller/automake.mk
> index 3eb45475c..6af6ee2a9 100644
> --- a/controller/automake.mk
> +++ b/controller/automake.mk
> @@ -63,18 +63,21 @@ controller_ovn_controller_SOURCES = \
> controller/route.h \
> controller/route.c \
> controller/garp_rarp.h \
> - controller/garp_rarp.c
> + controller/garp_rarp.c \
> + controller/neighbor-table-notify.h
>
> if HAVE_NETLINK
> controller_ovn_controller_SOURCES += \
> controller/neighbor-exchange-netlink.h \
> controller/neighbor-exchange-netlink.c \
> + controller/neighbor-table-notify.c \
> controller/route-exchange-netlink.h \
> controller/route-exchange-netlink.c \
> controller/route-exchange.c \
> controller/route-table-notify.c
> else
> controller_ovn_controller_SOURCES += \
> + controller/neighbor-table-notify-stub.c \
> controller/route-exchange-stub.c \
> controller/route-table-notify-stub.c
> endif
> diff --git a/controller/neighbor-exchange-netlink.c
> b/controller/neighbor-exchange-netlink.c
> index 3b9642da0..95c7e0d7d 100644
> --- a/controller/neighbor-exchange-netlink.c
> +++ b/controller/neighbor-exchange-netlink.c
> @@ -287,6 +287,11 @@ handle_ne_msg(const struct ne_table_msg *msg, void
> *data)
> struct ne_msg_handle_data *handle_data = data;
> const struct ne_nl_received_neigh *nd = &msg->nd;
>
> + /* OVN only manages VLAN 0 entries. */
> + if (nd->vlan) {
> + return;
> + }
> +
> if (!ne_is_ovn_owned(nd)) {
> if (!handle_data->learned_neighbors) {
> return;
> @@ -298,17 +303,15 @@ handle_ne_msg(const struct ne_table_msg *msg, void
> *data)
> }
>
> /* This neighbor was presumably added by OVN, see if it's still valid.
> - * OVN only adds neighbors with vlan and port set to 0, all others
> - * can be removed. */
> - if (!nd->vlan && !nd->port && handle_data->neighbors_to_advertise) {
> - uint32_t hash = advertise_neigh_hash(&nd->lladdr, &nd->addr);
> - struct advertise_neighbor_entry *an;
> - HMAP_FOR_EACH_WITH_HASH (an, node, hash, handle_data->neighbors) {
> - if (eth_addr_equals(an->lladdr, nd->lladdr)
> - && ipv6_addr_equals(&an->addr, &nd->addr)) {
> -
> hmapx_find_and_delete(handle_data->neighbors_to_advertise, an);
> - return;
> - }
> + * OVN only adds neighbors with port set to 0, all others can be
> + * removed. */
> + if (!nd->port && handle_data->neighbors_to_advertise) {
> + struct advertise_neighbor_entry *an =
> + advertise_neigh_find(handle_data->neighbors, nd->lladdr,
> + &nd->addr);
> + if (an) {
> + hmapx_find_and_delete(handle_data->neighbors_to_advertise,
> an);
> + return;
> }
> }
>
> @@ -341,7 +344,7 @@ ne_nl_add_neigh(int32_t if_index, uint8_t family,
> uint16_t port, uint16_t vlan)
> {
> uint32_t nl_flags = NLM_F_REQUEST | NLM_F_ACK |
> - NLM_F_CREATE | NLM_F_EXCL;
> + NLM_F_CREATE | NLM_F_REPLACE;
> bool dst_set = !ipv6_is_zero(addr);
> struct ofpbuf request;
> uint8_t request_stub[NETNL_REQ_BUFFER_SIZE];
> diff --git a/controller/neighbor-table-notify-stub.c
> b/controller/neighbor-table-notify-stub.c
> new file mode 100644
> index 000000000..bb4fe5991
> --- /dev/null
> +++ b/controller/neighbor-table-notify-stub.c
> @@ -0,0 +1,57 @@
> +/* 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 <stdbool.h>
> +
> +#include "openvswitch/compiler.h"
> +#include "neighbor-table-notify.h"
> +
> +bool
> +neighbor_table_notify_run(void)
> +{
> + return false;
> +}
> +
> +void
> +neighbor_table_notify_wait(void)
> +{
> +}
> +
> +void
> +neighbor_table_add_watch_request(
> + struct hmap *neighbor_table_watches OVS_UNUSED,
> + int32_t if_index OVS_UNUSED,
> + const char *if_name OVS_UNUSED)
> +{
> +}
> +
> +void
> +neighbor_table_watch_request_cleanup(
> + struct hmap *neighbor_table_watches OVS_UNUSED)
> +{
> +}
> +
> +void
> +neighbor_table_notify_update_watches(
> + const struct hmap *neighbor_table_watches OVS_UNUSED)
> +{
> +}
> +
> +void
> +neighbor_table_notify_destroy(void)
> +{
> +}
> diff --git a/controller/neighbor-table-notify.c
> b/controller/neighbor-table-notify.c
> new file mode 100644
> index 000000000..19b975e39
> --- /dev/null
> +++ b/controller/neighbor-table-notify.c
> @@ -0,0 +1,244 @@
> +/* 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 <linux/rtnetlink.h>
> +#include <net/if.h>
> +
> +#include "hash.h"
> +#include "hmapx.h"
> +#include "lib/util.h"
> +#include "netlink-notifier.h"
> +#include "openvswitch/vlog.h"
> +
> +#include "neighbor-exchange-netlink.h"
> +#include "neighbor-table-notify.h"
> +
> +VLOG_DEFINE_THIS_MODULE(neighbor_table_notify);
> +
> +struct neighbor_table_watch_request {
> + struct hmap_node node;
> + int32_t if_index;
> + char if_name[IFNAMSIZ + 1];
> +};
> +
> +struct neighbor_table_watch_entry {
> + struct hmap_node node;
> + int32_t if_index;
> + char if_name[IFNAMSIZ + 1];
> +};
> +
> +static struct hmap watches = HMAP_INITIALIZER(&watches);
> +static bool any_neighbor_table_changed;
> +static struct ne_table_msg nln_nmsg_change;
> +
> +static struct nln *nl_neighbor_handle;
> +static struct nln_notifier *nl_neighbor_notifier;
> +
> +static void fdb_table_change(const void *change_, void *aux);
> +
> +static void
> +neighbor_table_register_notifiers(void)
> +{
> + VLOG_INFO("Adding neighbor table watchers.");
> + ovs_assert(!nl_neighbor_handle);
> +
> + nl_neighbor_handle = nln_create(NETLINK_ROUTE, ne_table_parse,
> + &nln_nmsg_change);
> + ovs_assert(nl_neighbor_handle);
> +
> + nl_neighbor_notifier =
> + nln_notifier_create(nl_neighbor_handle, RTNLGRP_NEIGH,
> + fdb_table_change, NULL);
> + if (!nl_neighbor_notifier) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> + VLOG_WARN_RL(&rl, "Failed to create neighbor table watcher.");
> + }
> +}
> +
> +static void
> +neighbor_table_deregister_notifiers(void)
> +{
> + VLOG_INFO("Removing neighbor table watchers.");
> + ovs_assert(nl_neighbor_handle);
> +
> + nln_notifier_destroy(nl_neighbor_notifier);
> + nln_destroy(nl_neighbor_handle);
> + nl_neighbor_notifier = NULL;
> + nl_neighbor_handle = NULL;
> +}
> +
> +static uint32_t
> +neighbor_table_notify_hash_watch(int32_t if_index)
> +{
> + /* To allow lookups triggered by netlink messages, don't include the
> + * if_name in the hash. The netlink updates only include if_index. */
> + return hash_int(if_index, 0);
> +}
> +
> +static void
> +add_watch_entry(int32_t if_index, const char *if_name)
> +{
> + VLOG_INFO("Registering new neighbor table watcher "
> + "for if_index %s (%"PRId32").",
> + if_name, if_index);
> +
> + struct neighbor_table_watch_entry *we;
> + uint32_t hash = neighbor_table_notify_hash_watch(if_index);
> + we = xzalloc(sizeof *we);
> + we->if_index = if_index;
> + ovs_strzcpy(we->if_name, if_name, IFNAMSIZ + 1);
> + hmap_insert(&watches, &we->node, hash);
> +
> + if (!nl_neighbor_handle) {
> + neighbor_table_register_notifiers();
> + }
> +}
> +
> +static void
> +remove_watch_entry(struct neighbor_table_watch_entry *we)
> +{
> + VLOG_INFO("Removing neighbor table watcher for table %s (%"PRId32").",
> + we->if_name, we->if_index);
> + hmap_remove(&watches, &we->node);
> + free(we);
> +
> + if (hmap_is_empty(&watches)) {
> + neighbor_table_deregister_notifiers();
> + }
> +}
> +
> +bool
> +neighbor_table_notify_run(void)
> +{
> + any_neighbor_table_changed = false;
> +
> + if (nl_neighbor_handle) {
> + nln_run(nl_neighbor_handle);
> + }
> +
>
+ return any_neighbor_table_changed;
>
Is there no risk of having some compiler/compiler options "optimizing"
any_neighbor_table_changed and always returning false?
> +}
> +
> +void
> +neighbor_table_notify_wait(void)
> +{
> + if (nl_neighbor_handle) {
> + nln_wait(nl_neighbor_handle);
> + }
> +}
> +
> +void
> +neighbor_table_add_watch_request(struct hmap *neighbor_table_watches,
> + int32_t if_index, const char *if_name)
> +{
> + struct neighbor_table_watch_request *wr = xzalloc(sizeof *wr);
> +
> + wr->if_index = if_index;
> + ovs_strzcpy(wr->if_name, if_name, IFNAMSIZ + 1);
> + hmap_insert(neighbor_table_watches, &wr->node,
> + neighbor_table_notify_hash_watch(wr->if_index));
> +}
> +
> +void
> +neighbor_table_watch_request_cleanup(struct hmap *neighbor_table_watches)
> +{
> + struct neighbor_table_watch_request *wr;
> + HMAP_FOR_EACH_POP (wr, node, neighbor_table_watches) {
> + free(wr);
> + }
> +}
> +
> +static struct neighbor_table_watch_entry *
> +find_watch_entry(int32_t if_index, const char *if_name)
> +{
> + struct neighbor_table_watch_entry *we;
> + uint32_t hash = neighbor_table_notify_hash_watch(if_index);
> + HMAP_FOR_EACH_WITH_HASH (we, node, hash, &watches) {
> + if (if_index == we->if_index && !strcmp(if_name, we->if_name)) {
> + return we;
> + }
> + }
> + return NULL;
> +}
> +
> +static struct neighbor_table_watch_entry *
> +find_watch_entry_by_if_index(int32_t if_index)
> +{
> + struct neighbor_table_watch_entry *we;
> + uint32_t hash = neighbor_table_notify_hash_watch(if_index);
> + HMAP_FOR_EACH_WITH_HASH (we, node, hash, &watches) {
> + if (if_index == we->if_index) {
> + return we;
> + }
> + }
> + return NULL;
> +}
> +
> +void
> +neighbor_table_notify_update_watches(const struct hmap
> *neighbor_table_watches)
> +{
> + struct hmapx sync_watches = HMAPX_INITIALIZER(&sync_watches);
> + struct neighbor_table_watch_entry *we;
> + HMAP_FOR_EACH (we, node, &watches) {
> + hmapx_add(&sync_watches, we);
> + }
> +
> + struct neighbor_table_watch_request *wr;
> + HMAP_FOR_EACH (wr, node, neighbor_table_watches) {
> + we = find_watch_entry(wr->if_index, wr->if_name);
> + if (we) {
> + hmapx_find_and_delete(&sync_watches, we);
> + } else {
> + add_watch_entry(wr->if_index, wr->if_name);
> + }
> + }
> +
> + struct hmapx_node *node;
> + HMAPX_FOR_EACH (node, &sync_watches) {
> + remove_watch_entry(node->data);
> + }
> +
> + hmapx_destroy(&sync_watches);
> +}
> +
> +void
> +neighbor_table_notify_destroy(void)
> +{
> + struct neighbor_table_watch_entry *we;
> + HMAP_FOR_EACH_SAFE (we, node, &watches) {
> + remove_watch_entry(we);
> + }
> +}
> +
> +static void
> +fdb_table_change(const void *change_, void *aux OVS_UNUSED)
> +{
> + /* We currently track whether at least one recent neighbor table
> change
> + * was detected. If that's the case already there's no need to
> + * continue. */
> + if (any_neighbor_table_changed) {
> + return;
> + }
> +
> + const struct ne_table_msg *change = change_;
> +
> + if (change && !ne_is_ovn_owned(&change->nd)) {
> + if (find_watch_entry_by_if_index(change->nd.if_index)) {
> + any_neighbor_table_changed = true;
> + }
> + }
> +}
> diff --git a/controller/neighbor-table-notify.h
> b/controller/neighbor-table-notify.h
> new file mode 100644
> index 000000000..9f21271cc
> --- /dev/null
> +++ b/controller/neighbor-table-notify.h
> @@ -0,0 +1,45 @@
> +/* 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 NEIGHBOR_TABLE_NOTIFY_H
> +#define NEIGHBOR_TABLE_NOTIFY_H 1
> +
> +#include <stdbool.h>
> +#include "openvswitch/hmap.h"
> +
> +/* Returns true if any neighbor table has changed enough that we need
> + * to learn new neighbor entries. */
> +bool neighbor_table_notify_run(void);
> +void neighbor_table_notify_wait(void);
> +
> +/* Add a watch request to the hmap. The hmap should later be passed to
> + * neighbor_table_notify_update_watches*/
> +void neighbor_table_add_watch_request(struct hmap *neighbor_table_watches,
> + int32_t if_index, const char
> *if_name);
> +
> +/* Cleanup all watch request in the provided hmap that where added using
> + * neighbor_table_add_watch_request. */
> +void neighbor_table_watch_request_cleanup(
> + struct hmap *neighbor_table_watches);
> +
> +/* Updates the list of neighbor table watches that are currently active.
> + * hmap should contain struct neighbor_table_watch_request */
> +void neighbor_table_notify_update_watches(
> + const struct hmap *neighbor_table_watches);
> +
> +/* Cleans up all neighbor table watches. */
> +void neighbor_table_notify_destroy(void);
> +
> +#endif /* NEIGHBOR_TABLE_NOTIFY_H */
> diff --git a/controller/neighbor.c b/controller/neighbor.c
> index 8bfc2cf0d..150372fb2 100644
> --- a/controller/neighbor.c
> +++ b/controller/neighbor.c
> @@ -24,12 +24,29 @@
> static void neighbor_interface_monitor_destroy(
> struct neighbor_interface_monitor *);
>
> -uint32_t
> +static uint32_t
> advertise_neigh_hash(const struct eth_addr *eth, const struct in6_addr
> *ip)
> {
> return hash_bytes(ip, sizeof *ip, hash_bytes(eth, sizeof *eth, 0));
> }
>
> +struct advertise_neighbor_entry *
> +advertise_neigh_find(const struct hmap *neighbors, struct eth_addr mac,
> + const struct in6_addr *ip)
> +{
> + uint32_t hash = advertise_neigh_hash(&mac, ip);
> +
> + struct advertise_neighbor_entry *ne;
> + HMAP_FOR_EACH_WITH_HASH (ne, node, hash, neighbors) {
> + if (eth_addr_equals(ne->lladdr, mac) &&
> + ipv6_addr_equals(&ne->addr, ip)) {
> + return ne;
> + }
> + }
> +
> + return NULL;
> +}
> +
> void
> neighbor_run(struct neighbor_ctx_in *n_ctx_in OVS_UNUSED,
> struct neighbor_ctx_out *n_ctx_out OVS_UNUSED)
> diff --git a/controller/neighbor.h b/controller/neighbor.h
> index 3abc6e923..4cfb19c2b 100644
> --- a/controller/neighbor.h
> +++ b/controller/neighbor.h
> @@ -64,8 +64,9 @@ struct advertise_neighbor_entry {
> * all zero otherwise. */
> };
>
> -uint32_t advertise_neigh_hash(const struct eth_addr *,
> - const struct in6_addr *);
> +struct advertise_neighbor_entry *advertise_neigh_find(
> + const struct hmap *neighbors, struct eth_addr mac,
> + const struct in6_addr *ip);
> void neighbor_run(struct neighbor_ctx_in *, struct neighbor_ctx_out *);
> void neighbor_cleanup(struct vector *monitored_interfaces);
>
> --
> 2.50.1
>
> _______________________________________________
> 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
