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.
Thanks,
Roi
>
>>>> 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
