On 5 Feb 2025, at 11:26, Kevin Traynor wrote:
> On 03/02/2025 15:20, Eelco Chaudron wrote: >> >> >> On 3 Feb 2025, at 14:33, Roi Dayan wrote: >> >>> On 31/01/2025 12:25, Eelco Chaudron wrote: >>>> >>>> >>>> On 20 Jan 2025, at 11:12, Roi Dayan via dev wrote: >>>> >>>>> From: Eli Britstein <[email protected]> >>>>> >>>>> The tnl/neigh table was used only for the bridge, even if the tunnel's >>>>> underlay port is not the bridge's one (internal port for example). >>>>> Set it to use the correct port. >>>>> >>>>> Signed-off-by: Eli Britstein <[email protected]> >>>>> Acked-by: Roi Dayan <[email protected]> >>>> >>>> Hi Eli, >>>> >>>> Can you give a better explanation/exmaple why we need this change? I >>>> assume we need a test case for this anyway. >>>> >>>> Quickly looking at the code, it assumes that the bridge’s internal port >>>> (which has the same name as the bridge), which has the IP address, is used. >>>> >>>> Cheers, >>>> >>>> Eelco >>> >>> >>> Hi Eelco, >>> >>> Eli is on vacation I'll try to answer here. >>> The case could be when the user set ip not on the bridge netdev which has >>> the bridge name but a different port. e.g. ip is set on a new internal port >>> with vlan on it. >> >> I think this is not what is supported. Looking at the code and all the data >> structures, it all references the bridge and not any port on a bridge. >> >> Maybe other people have more history on this? >> > > Hi Eelco, I don't have history on this either. I agree some of the data > structs/tracing and ovs_router_insert() etc are still using 'bridge'. > Also, the set commands (below) are looking for BRIDGE and using that in > tnl_neigh_set(). > > If there is additional information really needed, perhaps it could be > added and shown with a '-v' option like the tnl/ports/show ? That would > avoid changing existing user commands/output. > > # ovs-appctl list-commands | grep tnl > tnl/arp/aging [SECS] > tnl/arp/flush > tnl/arp/set BRIDGE IP MAC > tnl/arp/show > tnl/egress_port_range min max > tnl/neigh/aging [SECS] > tnl/neigh/flush > tnl/neigh/set BRIDGE IP MAC > tnl/neigh/show > tnl/ports/show -v I didn’t spend too much time on this, but we also need to ensure that the port is part of the (correct) bridge. When it is removed or added, we must handle all the additional processing, including revalidation and other necessary steps. That’s why I believe the original design only supported the IP on the bridge interface. >>> Then it's more correct to show the interface name rather than the bridge >>> name for better visibility. >>> There is no need for a new test case. the existing test is updated to check >>> the output is the port name and not bridge name. >>> >>> Thanks, >>> Roi >>> >>>> >>>>> --- >>>>> lib/tnl-neigh-cache.c | 4 ++-- >>>>> ofproto/ofproto-dpif-xlate.c | 11 ++++++----- >>>>> tests/tunnel-push-pop.at | 2 +- >>>>> 3 files changed, 9 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c >>>>> index bdff1debc805..7e2eaa8262bf 100644 >>>>> --- a/lib/tnl-neigh-cache.c >>>>> +++ b/lib/tnl-neigh-cache.c >>>>> @@ -375,8 +375,8 @@ tnl_neigh_cache_show(struct unixctl_conn *conn, int >>>>> argc OVS_UNUSED, >>>>> struct ds ds = DS_EMPTY_INITIALIZER; >>>>> struct tnl_neigh_entry *neigh; >>>>> >>>>> - ds_put_cstr(&ds, "IP MAC >>>>> Bridge\n"); >>>>> - ds_put_cstr(&ds, >>>>> "==========================================================================\n"); >>>>> + ds_put_cstr(&ds, "IP MAC >>>>> Port\n"); >>>>> + ds_put_cstr(&ds, >>>>> "========================================================================\n"); >>>>> ovs_mutex_lock(&mutex); >>>>> CMAP_FOR_EACH(neigh, cmap_node, &table) { >>>>> int start_len, need_ws; >>>>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >>>>> index 4cc7001a5b25..d13547b4a7f9 100644 >>>>> --- a/ofproto/ofproto-dpif-xlate.c >>>>> +++ b/ofproto/ofproto-dpif-xlate.c >>>>> @@ -3916,17 +3916,18 @@ native_tunnel_output(struct xlate_ctx *ctx, const >>>>> struct xport *xport, >>>>> s_ip = in6_addr_get_mapped_ipv4(&s_ip6); >>>>> } >>>>> >>>>> - err = tnl_neigh_lookup(out_dev->xbridge->name, &d_ip6, &dmac); >>>>> + err = tnl_neigh_lookup(netdev_get_name(out_dev->netdev), &d_ip6, >>>>> &dmac); >>>>> if (err) { >>>>> struct in6_addr nh_s_ip6 = in6addr_any; >>>>> >>>>> xlate_report(ctx, OFT_DETAIL, >>>>> "neighbor cache miss for %s on bridge %s, " >>>>> "sending %s request", >>>>> - buf_dip6, out_dev->xbridge->name, d_ip ? "ARP" : >>>>> "ND"); >>>>> + buf_dip6, netdev_get_name(out_dev->netdev), >>>>> + d_ip ? "ARP" : "ND"); >>>>> >>>>> err = ovs_router_get_netdev_source_address(&d_ip6, >>>>> - >>>>> out_dev->xbridge->name, >>>>> + >>>>> netdev_get_name(out_dev->netdev), >>>>> &nh_s_ip6); >>>>> if (err) { >>>>> nh_s_ip6 = s_ip6; >>>>> @@ -4414,7 +4415,7 @@ terminate_native_tunnel(struct xlate_ctx *ctx, >>>>> const struct xport *xport, >>>>> * do tunnel neighbor snooping. */ >>>>> if (*tnl_port == ODPP_NONE && >>>>> (check_neighbor_reply(ctx, flow) || is_garp(flow, wc))) { >>>>> - tnl_neigh_snoop(flow, wc, ctx->xbridge->name, >>>>> + tnl_neigh_snoop(flow, wc, netdev_get_name(xport->netdev), >>>>> ctx->xin->allow_side_effects); >>>>> } else if (*tnl_port != ODPP_NONE && >>>>> ctx->xin->allow_side_effects && >>>>> @@ -4428,7 +4429,7 @@ terminate_native_tunnel(struct xlate_ctx *ctx, >>>>> const struct xport *xport, >>>>> s_ip6 = flow->ipv6_src; >>>>> } >>>>> >>>>> - tnl_neigh_set(ctx->xbridge->name, &s_ip6, mac); >>>>> + tnl_neigh_set(netdev_get_name(xport->netdev), &s_ip6, mac); >>>>> } >>>>> } >>>>> >>>>> diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at >>>>> index cf4e622014b2..b6635d09afbd 100644 >>>>> --- a/tests/tunnel-push-pop.at >>>>> +++ b/tests/tunnel-push-pop.at >>>>> @@ -1153,7 +1153,7 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p0 >>>>> 'recirc_id(0),in_port(1),dnl >>>>> >>>>> arp(sip=1.1.2.92,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b6,tha=aa:55:aa:55:00:03)']) >>>>> >>>>> AT_CHECK([ovs-appctl tnl/neigh/show | tail -n+3 | sort], [0], [dnl >>>>> -1.1.2.92 f8:bc:12:44:34:b6 br0 >>>>> +1.1.2.92 f8:bc:12:44:34:b6 vtep0 >>>>> ]) >>>>> >>>>> dnl Check GRE tunnel pop. >>>>> -- >>>>> 2.21.0 >>>>> >>>>> _______________________________________________ >>>>> dev mailing list >>>>> [email protected] >>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
