On 8/7/25 3:29 PM, Xavier Simonart wrote: > Hi Ales, Dumitru > Hi Xavier,
Thanks for the review! > Thanks for the patch > I have one questions below > > Thanks > Xavier > [...] > 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 <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? > I'm quite sure compilers won't optimize the call to nln_run() away. I'm not sure how that could ever be a valid / safe optimization. Regards, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
