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
>> 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
