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

Ilya Maximets i.maximets at ovn.org
Tue May 18 18:15:16 UTC 2021


On 5/18/21 4:44 PM, 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-authored-by: Bindiya Kurle <bindiyakurle at gmail.com>
> Co-authored-by: Matteo Croce <mcroce at linux.microsoft.com>
> Signed-off-by: Eelco Chaudron <echaudro at redhat.com>
> Signed-off-by: Bindiya Kurle <bindiyakurle at gmail.com>
> Signed-off-by: Matteo Croce <mcroce at linux.microsoft.com>
> Acked-by: Bindiya Kurle <bindiyakurle at gmail.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.
> v4:
>     - Fixed some style issues reported by Ilya.
>     - Changed OVS_NOT_REACHED() to an ovs_assert(), so information gets logged
>       on error.
>     - Use batch helper functions rather than access batch->count directly.
>     - Fixed memory leak when dec_ttl was the last action in the chain.
>     - Fixed do_xlate_actions() return if for exception in backward
>       compatibility mode.
>     - Added actions parsing test to odp.at
> 
> datapath/linux/compat/include/linux/openvswitch.h |   10 ++
>  lib/dpif-netdev.c                                 |    2 
>  lib/dpif.c                                        |    2 
>  lib/odp-execute.c                                 |   88 +++++++++++++++++++++
>  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/odp.at                                      |    1 
>  tests/system-traffic.at                           |   28 +++++++
>  13 files changed, 282 insertions(+), 17 deletions(-)
> 
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
> index 875de2025..a5357024d 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -1030,6 +1030,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*/
> @@ -1133,4 +1135,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
> +};
> +
> +#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 251788b04..c6032e013 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -8046,6 +8046,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 26e8bfb7d..a25d8e9ef 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1274,6 +1274,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..ff2921045 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)
> +{
> +    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)
> +{
> +    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)];
> +
> +    ovs_assert(nl_parse_nested(action, dec_ttl_policy, attrs,
> +                               ARRAY_SIZE(attrs))
> +               && attrs[OVS_DEC_TTL_ATTR_ACTION]);
> +
> +    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,40 @@ 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 (!dp_packet_batch_is_empty(&invalid_ttl)) {
> +                odp_dec_ttl_exception_handler(dp, &invalid_ttl, a,
> +                                              dp_execute_action);
> +            }
> +
> +            if (dp_packet_batch_is_empty(batch)) {
> +                /* The entire batch was handled as an exception, so we can
> +                 * stop processing actions. */
> +                return;
> +            }
> +            break;
> +        }
> +
>          case OVS_ACTION_ATTR_TRUNC: {
>              const struct ovs_action_trunc *trunc =
>                          nl_attr_get_unspec(a, sizeof *trunc);
> @@ -1077,6 +1162,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 e1199d1da..922104e72 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;
>      }
> @@ -1110,6 +1112,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)
> @@ -1257,7 +1278,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);
> @@ -2505,6 +2530,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 864c136b5..83989fb90 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 7108c8a30..3cac92940 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"
> @@ -4840,9 +4841,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
> @@ -5075,10 +5079,6 @@ compose_dec_ttl(struct xlate_ctx *ctx, struct ofpact_cnt_ids *ids)
>  {
>      struct flow *flow = &ctx->xin->flow;
>  
> -    if (!is_ip_any(flow)) {
> -        return false;
> -    }
> -
>      ctx->wc->masks.nw_ttl = 0xff;
>      if (flow->nw_ttl > 1) {
>          flow->nw_ttl--;
> @@ -5088,7 +5088,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. */
> @@ -5129,7 +5130,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);
>          }
>      }
>  
> @@ -5162,7 +5163,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);
>          }
>      }
>  
> @@ -5186,6 +5187,40 @@ xlate_delete_field(struct xlate_ctx *ctx,
>      ds_destroy(&s);
>  }
>  
> +/* New handling for dec_ttl action. */

'New handling' makes sense in a patch, but doesn't while reading the code.

> +static bool
> +xlate_dec_ttl_action(struct xlate_ctx *ctx, struct ofpact_cnt_ids *ids)
> +{
> +    struct flow *flow = &ctx->xin->flow;
> +    size_t offset, offset_actions;
> +    size_t i;
> +
> +    if (!is_ip_any(flow)) {
> +        return false;
> +    }
> +
> +    if (!ctx->xbridge->support.dec_ttl_action
> +        || netdev_is_flow_api_enabled()) {
> +        return compose_dec_ttl(ctx, ids);
> +    }
> +
> +    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);
> +    return false;
> +}
> +
>  /* Emits an action that outputs to 'port', within 'ctx'.
>   *
>   * 'controller_len' affects only packets sent to an OpenFlow controller.  It
> @@ -5236,7 +5271,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;
> @@ -6797,7 +6832,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;
>  
> @@ -7005,8 +7040,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))) {
> +            if (xlate_dec_ttl_action(ctx, ofpact_get_DEC_TTL(a))) {
>                  return;
>              }
>              break;
> 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 };

It's probbaly better to just memset it as in other similar functions
to avoid compiler's complains.

> +    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);

During discussions about all-zero SNAT feature support I remembered that
we have a 'capabilities' table that should reflect all the datapath
supported fetures.  So, this should be added there as well.  And documented
in vswitchd/vswitch.xml.

>  
>      /* 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/odp.at b/tests/odp.at
> index b762ebb2b..24946bec4 100644
> --- a/tests/odp.at
> +++ b/tests/odp.at
> @@ -384,6 +384,7 @@ check_pkt_len(size=200,gt(drop),le(5))
>  check_pkt_len(size=200,gt(ct(nat)),le(drop))
>  check_pkt_len(size=200,gt(set(eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15))),le(set(eth(src=00:01:02:03:04:06,dst=10:11:12:13:14:16))))
>  lb_output(1)
> +dec_ttl(le_1(userspace(pid=3614533484,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535))))

Maybe it will make sense to also add a very simple variant of this action,
e.g. dec_ttl(le_1(drop)).

>  ])
>  AT_CHECK_UNQUOTED([ovstest test-odp parse-actions < actions.txt], [0],
>    [`cat actions.txt`
> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> index fb5b9a36d..ef9b555b6 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])
> 



More information about the dev mailing list