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