[ovs-dev] [PATCH v3] datapath: Add a new action dec_ttl

Ilya Maximets i.maximets at ovn.org
Thu May 13 16:36:21 UTC 2021


On 11/24/20 11:43 AM, Eelco Chaudron wrote:
> Add support for the dec_ttl action. Instead of programming the datapath with
> a flow that matches the packet TTL and an IP set, use a single dec_ttl action.
> 
> The old behavior is kept if the new action is not supported by the datapath.
> 
>   # ovs-ofctl dump-flows br0
>    cookie=0x0, duration=12.538s, table=0, n_packets=4, n_bytes=392, ip actions=dec_ttl,NORMAL
>    cookie=0x0, duration=12.536s, table=0, n_packets=4, n_bytes=168, actions=NORMAL
> 
>   # ping -c1 -t 20 192.168.0.2
>   PING 192.168.0.2 (192.168.0.2) 56(84) bytes of data.
>   IP (tos 0x0, ttl 19, id 45336, offset 0, flags [DF], proto ICMP (1), length 84)
>       192.168.0.1 > 192.168.0.2: ICMP echo request, id 8865, seq 1, length 64
> 
> Linux netlink datapath support depends on upstream Linux commit:
>   744676e77720 ("openvswitch: add TTL decrement action")
> 
> 
> Note that in the Linux kernel tree the OVS_ACTION_ATTR_ADD_MPLS has been
> defined, and to make sure the IDs are in sync, it had to be added to the
> OVS source tree. This required some additional case statements, which
> should be revisited once the OVS implementation is added.
> 
> 
> Co-developed-by: Matteo Croce <mcroce at linux.microsoft.com>
> Co-developed-by: Bindiya Kurle <bindiyakurle at gmail.com>
> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
> 
> ---
> v2: - Used definition instead of numeric value in format_dec_ttl_action()
>     - Changed format from "dec_ttl(ttl<=1(<actions>)) to
>       "dec_ttl(le_1(<actions>))" to be more in line with the check_pkt_len action.
>     - Cleaned up format_dec_ttl_action()
> v3:
>     - Fixed parsing of "dec_ttl()" action for adding a dp flow.
>     - Changed implementation to use the fixed kernel mod implementation
>       https://marc.info/?l=linux-netdev&m=160577671609295&w=2
>     - Removed introduced force_last flag from odp_execute_actions
>     - For now, do not use this new attribute if HW offload is supported, as
>       it's causing a performance regression due to HW offload not being
>       supported. I will fix this in a separate patch.
>     - Added datapath test case for dec_ttl action.
> 
>  datapath/linux/compat/include/linux/openvswitch.h |   10 ++
>  lib/dpif-netdev.c                                 |    2 
>  lib/dpif.c                                        |    2 
>  lib/odp-execute.c                                 |   87 +++++++++++++++++++++
>  lib/odp-util.c                                    |   45 +++++++++++
>  lib/packets.h                                     |   13 +++
>  ofproto/ofproto-dpif-ipfix.c                      |    2 
>  ofproto/ofproto-dpif-sflow.c                      |    2 
>  ofproto/ofproto-dpif-xlate.c                      |   60 ++++++++++++--
>  ofproto/ofproto-dpif.c                            |   40 ++++++++++
>  ofproto/ofproto-dpif.h                            |    6 +
>  tests/system-traffic.at                           |   28 +++++++
>  12 files changed, 282 insertions(+), 15 deletions(-)
> 
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
> index 2d884312f..3016576fe 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -1021,6 +1021,8 @@ enum ovs_action_attr {
>  	OVS_ACTION_ATTR_METER,        /* u32 meter number. */
>  	OVS_ACTION_ATTR_CLONE,        /* Nested OVS_CLONE_ATTR_*.  */
>  	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> +	OVS_ACTION_ATTR_ADD_MPLS,      /* struct ovs_action_add_mpls. */
> +	OVS_ACTION_ATTR_DEC_TTL,       /* Nested OVS_DEC_TTL_ATTR_*. */
>  
>  #ifndef __KERNEL__
>  	OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
> @@ -1124,4 +1126,12 @@ struct ovs_zone_limit {
>  				      * keys. False otherwise.
>  				      */
>  
> +enum ovs_dec_ttl_attr {
> +	OVS_DEC_TTL_ATTR_UNSPEC,
> +	OVS_DEC_TTL_ATTR_ACTION,        /* Nested struct nlattr */
> +	__OVS_DEC_TTL_ATTR_MAX
> + };

Here is an extra space before '}'.

> +
> +#define OVS_DEC_TTL_ATTR_MAX (__OVS_DEC_TTL_ATTR_MAX - 1)
> +
>  #endif /* _LINUX_OPENVSWITCH_H */
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 300861ca5..b6e313304 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -7975,6 +7975,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>      case OVS_ACTION_ATTR_CT_CLEAR:
>      case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>      case OVS_ACTION_ATTR_DROP:
> +    case OVS_ACTION_ATTR_DEC_TTL:
> +    case OVS_ACTION_ATTR_ADD_MPLS:
>      case __OVS_ACTION_ATTR_MAX:
>          OVS_NOT_REACHED();
>      }
> diff --git a/lib/dpif.c b/lib/dpif.c
> index ac2860764..f87afd2f5 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1273,6 +1273,8 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
>      case OVS_ACTION_ATTR_UNSPEC:
>      case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>      case OVS_ACTION_ATTR_DROP:
> +    case OVS_ACTION_ATTR_ADD_MPLS:
> +    case OVS_ACTION_ATTR_DEC_TTL:
>      case __OVS_ACTION_ATTR_MAX:
>          OVS_NOT_REACHED();
>      }
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 6eeda2a61..e1001800f 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -719,6 +719,55 @@ odp_execute_sample(void *dp, struct dp_packet *packet, bool steal,
>                          nl_attr_get_size(subactions), dp_execute_action);
>  }
>  
> +static bool execute_dec_ttl(struct dp_packet *packet)

