On 9/29/25 10:38 AM, Ales Musil wrote:
> On Sat, Sep 27, 2025 at 12:20 AM Dumitru Ceara <[email protected]> wrote:
>
>> An upcoming patch will take advantage of this routine for creating
>> OpenFlow rules for ARP/ND entries that were learned locally through
>> EVPN.
>>
>> Signed-off-by: Dumitru Ceara <[email protected]>
>> ---
>>
>
> Hi Dumitru,
>
Hi Ales,
> thank you for the patch. I have two small nits below.
>
Thanks for the review!
>
>> controller/automake.mk | 2 +
>> controller/lflow.c | 154 +++++++++++----------------------------
>> controller/neighbor-of.c | 111 ++++++++++++++++++++++++++++
>> controller/neighbor-of.h | 45 ++++++++++++
>> 4 files changed, 200 insertions(+), 112 deletions(-)
>> create mode 100644 controller/neighbor-of.c
>> create mode 100644 controller/neighbor-of.h
>>
>> diff --git a/controller/automake.mk b/controller/automake.mk
>> index d53d932c12..b3c6293fa9 100644
>> --- a/controller/automake.mk
>> +++ b/controller/automake.mk
>> @@ -36,6 +36,8 @@ controller_ovn_controller_SOURCES = \
>> controller/ofctrl-seqno.h \
>> controller/neighbor.c \
>> controller/neighbor.h \
>> + controller/neighbor-of.c \
>> + controller/neighbor-of.h \
>> controller/pinctrl.c \
>> controller/pinctrl.h \
>> controller/patch.c \
>> diff --git a/controller/lflow.c b/controller/lflow.c
>> index 197adaae95..e84fb2486c 100644
>> --- a/controller/lflow.c
>> +++ b/controller/lflow.c
>> @@ -35,6 +35,7 @@
>> #include "lib/ovn-sb-idl.h"
>> #include "lib/extend-table.h"
>> #include "lib/uuidset.h"
>> +#include "neighbor-of.h"
>> #include "packets.h"
>> #include "physical.h"
>> #include "simap.h"
>> @@ -1288,12 +1289,12 @@ consider_logical_flow(const struct
>> sbrec_logical_flow *lflow,
>> }
>>
>> static void
>> -consider_neighbor_flow(struct ovsdb_idl_index *sbrec_port_binding_by_name,
>> - const struct hmap *local_datapaths,
>> - const struct sbrec_mac_binding *b,
>> - const struct sbrec_static_mac_binding *smb,
>> - struct ovn_desired_flow_table *flow_table,
>> - uint16_t priority)
>> +consider_neighbor_flow__(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> + const struct hmap *local_datapaths,
>> + const struct sbrec_mac_binding *b,
>> + const struct sbrec_static_mac_binding *smb,
>> + struct ovn_desired_flow_table *flow_table,
>> + enum neigh_of_rule_prio priority)
>> {
>> if (!b && !smb) {
>> return;
>> @@ -1302,6 +1303,7 @@ consider_neighbor_flow(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> char *logical_port = b ? b->logical_port : smb->logical_port;
>> char *ip = b ? b->ip : smb->ip;
>> char *mac = b ? b->mac : smb->mac;
>> + bool needs_usage_tracking = !!b;
>>
>> const struct sbrec_port_binding *pb
>> = lport_lookup_by_name(sbrec_port_binding_by_name, logical_port);
>> @@ -1317,97 +1319,16 @@ consider_neighbor_flow(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> return;
>> }
>>
>> - struct match get_arp_match = MATCH_CATCHALL_INITIALIZER;
>> - struct match lookup_arp_match = MATCH_CATCHALL_INITIALIZER;
>> - struct match mb_cache_use_match = MATCH_CATCHALL_INITIALIZER;
>> - struct match lookup_arp_for_stats_match = MATCH_CATCHALL_INITIALIZER;
>> -
>> - match_set_dl_src(&lookup_arp_match, mac_addr);
>> - match_set_metadata(&lookup_arp_match,
>> htonll(pb->datapath->tunnel_key));
>> - match_set_reg(&lookup_arp_match, MFF_LOG_INPORT - MFF_REG0,
>> - pb->tunnel_key);
>> -
>> - if (strchr(ip, '.')) {
>> - ovs_be32 ip_addr;
>> - if (!ip_parse(ip, &ip_addr)) {
>> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> - VLOG_WARN_RL(&rl, "bad 'ip' %s", ip);
>> - return;
>> - }
>> -
>> - match_set_reg(&get_arp_match, 0, ntohl(ip_addr));
>> -
>> - match_set_dl_type(&lookup_arp_match, htons(ETH_TYPE_ARP));
>> - lookup_arp_for_stats_match = lookup_arp_match;
>> -
>> - match_set_reg(&lookup_arp_match, 0, ntohl(ip_addr));
>> -
>> - match_set_dl_type(&mb_cache_use_match, htons(ETH_TYPE_IP));
>> - match_set_nw_src(&mb_cache_use_match, ip_addr);
>> -
>> - match_set_arp_opcode_masked(&lookup_arp_for_stats_match, 2, 0xff);
>> - match_set_arp_spa_masked(&lookup_arp_for_stats_match, ip_addr,
>> - htonl(0xffffffff));
>> - } else {
>> - struct in6_addr ip6;
>> - if (!ipv6_parse(ip, &ip6)) {
>> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> - VLOG_WARN_RL(&rl, "bad 'ip' %s", ip);
>> - return;
>> - }
>> - ovs_be128 value;
>> - memcpy(&value, &ip6, sizeof(value));
>> - match_set_xxreg(&get_arp_match, 1, ntoh128(value));
>> -
>> - match_set_dl_type(&lookup_arp_match, htons(ETH_TYPE_IPV6));
>> - match_set_nw_proto(&lookup_arp_match, 58);
>> - match_set_icmp_code(&lookup_arp_match, 0);
>> -
>> - match_set_xxreg(&lookup_arp_match, 0, ntoh128(value));
>> -
>> - match_set_dl_type(&mb_cache_use_match, htons(ETH_TYPE_IPV6));
>> - match_set_ipv6_src(&mb_cache_use_match, &ip6);
>> - }
>> -
>> - match_set_metadata(&get_arp_match, htonll(pb->datapath->tunnel_key));
>> - match_set_reg(&get_arp_match, MFF_LOG_OUTPORT - MFF_REG0,
>> pb->tunnel_key);
>> -
>> - match_set_dl_src(&mb_cache_use_match, mac_addr);
>> - match_set_reg(&mb_cache_use_match, MFF_LOG_INPORT - MFF_REG0,
>> - pb->tunnel_key);
>> - match_set_metadata(&mb_cache_use_match,
>> htonll(pb->datapath->tunnel_key));
>> -
>> - uint64_t stub[1024 / 8];
>> - struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub);
>> - put_load_bytes(mac_addr.ea, sizeof mac_addr.ea, MFF_ETH_DST, 0, 48,
>> - &ofpacts);
>> - put_load(1, MFF_LOG_FLAGS, MLF_LOOKUP_MAC_BIT, 1, &ofpacts);
>> - ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, priority,
>> - b ? b->header_.uuid.parts[0] :
>> smb->header_.uuid.parts[0],
>> - &get_arp_match, &ofpacts,
>> - b ? &b->header_.uuid : &smb->header_.uuid);
>> -
>> - ofpbuf_clear(&ofpacts);
>> - put_load(1, MFF_LOG_FLAGS, MLF_LOOKUP_MAC_BIT, 1, &ofpacts);
>> - ofctrl_add_flow(flow_table, OFTABLE_MAC_LOOKUP, priority,
>> - b ? b->header_.uuid.parts[0] :
>> smb->header_.uuid.parts[0],
>> - &lookup_arp_match, &ofpacts,
>> - b ? &b->header_.uuid : &smb->header_.uuid);
>> -
>> - if (b) {
>> - ofpbuf_clear(&ofpacts);
>> - if (strchr(ip, '.')) {
>> - ofctrl_add_flow(flow_table, OFTABLE_MAC_CACHE_USE, priority,
>> - b->header_.uuid.parts[0],
>> - &lookup_arp_for_stats_match,
>> - &ofpacts, &b->header_.uuid);
>> - }
>> - ofctrl_add_flow(flow_table, OFTABLE_MAC_CACHE_USE, priority,
>> - b->header_.uuid.parts[0], &mb_cache_use_match,
>> - &ofpacts, &b->header_.uuid);
>> + struct in6_addr ip_addr;
>> + if (!ip46_parse(ip, &ip_addr)) {
>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> + VLOG_WARN_RL(&rl, "bad 'ip' %s", ip);
>> + return;
>> }
>>
>> - ofpbuf_uninit(&ofpacts);
>> + consider_neighbor_flow(pb, b ? &b->header_.uuid : &smb->header_.uuid,
>> + &ip_addr, mac_addr, flow_table, priority,
>> + needs_usage_tracking);
>> }
>>
>> /* Adds an OpenFlow flow to flow tables for each MAC binding in the OVN
>> @@ -1422,16 +1343,19 @@ add_neighbor_flows(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> /* Add flows for learnt MAC bindings */
>> const struct sbrec_mac_binding *b;
>> SBREC_MAC_BINDING_TABLE_FOR_EACH (b, mac_binding_table) {
>> - consider_neighbor_flow(sbrec_port_binding_by_name,
>> local_datapaths,
>> - b, NULL, flow_table, 100);
>> + consider_neighbor_flow__(sbrec_port_binding_by_name,
>> local_datapaths,
>> + b, NULL, flow_table,
>> + NEIGH_OF_DYNAMIC_MAC_BINDING_PRIO);
>> }
>>
>> /* Add flows for statically configured MAC bindings */
>> const struct sbrec_static_mac_binding *smb;
>> SBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH (smb, smb_table) {
>> - consider_neighbor_flow(sbrec_port_binding_by_name,
>> local_datapaths,
>> - NULL, smb, flow_table,
>> - smb->override_dynamic_mac ? 150 : 50);
>> + consider_neighbor_flow__(sbrec_port_binding_by_name,
>> local_datapaths,
>> + NULL, smb, flow_table,
>> + smb->override_dynamic_mac
>> + ? NEIGH_OF_STATIC_MAC_BINDING_HIGH_PRIO
>> + : NEIGH_OF_STATIC_MAC_BINDING_LOW_PRIO);
>> }
>> }
>>
>> @@ -1960,8 +1884,9 @@ lflow_handle_changed_mac_bindings(
>> }
>> VLOG_DBG("handle new mac_binding "UUID_FMT,
>> UUID_ARGS(&mb->header_.uuid));
>> - consider_neighbor_flow(sbrec_port_binding_by_name,
>> local_datapaths,
>> - mb, NULL, flow_table, 100);
>> + consider_neighbor_flow__(sbrec_port_binding_by_name,
>> + local_datapaths, mb, NULL,
>> flow_table,
>> + NEIGH_OF_DYNAMIC_MAC_BINDING_PRIO);
>> }
>> }
>> }
>> @@ -1988,9 +1913,11 @@ lflow_handle_changed_static_mac_bindings(
>> }
>> VLOG_DBG("handle new static_mac_binding "UUID_FMT,
>> UUID_ARGS(&smb->header_.uuid));
>> - consider_neighbor_flow(sbrec_port_binding_by_name,
>> local_datapaths,
>> - NULL, smb, flow_table,
>> - smb->override_dynamic_mac ? 150 : 50);
>> + consider_neighbor_flow__(sbrec_port_binding_by_name,
>> + local_datapaths, NULL, smb,
>> flow_table,
>> + smb->override_dynamic_mac
>> + ?
>> NEIGH_OF_STATIC_MAC_BINDING_HIGH_PRIO
>> + :
>> NEIGH_OF_STATIC_MAC_BINDING_LOW_PRIO);
>> }
>> }
>> }
>> @@ -2193,9 +2120,10 @@ lflow_add_flows_for_datapath(const struct
>> sbrec_datapath_binding *dp,
>> const struct sbrec_mac_binding *mb;
>> SBREC_MAC_BINDING_FOR_EACH_EQUAL (
>> mb, mb_index_row, l_ctx_in->sbrec_mac_binding_by_datapath) {
>> - consider_neighbor_flow(l_ctx_in->sbrec_port_binding_by_name,
>> - l_ctx_in->local_datapaths,
>> - mb, NULL, l_ctx_out->flow_table, 100);
>> + consider_neighbor_flow__(l_ctx_in->sbrec_port_binding_by_name,
>> + l_ctx_in->local_datapaths,
>> + mb, NULL, l_ctx_out->flow_table,
>> + NEIGH_OF_DYNAMIC_MAC_BINDING_PRIO);
>> }
>> sbrec_mac_binding_index_destroy_row(mb_index_row);
>>
>> @@ -2206,10 +2134,12 @@ lflow_add_flows_for_datapath(const struct
>> sbrec_datapath_binding *dp,
>> const struct sbrec_static_mac_binding *smb;
>> SBREC_STATIC_MAC_BINDING_FOR_EACH_EQUAL (
>> smb, smb_index_row,
>> l_ctx_in->sbrec_static_mac_binding_by_datapath) {
>> - consider_neighbor_flow(l_ctx_in->sbrec_port_binding_by_name,
>> - l_ctx_in->local_datapaths,
>> - NULL, smb, l_ctx_out->flow_table,
>> - smb->override_dynamic_mac ? 150 : 50);
>> + consider_neighbor_flow__(l_ctx_in->sbrec_port_binding_by_name,
>> + l_ctx_in->local_datapaths,
>> + NULL, smb, l_ctx_out->flow_table,
>> + smb->override_dynamic_mac
>> + ? NEIGH_OF_STATIC_MAC_BINDING_HIGH_PRIO
>> + : NEIGH_OF_STATIC_MAC_BINDING_LOW_PRIO);
>> }
>> sbrec_static_mac_binding_index_destroy_row(smb_index_row);
>>
>> diff --git a/controller/neighbor-of.c b/controller/neighbor-of.c
>> new file mode 100644
>> index 0000000000..ea9d9881ad
>> --- /dev/null
>> +++ b/controller/neighbor-of.c
>> @@ -0,0 +1,111 @@
>> +/* Copyright (c) 2015, 2016, 2017 Nicira, Inc.
>>
>
> nit: I think we shouldn't carry the copyright to a new file,
> but I don't have a strong preference.
>
I think we used both approaches in the past. But more recently we
didn't carry the copyright to new files. I'll do the same here.
> + * 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 "lflow.h"
>> +#include "neighbor-of.h"
>> +#include "openvswitch/match.h"
>> +#include "ovn/logical-fields.h"
>> +
>> +void
>> +consider_neighbor_flow(const struct sbrec_port_binding *pb,
>> + const struct uuid *neighbor_uuid,
>> + const struct in6_addr *ip, struct eth_addr mac,
>> + struct ovn_desired_flow_table *flow_table,
>> + enum neigh_of_rule_prio priority,
>> + bool needs_usage_tracking)
>> +{
>> + struct match get_arp_match = MATCH_CATCHALL_INITIALIZER;
>> + struct match lookup_arp_match = MATCH_CATCHALL_INITIALIZER;
>> + struct match mb_cache_use_match = MATCH_CATCHALL_INITIALIZER;
>> + struct match lookup_arp_for_stats_match = MATCH_CATCHALL_INITIALIZER;
>> +
>> + match_set_dl_src(&lookup_arp_match, mac);
>> + match_set_metadata(&lookup_arp_match,
>> htonll(pb->datapath->tunnel_key));
>> + match_set_reg(&lookup_arp_match, MFF_LOG_INPORT - MFF_REG0,
>> + pb->tunnel_key);
>> +
>> + if (IN6_IS_ADDR_V4MAPPED(ip)) {
>> + ovs_be32 ip_addr = in6_addr_get_mapped_ipv4(ip);
>> + match_set_reg(&get_arp_match, 0, ntohl(ip_addr));
>> +
>> + match_set_dl_type(&lookup_arp_match, htons(ETH_TYPE_ARP));
>> + lookup_arp_for_stats_match = lookup_arp_match;
>> +
>> + match_set_reg(&lookup_arp_match, 0, ntohl(ip_addr));
>> +
>> + match_set_dl_type(&mb_cache_use_match, htons(ETH_TYPE_IP));
>> + match_set_nw_src(&mb_cache_use_match, ip_addr);
>> +
>> + match_set_arp_opcode_masked(&lookup_arp_for_stats_match, 2, 0xff);
>> + match_set_arp_spa_masked(&lookup_arp_for_stats_match, ip_addr,
>> + htonl(0xffffffff));
>> + } else {
>> + ovs_be128 value;
>> + memcpy(&value, ip, sizeof(value));
>> + match_set_xxreg(&get_arp_match, 1, ntoh128(value));
>> +
>> + match_set_dl_type(&lookup_arp_match, htons(ETH_TYPE_IPV6));
>> + match_set_nw_proto(&lookup_arp_match, 58);
>> + match_set_icmp_code(&lookup_arp_match, 0);
>> +
>> + match_set_xxreg(&lookup_arp_match, 0, ntoh128(value));
>> +
>> + match_set_dl_type(&mb_cache_use_match, htons(ETH_TYPE_IPV6));
>> + match_set_ipv6_src(&mb_cache_use_match, ip);
>> + }
>> +
>> + match_set_metadata(&get_arp_match, htonll(pb->datapath->tunnel_key));
>> + match_set_reg(&get_arp_match, MFF_LOG_OUTPORT - MFF_REG0,
>> pb->tunnel_key);
>> +
>> + match_set_dl_src(&mb_cache_use_match, mac);
>> + match_set_reg(&mb_cache_use_match, MFF_LOG_INPORT - MFF_REG0,
>> + pb->tunnel_key);
>> + match_set_metadata(&mb_cache_use_match,
>> htonll(pb->datapath->tunnel_key));
>> +
>> + uint64_t stub[1024 / 8];
>> + struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub);
>> + put_load_bytes(mac.ea, sizeof mac.ea, MFF_ETH_DST, 0, 48,
>> + &ofpacts);
>> + put_load(1, MFF_LOG_FLAGS, MLF_LOOKUP_MAC_BIT, 1, &ofpacts);
>> + ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, priority,
>> + neighbor_uuid->parts[0],
>> + &get_arp_match, &ofpacts,
>> + neighbor_uuid);
>> +
>> + ofpbuf_clear(&ofpacts);
>> + put_load(1, MFF_LOG_FLAGS, MLF_LOOKUP_MAC_BIT, 1, &ofpacts);
>> + ofctrl_add_flow(flow_table, OFTABLE_MAC_LOOKUP, priority,
>> + neighbor_uuid->parts[0],
>> + &lookup_arp_match, &ofpacts,
>> + neighbor_uuid);
>> +
>> + if (needs_usage_tracking) {
>> + ofpbuf_clear(&ofpacts);
>> + if (IN6_IS_ADDR_V4MAPPED(ip)) {
>> + ofctrl_add_flow(flow_table, OFTABLE_MAC_CACHE_USE, priority,
>> + neighbor_uuid->parts[0],
>> + &lookup_arp_for_stats_match,
>> + &ofpacts, neighbor_uuid);
>> + }
>> + ofctrl_add_flow(flow_table, OFTABLE_MAC_CACHE_USE, priority,
>> + neighbor_uuid->parts[0], &mb_cache_use_match,
>> + &ofpacts, neighbor_uuid);
>> + }
>> +
>> + ofpbuf_uninit(&ofpacts);
>> +}
>> diff --git a/controller/neighbor-of.h b/controller/neighbor-of.h
>> new file mode 100644
>> index 0000000000..874200e8be
>> --- /dev/null
>> +++ b/controller/neighbor-of.h
>> @@ -0,0 +1,45 @@
>> +/* Copyright (c) 2015, 2016, 2017 Nicira, Inc.
>>
>
> nit: Same here.
>
Ack.
>
>> + * 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_OF_H
>> +#define NEIGHBOR_OF_H 1
>> +
>> +#include "ovn-sb-idl.h"
>> +#include "openvswitch/uuid.h"
>> +#include "ofctrl.h"
>> +
>> +/* Priorities of ovn-controller generated flows for various types of MAC
>> + * Bindings in different situations. Valid preference orders, based on
>> + * the SB.Static_MAC_Binding.override_dynamic_mac value are:
>> + *
>> + * - static-mac-binding < dynamic-mac-binding
>> + * - dynamic-mac-binding < static-mac-binding
>> + */
>> +enum neigh_of_rule_prio {
>> + NEIGH_OF_STATIC_MAC_BINDING_LOW_PRIO = 50,
>> + NEIGH_OF_DYNAMIC_MAC_BINDING_PRIO = 100,
>> + NEIGH_OF_STATIC_MAC_BINDING_HIGH_PRIO = 150,
>> +};
>> +
>> +void
>> +consider_neighbor_flow(const struct sbrec_port_binding *,
>> + const struct uuid *neighbor_uuid,
>> + const struct in6_addr *, struct eth_addr,
>> + struct ovn_desired_flow_table *,
>> + enum neigh_of_rule_prio priority,
>> + bool needs_usage_tracking);
>> +
>> +#endif /* NEIGHBOR_OF_H */
>> --
>> 2.51.0
>>
>>
> Regardless of the nit:
> Acked-by: Ales Musil <[email protected]>
>
I addressed your comments and pushed the patch to main.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
