On 12/04/2025 1:06, Ilya Maximets wrote:
> On 4/10/25 2:00 PM, Roi Dayan wrote:
>> From: Eli Britstein <el...@nvidia.com>
>>
>> Upon tunnel output failure, due to routing failure for example, add an
>> explicit drop action, so it will appear in the dp-flows for better
>> visibility for that case.
>> For those, add additional drop reasons.
>>
>> Signed-off-by: Eli Britstein <el...@nvidia.com>
>> Reviewed-by: Roi Dayan <r...@nvidia.com>
>> ---
>>
>> Notes:
>> v2
>> - Add new translation errors for new cases.
>> - Introduce put_cloned_drop_action() for tunnel cases for readability.
>> - Add test.
>>
>> include/linux/openvswitch.h | 4 ++++
>> lib/odp-execute.c | 16 +++++++++++++
>> ofproto/ofproto-dpif-xlate.c | 44 ++++++++++++++++++++++++++++++++++++
>> tests/tunnel-push-pop.at | 17 ++++++++++++++
>> 4 files changed, 81 insertions(+)
>>
>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>> index 48b3a73227f6..accd75548175 100644
>> --- a/include/linux/openvswitch.h
>> +++ b/include/linux/openvswitch.h
>> @@ -462,6 +462,10 @@ enum xlate_error {
>> XLATE_RECIRCULATION_CONFLICT,
>> XLATE_TOO_MANY_MPLS_LABELS,
>> XLATE_INVALID_TUNNEL_METADATA,
>> + XLATE_TUNNEL_ROUTING_FAILED,
>> + XLATE_TUNNEL_OUTPUT_NO_ETHERNET,
>> + XLATE_TUNNEL_NEIGH_CACHE_MISS,
>> + XLATE_TUNNEL_HEADER_BUILD_FAILED,
>
> These should be added to the end of the enum. This is part of the public API
> and people may rely on the numeric values of these errors. So, we should
> avoid
> changing them.
Ack for all the comments. I'll send v3 fixed. thanks.
>
>> XLATE_UNSUPPORTED_PACKET_TYPE,
>> XLATE_CONGESTION_DROP,
>> XLATE_FORWARDING_DISABLED,
>> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
>> index 15577d5394fa..0ae5a6265240 100644
>> --- a/lib/odp-execute.c
>> +++ b/lib/odp-execute.c
>> @@ -53,6 +53,10 @@ COVERAGE_DEFINE(drop_action_no_recirculation_context);
>> COVERAGE_DEFINE(drop_action_recirculation_conflict);
>> COVERAGE_DEFINE(drop_action_too_many_mpls_labels);
>> COVERAGE_DEFINE(drop_action_invalid_tunnel_metadata);
>> +COVERAGE_DEFINE(drop_action_tunnel_routing_failed);
>> +COVERAGE_DEFINE(drop_action_tunnel_output_no_ethernet);
>> +COVERAGE_DEFINE(drop_action_tunnel_neigh_cache_miss);
>> +COVERAGE_DEFINE(drop_action_tunnel_header_build_failed);
>
> Since enum should be re-ordered, these should also go to the end according
> to the enum definition order.
>
>> COVERAGE_DEFINE(drop_action_unsupported_packet_type);
>> COVERAGE_DEFINE(drop_action_congestion);
>> COVERAGE_DEFINE(drop_action_forwarding_disabled);
>> @@ -91,6 +95,18 @@ dp_update_drop_action_counter(enum xlate_error
>> drop_reason,
>> case XLATE_INVALID_TUNNEL_METADATA:
>> COVERAGE_ADD(drop_action_invalid_tunnel_metadata, delta);
>> break;
>> + case XLATE_TUNNEL_ROUTING_FAILED:
>> + COVERAGE_ADD(drop_action_tunnel_routing_failed, delta);
>> + break;
>> + case XLATE_TUNNEL_OUTPUT_NO_ETHERNET:
>> + COVERAGE_ADD(drop_action_tunnel_output_no_ethernet, delta);
>> + break;
>> + case XLATE_TUNNEL_NEIGH_CACHE_MISS:
>> + COVERAGE_ADD(drop_action_tunnel_neigh_cache_miss, delta);
>> + break;
>> + case XLATE_TUNNEL_HEADER_BUILD_FAILED:
>> + COVERAGE_ADD(drop_action_tunnel_header_build_failed, delta);
>> + break;
>
> Same here.
>
>> case XLATE_UNSUPPORTED_PACKET_TYPE:
>> COVERAGE_ADD(drop_action_unsupported_packet_type, delta);
>> break;
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 406d083b3fa8..9add20633fe4 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -449,6 +449,14 @@ const char *xlate_strerror(enum xlate_error error)
>> return "Too many MPLS labels";
>> case XLATE_INVALID_TUNNEL_METADATA:
>> return "Invalid tunnel metadata";
>> + case XLATE_TUNNEL_ROUTING_FAILED:
>> + return "Native tunnel routing failed";
>> + case XLATE_TUNNEL_OUTPUT_NO_ETHERNET:
>> + return "Tunnel output no ethernet";
>
> "Tunnel output device lacks Ethernet address"
>
>> + case XLATE_TUNNEL_NEIGH_CACHE_MISS:
>> + return "Tunnel neighbor cache miss";
>> + case XLATE_TUNNEL_HEADER_BUILD_FAILED:
>> + return "Native tunnel header build failed";
>
> And the order here as well.
>
>> case XLATE_UNSUPPORTED_PACKET_TYPE:
>> return "Unsupported packet type";
>> case XLATE_CONGESTION_DROP:
>> @@ -461,6 +469,10 @@ const char *xlate_strerror(enum xlate_error error)
>> return "Unknown error";
>> }
>>
>> +static void put_cloned_drop_action(struct ofproto_dpif *ofproto,
>> + struct ofpbuf *odp_actions,
>> + enum xlate_error error,
>> + bool cloned);
>> static void xlate_action_set(struct xlate_ctx *ctx);
>> static void xlate_commit_actions(struct xlate_ctx *ctx);
>>
>> @@ -3893,6 +3905,8 @@ native_tunnel_output(struct xlate_ctx *ctx, const
>> struct xport *xport,
>>
>> err = tnl_route_lookup_flow(ctx, flow, &d_ip6, &s_ip6, &out_dev);
>> if (err) {
>> + put_cloned_drop_action(ctx->xbridge->ofproto, ctx->odp_actions,
>> + XLATE_TUNNEL_ROUTING_FAILED,
>> !is_last_action);
>> xlate_report(ctx, OFT_WARN, "native tunnel routing failed");
>> return err;
>> }
>> @@ -3904,6 +3918,9 @@ native_tunnel_output(struct xlate_ctx *ctx, const
>> struct xport *xport,
>> /* Use mac addr of bridge port of the peer. */
>> err = netdev_get_etheraddr(out_dev->netdev, &smac);
>> if (err) {
>> + put_cloned_drop_action(ctx->xbridge->ofproto, ctx->odp_actions,
>> + XLATE_TUNNEL_OUTPUT_NO_ETHERNET,
>> + !is_last_action);
>> xlate_report(ctx, OFT_WARN,
>> "tunnel output device lacks Ethernet address");
>> return err;
>> @@ -3918,6 +3935,9 @@ native_tunnel_output(struct xlate_ctx *ctx, const
>> struct xport *xport,
>> if (err) {
>> struct in6_addr nh_s_ip6 = in6addr_any;
>>
>> + put_cloned_drop_action(ctx->xbridge->ofproto, ctx->odp_actions,
>> + XLATE_TUNNEL_NEIGH_CACHE_MISS,
>> + !is_last_action);
>> xlate_report(ctx, OFT_DETAIL,
>> "neighbor cache miss for %s on bridge %s, "
>> "sending %s request",
>> @@ -3958,6 +3978,9 @@ native_tunnel_output(struct xlate_ctx *ctx, const
>> struct xport *xport,
>> netdev_init_tnl_build_header_params(&tnl_params, flow, &s_ip6, dmac,
>> smac);
>> err = tnl_port_build_header(xport->ofport, &tnl_push_data, &tnl_params);
>> if (err) {
>> + put_cloned_drop_action(ctx->xbridge->ofproto, ctx->odp_actions,
>> + XLATE_TUNNEL_HEADER_BUILD_FAILED,
>> + !is_last_action);
>> xlate_report(ctx, OFT_WARN, "native tunnel header build failed");
>> return err;
>> }
>> @@ -6508,6 +6531,27 @@ put_drop_action(struct ofproto_dpif *ofproto, struct
>> ofpbuf *odp_actions,
>> nl_msg_put_u32(odp_actions, OVS_ACTION_ATTR_DROP, error);
>> }
>>
>> +static void
>> +put_cloned_drop_action(struct ofproto_dpif *ofproto,
>> + struct ofpbuf *odp_actions,
>> + enum xlate_error error,
>> + bool cloned)
>> +{
>> + size_t offset;
>> +
>> + if (!ovs_explicit_drop_action_supported(ofproto)) {
>> + return;
>> + }
>> +
>> + if (cloned) {
>> + offset = nl_msg_start_nested(odp_actions, OVS_ACTION_ATTR_CLONE);
>> + }
>> + nl_msg_put_u32(odp_actions, OVS_ACTION_ATTR_DROP, error);
>> + if (cloned) {
>> + nl_msg_end_nested(odp_actions, offset);
>> + }
>> +}
>> +
>> static void
>> put_ct_helper(struct xlate_ctx *ctx,
>> struct ofpbuf *odp_actions, struct ofpact_conntrack *ofc)
>> diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
>> index cf4e622014b2..718ae69dcfab 100644
>> --- a/tests/tunnel-push-pop.at
>> +++ b/tests/tunnel-push-pop.at
>> @@ -1348,3 +1348,20 @@ AT_CHECK([grep -q "GENEVE_ACT" stdout])
>>
>> OVS_VSWITCHD_STOP
>> AT_CLEANUP
>> +
>> +AT_SETUP([tunnel_push_pop - tunnel drop])
>> +OVS_VSWITCHD_START([dnl
>> + -- add-port br0 t1 -- set Interface t1 type=geneve \
>> + options:remote_ip=1.1.1.1 \
>> + -- add-port br0 p0 -- set Interface p0 type=dummy ofport_request=1 \
>> + -- add-port br0 p1 -- set Interface p1 type=dummy ofport_request=2])
>> +
>> +AT_CHECK([ovs-ofctl add-flow br0 in_port=p0,action=t1,p1])
>> +
>> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
>> 'in_port(1),eth(src=aa:55:aa:55:00:00,dst=f8:bc:12:ff:ff:ff),eth_type(0x0800),ipv4(src=1.1.3.92,dst=1.1.3.88,proto=1,tos=0,ttl=64,frag=no),icmp(type=0,code=0)'],
>> [0], [stdout])
>
> There is no need to specify the full packet here, could just be:
>
> AT_CHECK([ovs-appctl ofproto/trace br0 in_port=p0], [0], [stdout])
>
>> +AT_CHECK([tail -1 stdout], [0],
>> + [Datapath actions: clone(drop),2
>> +])
>> +
>> +OVS_VSWITCHD_STOP
>> +AT_CLEANUP
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