Function name should start on a next line.

> +{
> +    struct eth_header *eth = dp_packet_eth(packet);
> +
> +    if (dl_type_is_ipv4(eth->eth_type)) {
> +        struct ip_header *nh = dp_packet_l3(packet);
> +        uint8_t old_ttl = nh->ip_ttl;
> +
> +        if (old_ttl <= 1) {
> +            return true;
> +        }
> +
> +        nh->ip_ttl--;
> +        nh->ip_csum = recalc_csum16(nh->ip_csum, htons(old_ttl << 8),
> +                                    htons(nh->ip_ttl << 8));
> +    } else if (dl_type_is_ipv6(eth->eth_type)) {
> +        struct ovs_16aligned_ip6_hdr *nh = dp_packet_l3(packet);
> +
> +        if (nh->ip6_hlim <= 1) {
> +            return true;
> +        }
> +
> +        nh->ip6_hlim--;
> +    }
> +
> +    return false;
> +}
> +
> +static void odp_dec_ttl_exception_handler(void *dp,
> +                                          struct dp_packet_batch *batch,
> +                                          const struct nlattr *action,
> +                                          odp_execute_cb dp_execute_action)

Function name should start on a next line.

> +{
> +    static const struct nl_policy dec_ttl_policy[] = {
> +        [OVS_DEC_TTL_ATTR_ACTION] = { .type = NL_A_NESTED },
> +    };
> +    struct nlattr *attrs[ARRAY_SIZE(dec_ttl_policy)];
> +
> +    if (!nl_parse_nested(action, dec_ttl_policy, attrs, ARRAY_SIZE(attrs)) ||
> +        !attrs[OVS_DEC_TTL_ATTR_ACTION]) {
> +        OVS_NOT_REACHED();

This should, probably, be an ovs_assert() instead to have some clue in logs
if this happened for some reason.

> +    }
> +
> +    odp_execute_actions(dp, batch, true,
> +                        nl_attr_get(attrs[OVS_DEC_TTL_ATTR_ACTION]),
> +                        nl_attr_get_size(attrs[OVS_DEC_TTL_ATTR_ACTION]),
> +                        dp_execute_action);
> +}
> +
>  static void
>  odp_execute_clone(void *dp, struct dp_packet_batch *batch, bool steal,
>                     const struct nlattr *actions,
> @@ -734,7 +783,7 @@ odp_execute_clone(void *dp, struct dp_packet_batch *batch, bool steal,
>          dp_packet_batch_clone(&clone_pkt_batch, batch);
>          dp_packet_batch_reset_cutlen(batch);
>          odp_execute_actions(dp, &clone_pkt_batch, true, nl_attr_get(actions),
> -                        nl_attr_get_size(actions), dp_execute_action);
> +                            nl_attr_get_size(actions), dp_execute_action);
>      }
>      else {
>          odp_execute_actions(dp, batch, true, nl_attr_get(actions),
> @@ -820,6 +869,8 @@ requires_datapath_assistance(const struct nlattr *a)
>      case OVS_ACTION_ATTR_CT_CLEAR:
>      case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>      case OVS_ACTION_ATTR_DROP:
> +    case OVS_ACTION_ATTR_ADD_MPLS:
> +    case OVS_ACTION_ATTR_DEC_TTL:
>          return false;
>  
>      case OVS_ACTION_ATTR_UNSPEC:
> @@ -979,6 +1030,39 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
>              }
>              break;
>  
> +        case OVS_ACTION_ATTR_DEC_TTL: {
> +            const size_t cnt = dp_packet_batch_size(batch);
> +            struct dp_packet_batch invalid_ttl;
> +            size_t i;
> +
> +            /* Make batch for invalid ttl packets. */
> +            dp_packet_batch_init(&invalid_ttl);
> +            invalid_ttl.trunc = batch->trunc;
> +            invalid_ttl.do_not_steal = batch->do_not_steal;
> +
> +            /* Add packets with ttl <=1 to the invalid_ttl batch
> +             * and remove it from the batch. */
> +            DP_PACKET_BATCH_REFILL_FOR_EACH (i, cnt, packet, batch) {
> +                if (execute_dec_ttl(packet)) {
> +                    dp_packet_batch_add(&invalid_ttl, packet);
> +                } else {
> +                    dp_packet_batch_refill(batch, packet, i);
> +                }
> +            }
> +
> +            /* Execute action on packets with ttl <= 1. */
> +            if (invalid_ttl.count > 0) {

dp_packet_batch_size(&invalid_ttl)

> +                odp_dec_ttl_exception_handler(dp, &invalid_ttl, a,
> +                                              dp_execute_action);
> +            }
> +
> +            if (last_action || !batch->count) {

dp_packet_batch_is_empty(batch)

> +                /* We do not need to free the packets. */

Why exactly we do not need to free the packets?
If for any reason dec_ttl will be the last action in the list we
will just leak all the packets here.  We, probbaly, should not have
this 'if' condition at all.

> +                return;
> +            }
> +            break;
> +        }
> +
>          case OVS_ACTION_ATTR_TRUNC: {
>              const struct ovs_action_trunc *trunc =
>                          nl_attr_get_unspec(a, sizeof *trunc);
> @@ -1077,6 +1161,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
>          case OVS_ACTION_ATTR_RECIRC:
>          case OVS_ACTION_ATTR_CT:
>          case OVS_ACTION_ATTR_UNSPEC:
> +        case OVS_ACTION_ATTR_ADD_MPLS:
>          case __OVS_ACTION_ATTR_MAX:
>              OVS_NOT_REACHED();
>          }
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 0bd2f9aa8..f5a619c36 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -143,8 +143,10 @@ odp_action_len(uint16_t type)
>      case OVS_ACTION_ATTR_POP_NSH: return 0;
>      case OVS_ACTION_ATTR_CHECK_PKT_LEN: return ATTR_LEN_VARIABLE;
>      case OVS_ACTION_ATTR_DROP: return sizeof(uint32_t);
> +    case OVS_ACTION_ATTR_DEC_TTL: return ATTR_LEN_VARIABLE;
>  
>      case OVS_ACTION_ATTR_UNSPEC:
> +    case OVS_ACTION_ATTR_ADD_MPLS:
>      case __OVS_ACTION_ATTR_MAX:
>          return ATTR_LEN_INVALID;
>      }
> @@ -1109,6 +1111,25 @@ format_odp_check_pkt_len_action(struct ds *ds, const struct nlattr *attr,
>      ds_put_cstr(ds, "))");
>  }
>  
> +static void
> +format_dec_ttl_action(struct ds *ds, const struct nlattr *attr,
> +                      const struct hmap *portno_names)
> +{
> +    const struct nlattr *a;
> +    unsigned int left;
> +
> +    ds_put_cstr(ds,"dec_ttl(le_1(");
> +    NL_ATTR_FOR_EACH (a, left,
> +                      nl_attr_get(attr), nl_attr_get_size(attr)) {
> +        if (nl_attr_type(a) == OVS_DEC_TTL_ATTR_ACTION) {
> +           format_odp_actions(ds, nl_attr_get(a),
> +                              nl_attr_get_size(a), portno_names);
> +           break;
> +        }
> +    }
> +    ds_put_format(ds, "))");
> +}
> +
>  static void
>  format_odp_action(struct ds *ds, const struct nlattr *a,
>                    const struct hmap *portno_names)
> @@ -1256,7 +1277,11 @@ format_odp_action(struct ds *ds, const struct nlattr *a,
>      case OVS_ACTION_ATTR_DROP:
>          ds_put_cstr(ds, "drop");
>          break;
> +    case OVS_ACTION_ATTR_DEC_TTL:
> +        format_dec_ttl_action(ds, a, portno_names);
> +        break;
>      case OVS_ACTION_ATTR_UNSPEC:
> +    case OVS_ACTION_ATTR_ADD_MPLS:
>      case __OVS_ACTION_ATTR_MAX:
>      default:
>          format_generic_odp_action(ds, a);
> @@ -2498,6 +2523,26 @@ parse_odp_action__(struct parse_odp_context *context, const char *s,
>              return n + 1;
>          }
>      }
> +    {
> +        if (!strncmp(s, "dec_ttl(le_1(", 13)) {
> +            size_t actions_ofs, ofs;
> +            int n = 13;
> +
> +            ofs = nl_msg_start_nested(actions, OVS_ACTION_ATTR_DEC_TTL);
> +            actions_ofs = nl_msg_start_nested(actions,
> +                                              OVS_DEC_TTL_ATTR_ACTION);
> +
> +            int retval = parse_action_list(context, s + n, actions);
> +            if (retval < 0) {
> +                return retval;
> +            }
> +
> +            n += retval;
> +            nl_msg_end_nested(actions, actions_ofs);
> +            nl_msg_end_nested(actions, ofs);
> +            return n + 2;
> +        }
> +    }
>  
>      {
>          if (!strncmp(s, "push_nsh(", 9)) {
> diff --git a/lib/packets.h b/lib/packets.h
> index 481bc22fa..b7fa7b121 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -1211,10 +1211,19 @@ bool in6_is_lla(struct in6_addr *addr);
>  void ipv6_multicast_to_ethernet(struct eth_addr *eth,
>                                  const struct in6_addr *ip6);
>  
> +static inline bool dl_type_is_ipv4(ovs_be16 dl_type)
> +{
> +    return dl_type == htons(ETH_TYPE_IP);
> +}
> +
> +static inline bool dl_type_is_ipv6(ovs_be16 dl_type)
> +{
> +    return dl_type == htons(ETH_TYPE_IPV6);
> +}
> +
>  static inline bool dl_type_is_ip_any(ovs_be16 dl_type)
>  {
> -    return dl_type == htons(ETH_TYPE_IP)
> -        || dl_type == htons(ETH_TYPE_IPV6);
> +    return dl_type_is_ipv4(dl_type) || dl_type_is_ipv6(dl_type);
>  }
>  
>  /* Tunnel header */
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index 796eb6f88..1c8e7ae26 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -3018,6 +3018,8 @@ dpif_ipfix_read_actions(const struct flow *flow,
>          case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>          case OVS_ACTION_ATTR_UNSPEC:
>          case OVS_ACTION_ATTR_DROP:
> +        case OVS_ACTION_ATTR_ADD_MPLS:
> +        case OVS_ACTION_ATTR_DEC_TTL:
>          case __OVS_ACTION_ATTR_MAX:
>          default:
>              break;
> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
> index fdcb9eabb..c8bf6bee4 100644
> --- a/ofproto/ofproto-dpif-sflow.c
> +++ b/ofproto/ofproto-dpif-sflow.c
> @@ -1178,6 +1178,7 @@ dpif_sflow_read_actions(const struct flow *flow,
>          case OVS_ACTION_ATTR_CT_CLEAR:
>          case OVS_ACTION_ATTR_METER:
>          case OVS_ACTION_ATTR_LB_OUTPUT:
> +        case OVS_ACTION_ATTR_DEC_TTL:
>              break;
>  
>          case OVS_ACTION_ATTR_SET_MASKED:
> @@ -1226,6 +1227,7 @@ dpif_sflow_read_actions(const struct flow *flow,
>          case OVS_ACTION_ATTR_UNSPEC:
>          case OVS_ACTION_ATTR_CHECK_PKT_LEN:
>          case OVS_ACTION_ATTR_DROP:
> +        case OVS_ACTION_ATTR_ADD_MPLS:
>          case __OVS_ACTION_ATTR_MAX:
>          default:
>              break;
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 11aa20754..d5675c9bd 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -40,6 +40,7 @@
>  #include "mac-learning.h"
>  #include "mcast-snooping.h"
>  #include "multipath.h"
> +#include "netdev-offload.h"
>  #include "netdev-vport.h"
>  #include "netlink.h"
>  #include "nx-match.h"
> @@ -4839,9 +4840,12 @@ xlate_controller_action(struct xlate_ctx *ctx, int len,
>                          enum ofp_packet_in_reason reason,
>                          uint16_t controller_id,
>                          uint32_t provider_meter_id,
> -                        const uint8_t *userdata, size_t userdata_len)
> +                        const uint8_t *userdata, size_t userdata_len,
> +                        bool commit)
>  {
> -    xlate_commit_actions(ctx);
> +    if (commit) {
> +        xlate_commit_actions(ctx);
> +    }
>  
>      /* A packet sent by an action in a table-miss rule is considered an
>       * explicit table miss.  OpenFlow before 1.3 doesn't have that concept so
> @@ -5087,7 +5091,8 @@ compose_dec_ttl(struct xlate_ctx *ctx, struct ofpact_cnt_ids *ids)
>  
>          for (i = 0; i < ids->n_controllers; i++) {
>              xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL,
> -                                    ids->cnt_ids[i], UINT32_MAX, NULL, 0);
> +                                    ids->cnt_ids[i], UINT32_MAX, NULL, 0,
> +                                    true);
>          }
>  
>          /* Stop processing for current table. */
> @@ -5128,7 +5133,7 @@ compose_dec_nsh_ttl_action(struct xlate_ctx *ctx)
>              return false;
>          } else {
>              xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL,
> -                                    0, UINT32_MAX, NULL, 0);
> +                                    0, UINT32_MAX, NULL, 0, true);
>          }
>      }
>  
> @@ -5161,7 +5166,7 @@ compose_dec_mpls_ttl_action(struct xlate_ctx *ctx)
>              return false;
>          } else {
>              xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL, 0,
> -                                    UINT32_MAX, NULL, 0);
> +                                    UINT32_MAX, NULL, 0, true);
>          }
>      }
>  
> @@ -5185,6 +5190,42 @@ xlate_delete_field(struct xlate_ctx *ctx,
>      ds_destroy(&s);
>  }
>  
> +/* New handling for dec_ttl action. */
> +static void
> +xlate_dec_ttl_action(struct xlate_ctx *ctx, struct ofpact_cnt_ids *ids)
> +{
> +    struct flow *flow = &ctx->xin->flow;
> +    struct flow_wildcards *wc = ctx->wc;
> +    size_t offset, offset_actions;
> +    size_t i;
> +
> +    if (!is_ip_any(flow)) {
> +        return;
> +    }
> +
> +    if (!ctx->xbridge->support.dec_ttl_action
> +        || netdev_is_flow_api_enabled()) {
> +        wc->masks.nw_ttl = 0xff;
> +        compose_dec_ttl(ctx, ids);

compose_dec_ttl() returns boolean value that indicates if translation
should proceed or not, but the value just ignored here.
We should return the result from this function and use it in the caller.

> +        return;
> +    }
> +
> +    xlate_commit_actions(ctx);
> +    offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_DEC_TTL);
> +    offset_actions = nl_msg_start_nested(ctx->odp_actions,
> +                                         OVS_DEC_TTL_ATTR_ACTION);
> +
> +    for (i = 0; i < ids->n_controllers; i++) {
> +        xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL,
> +                                ids->cnt_ids[i], UINT32_MAX, NULL, 0, false);
> +    }
> +
> +    nl_msg_end_nested(ctx->odp_actions, offset_actions);
> +    nl_msg_end_nested(ctx->odp_actions, offset);
> +
> +    xlate_commit_actions(ctx);
> +}
> +
>  /* Emits an action that outputs to 'port', within 'ctx'.
>   *
>   * 'controller_len' affects only packets sent to an OpenFlow controller.  It
> @@ -5235,7 +5276,7 @@ xlate_output_action(struct xlate_ctx *ctx, ofp_port_t port,
>                                   : group_bucket_action ? OFPR_GROUP
>                                   : ctx->in_action_set ? OFPR_ACTION_SET
>                                   : OFPR_ACTION),
> -                                0, UINT32_MAX, NULL, 0);
> +                                0, UINT32_MAX, NULL, 0, true);
>          break;
>      case OFPP_NONE:
>          break;
> @@ -6796,7 +6837,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>                                          controller->controller_id,
>                                          controller->provider_meter_id,
>                                          controller->userdata,
> -                                        controller->userdata_len);
> +                                        controller->userdata_len, true);
>              }
>              break;
>  
> @@ -7004,10 +7045,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>              break;
>  
>          case OFPACT_DEC_TTL:
> -            wc->masks.nw_ttl = 0xff;
> -            if (compose_dec_ttl(ctx, ofpact_get_DEC_TTL(a))) {
> -                return;
> -            }
> +            xlate_dec_ttl_action(ctx, ofpact_get_DEC_TTL(a));

Here we need to 'return' if xlate_dec_ttl_action() returns true.

>              break;
>  
>          case OFPACT_NOTE:
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index fd0b2fdea..49e026359 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1186,6 +1186,45 @@ check_trunc_action(struct dpif_backer *backer)
>      return !error;
>  }
>  
> +/* Tests whether 'dpif' datapath supports decrement of the IP TTL via
> + * OVS_ACTION_DEC_TTL. */
> +static bool
> +check_dec_ttl_action(struct dpif *dpif)
> +{
> +    struct odputil_keybuf keybuf;
> +    struct flow flow = { 0 };
> +    struct ofpbuf actions;
> +    uint32_t actbuf[4];
> +    struct ofpbuf key;
> +    bool supported;
> +    size_t start;
> +
> +    struct odp_flow_key_parms odp_parms = {
> +        .flow = &flow,
> +        .probe = true,
> +    };
> +
> +    ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> +    odp_flow_key_from_flow(&odp_parms, &key);
> +
> +    ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf);
> +    start = nl_msg_start_nested(&actions, OVS_ACTION_ATTR_DEC_TTL);
> +    nl_msg_put_flag(&actions, OVS_DEC_TTL_ATTR_ACTION);
> +    nl_msg_end_nested(&actions, start);
> +
> +    supported = dpif_probe_feature(dpif, "dec_ttl", &key, &actions, NULL);
> +
> +    if (supported) {
> +        VLOG_INFO("%s: Datapath supports dec_ttl action",
> +                  dpif_name(dpif));
> +    } else {
> +        VLOG_INFO("%s: Datapath does not support dec_ttl action",
> +                  dpif_name(dpif));
> +    }
> +
> +    return supported;
> +}
> +
>  /* Tests whether 'backer''s datapath supports the clone action
>   * OVS_ACTION_ATTR_CLONE.   */
>  static bool
> @@ -1590,6 +1629,7 @@ check_support(struct dpif_backer *backer)
>          dpif_supports_explicit_drop_action(backer->dpif);
>      backer->rt_support.lb_output_action=
>          dpif_supports_lb_output_action(backer->dpif);
> +    backer->rt_support.dec_ttl_action = check_dec_ttl_action(backer->dpif);
>  
>      /* Flow fields. */
>      backer->rt_support.odp.ct_state = check_ct_state(backer);
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index b41c3d82a..f4d1152bc 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -204,7 +204,11 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *,
>      DPIF_SUPPORT_FIELD(bool, explicit_drop_action, "Explicit Drop action")  \
>                                                                              \
>      /* True if the datapath supports balance_tcp optimization */            \
> -    DPIF_SUPPORT_FIELD(bool, lb_output_action, "Optimized Balance TCP mode")
> +    DPIF_SUPPORT_FIELD(bool, lb_output_action, "Optimized Balance TCP mode")\
> +                                                                            \
> +    /* True if the datapath supports dec_ttl action. */                     \
> +    DPIF_SUPPORT_FIELD(bool, dec_ttl_action, "Decrement TTL action")
> +
>  
>  
>  /* Stores the various features which the corresponding backer supports. */
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index 14f349b5b..ee6b875e7 100644
> --- a/tests/system-traffic.at
> +++ b/tests/system-traffic.at
> @@ -1400,6 +1400,34 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip], [0], [dnl
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>  
> +AT_SETUP([datapath - dec_ttl action])
> +OVS_TRAFFIC_VSWITCHD_START()
> +
> +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"])
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +
> +AT_CHECK([ovs-vsctl -- set interface ovs-p0 ofport_request=1 \
> +                    -- set interface ovs-p1 ofport_request=2])
> +
> +AT_CHECK([ovs-ofctl del-flows br0])
> +AT_CHECK([ovs-ofctl add-flow br0 in_port=1,action=dec_ttl,2])
> +AT_CHECK([ovs-ofctl add-flow br0 in_port=2,action=dec_ttl,1])
> +
> +NS_CHECK_EXEC([at_ns0], [ping -c 1 -w 2 10.1.1.2 | grep -o -E 'ttl=[[[:digit:]]]+'], [0], [dnl
> +ttl=63
> +])
> +
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 -t 1 10.1.1.2 | FORMAT_PING], [0], [dnl
> +1 packets transmitted, 0 received, 100% packet loss, time 0ms
> +])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  AT_BANNER([conntrack])
>  
>  AT_SETUP([conntrack - controller])
> 

Since you're ading a new datapath action, tests for tests/odp.at
are required.

On a general note, OVS doesn't use Co-develped-by tag, we're using
Co-authored-by instead.  And co-authors needs to Sign-off.

Best regards, Ilya Maximets.


More information about the dev mailing list