On 16 Feb 2025, at 8:53, Roi Dayan wrote:
> On 14/02/2025 12:47, Eelco Chaudron wrote: >> >> >> On 5 Feb 2025, at 11:45, Roi Dayan wrote: >> >>> On 05/02/2025 12: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. >> >> >> Hi Roi, >> >> Sorry for the late response; I was quite busy with other tasks. However, I >> did find some time to dive into this piece of code. (I also found some other >> areas for improvement and will send patches later.) >> >> The cache is nothing more than a neighbor table, and having a port in the >> output does not provide any benefits. In fact, it adds more confusion, as >> the port is not related to the entry at all; i.e., it does not represent the >> egress port. The actual egress port is determined by the OpenFlow rules. >> >> This differs from, for example, the kernel neighbor table, which does show >> the actual egress port. Given this, I believe it’s best to keep the current >> output, as it clearly indicates that the packet will exit via the mentioned >> bridge using the existing OpenFlow rules. >> >> Cheers, >> >> Eelco > > Hi, > > Ok we can drop this patch but we thought it could improve output as we didn't > really see a scenario were ip is configured on port A while there will be an > openflow rule to force output to port B anyway. Not sure what configuration you are referring to, but for 99%, the port the system IP is assigned on will not be the output port. Mainly because the default NORMAL rule will not allow this. In addition, if you are able to force the output to a physical port with custom OpenFlow rules, this will only work on DPDK ports with a bifurcated driver. >>>>> 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 >>>> >>> >>> Even though documented tnl/neigh/set to accept a bridge I see how the show >>> command fits >>> but what if ip was set with externally from ovs with ip command? >>> Also in the test the ip is not set on the bridge. In the test case it was >>> set with netdev-dummy/ip4addr and this is why the test was modified to >>> print vtep0 and not br0. >>> >>> >>>>>> 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
