[ovs-dev] [PATCH v3 1/3] dpif: Add support for OVS_ACTION_ATTR_CT_CLEAR

Flavio Leitner fbl at sysclose.org
Mon Nov 20 13:59:10 UTC 2017


On Sat, 18 Nov 2017 19:41:44 -0500
Eric Garver <e at erig.me> wrote:
> This supports using the ct_clear action in the kernel datapath. To
> preserve compatibility with current ct_clear behavior on old kernels, we
> only pass this action down to the datapath if a probe reveals the
> datapath actually supports it.
> 
> Signed-off-by: Eric Garver <e at erig.me>
> Acked-by: William Tu <u9012063 at gmail.com>
> ---
>  NEWS                                              |  3 +++
>  datapath/linux/compat/include/linux/openvswitch.h |  2 ++
>  lib/conntrack.c                                   | 10 +++++++
>  lib/conntrack.h                                   |  1 +
>  lib/dpif-netdev.c                                 |  1 +
>  lib/dpif.c                                        |  1 +
>  lib/odp-execute.c                                 |  7 +++++
>  lib/odp-util.c                                    | 11 ++++++++
>  lib/ofp-actions.c                                 |  1 +
>  ofproto/ofproto-dpif-ipfix.c                      |  1 +
>  ofproto/ofproto-dpif-sflow.c                      |  1 +
>  ofproto/ofproto-dpif-xlate.c                      | 14 +++++++++-
>  ofproto/ofproto-dpif.c                            | 32 +++++++++++++++++++++++
>  ofproto/ofproto-dpif.h                            |  5 +++-
>  tests/odp.at                                      |  1 +
>  15 files changed, 89 insertions(+), 2 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index a93237f9078b..41123aab682e 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -12,6 +12,9 @@ Post-v2.8.0
>           IPv6 packets.
>     - Linux kernel 4.13
>       * Add support for compiling OVS with the latest Linux 4.13 kernel
> +   - OpenFlow:
> +     * ct_clear action is now backed by kernel datapath. Support is probed for
> +       when OVS starts.
>  
>  v2.8.0 - 31 Aug 2017
>  --------------------
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
> index 561f8950277d..4874d5065e0a 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -887,6 +887,7 @@ enum ovs_nat_attr {
>   * @OVS_ACTION_ATTR_PUSH_ETH: Push a new outermost Ethernet header onto the
>   * packet.
>   * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the packet.
> + * @OVS_ACTION_ATTR_CT_CLEAR: Clear conntrack state from the packet.
>   * @OVS_ACTION_ATTR_ENCAP_NSH: encap NSH action to push NSH header.
>   * @OVS_ACTION_ATTR_DECAP_NSH: decap NSH action to remove NSH header.
>   *
> @@ -924,6 +925,7 @@ enum ovs_action_attr {
>  	OVS_ACTION_ATTR_TRUNC,        /* u32 struct ovs_action_trunc. */
>  	OVS_ACTION_ATTR_PUSH_ETH,     /* struct ovs_action_push_eth. */
>  	OVS_ACTION_ATTR_POP_ETH,      /* No argument. */
> +	OVS_ACTION_ATTR_CT_CLEAR,     /* No argument. */
>  
>  #ifndef __KERNEL__
>  	OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index f5a3aa9fab34..307902cea413 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -1242,6 +1242,16 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
>      return 0;
>  }
>  
> +int
> +conntrack_clear(struct dp_packet *packet)
> +{
> +    /* According to pkt_metadata_init(), ct_state == 0 is enough to make all of
> +     * the conntrack fields invalid. */
> +    packet->md.ct_state = 0;
> +
> +    return 0;
> +}
> +
>  static void
>  set_mark(struct dp_packet *pkt, struct conn *conn, uint32_t val, uint32_t mask)
>  {
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index fbeef1c4754e..6c19f3c65804 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -97,6 +97,7 @@ int conntrack_execute(struct conntrack *, struct dp_packet_batch *,
>                        const char *helper,
>                        const struct nat_action_info_t *nat_action_info,
>                        long long now);
> +int conntrack_clear(struct dp_packet *packet);
>  
>  struct conntrack_dump {
>      struct conntrack *ct;
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index db7831874fed..e017368d6dae 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -5660,6 +5660,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>      case OVS_ACTION_ATTR_CLONE:
>      case OVS_ACTION_ATTR_ENCAP_NSH:
>      case OVS_ACTION_ATTR_DECAP_NSH:
> +    case OVS_ACTION_ATTR_CT_CLEAR:
>      case __OVS_ACTION_ATTR_MAX:
>          OVS_NOT_REACHED();
>      }
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 3f0742d382ed..7bc4f9b3dc32 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1275,6 +1275,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
>      case OVS_ACTION_ATTR_CLONE:
>      case OVS_ACTION_ATTR_ENCAP_NSH:
>      case OVS_ACTION_ATTR_DECAP_NSH:
> +    case OVS_ACTION_ATTR_CT_CLEAR:
>      case OVS_ACTION_ATTR_UNSPEC:
>      case __OVS_ACTION_ATTR_MAX:
>          OVS_NOT_REACHED();
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 30114795584e..9fd084c1d2e7 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -34,6 +34,7 @@
>  #include "unaligned.h"
>  #include "util.h"
>  #include "csum.h"
> +#include "conntrack.h"
>  
>  /* Masked copy of an ethernet address. 'src' is already properly masked. */
>  static void
> @@ -655,6 +656,7 @@ requires_datapath_assistance(const struct nlattr *a)
>      case OVS_ACTION_ATTR_CLONE:
>      case OVS_ACTION_ATTR_ENCAP_NSH:
>      case OVS_ACTION_ATTR_DECAP_NSH:
> +    case OVS_ACTION_ATTR_CT_CLEAR:
>          return false;
>  
>      case OVS_ACTION_ATTR_UNSPEC:
> @@ -839,6 +841,11 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
>              }
>              break;
>          }
> +        case OVS_ACTION_ATTR_CT_CLEAR:
> +            DP_PACKET_BATCH_FOR_EACH (packet, batch) {
> +                conntrack_clear(packet);
> +            }
> +            break;
>  
>          case OVS_ACTION_ATTR_OUTPUT:
>          case OVS_ACTION_ATTR_TUNNEL_PUSH:
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index b348ab680d9f..d989f9e1d9fb 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -117,6 +117,7 @@ odp_action_len(uint16_t type)
>      case OVS_ACTION_ATTR_SET_MASKED: return ATTR_LEN_VARIABLE;
>      case OVS_ACTION_ATTR_SAMPLE: return ATTR_LEN_VARIABLE;
>      case OVS_ACTION_ATTR_CT: return ATTR_LEN_VARIABLE;
> +    case OVS_ACTION_ATTR_CT_CLEAR: return 0;
>      case OVS_ACTION_ATTR_PUSH_ETH: return sizeof(struct ovs_action_push_eth);
>      case OVS_ACTION_ATTR_POP_ETH: return 0;
>      case OVS_ACTION_ATTR_CLONE: return ATTR_LEN_VARIABLE;
> @@ -1045,6 +1046,9 @@ format_odp_action(struct ds *ds, const struct nlattr *a,
>      case OVS_ACTION_ATTR_CT:
>          format_odp_conntrack_action(ds, a);
>          break;
> +    case OVS_ACTION_ATTR_CT_CLEAR:
> +        ds_put_cstr(ds, "ct_clear");
> +        break;
>      case OVS_ACTION_ATTR_CLONE:
>          format_odp_clone_action(ds, a, portno_names);
>          break;
> @@ -2122,6 +2126,13 @@ parse_odp_action(const char *s, const struct simap *port_names,
>      }
>  
>      {
> +        if (!strncmp(s, "ct_clear", 8)) {
> +            nl_msg_put_flag(actions, OVS_ACTION_ATTR_CT_CLEAR);
> +            return 8;
> +        }
> +    }
> +
> +    {
>          int retval;
>  
>          retval = parse_conntrack_action(s, actions);
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index e5d03e155543..a8f861e513f9 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -7315,6 +7315,7 @@ ofpacts_execute_action_set(struct ofpbuf *action_list,
>      if (!ofpacts_copy_last(action_list, action_set, OFPACT_GROUP) &&
>          !ofpacts_copy_last(action_list, action_set, OFPACT_OUTPUT) &&
>          !ofpacts_copy_last(action_list, action_set, OFPACT_RESUBMIT) &&
> +        !ofpacts_copy_last(action_list, action_set, OFPACT_CT_CLEAR) &&
>          !ofpacts_copy_last(action_list, action_set, OFPACT_CT)) {
>          ofpbuf_clear(action_list);
>      }
> diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> index 4d168784beb9..4fba247dd816 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -2813,6 +2813,7 @@ dpif_ipfix_read_actions(const struct flow *flow,
>          case OVS_ACTION_ATTR_TRUNC:
>          case OVS_ACTION_ATTR_HASH:
>          case OVS_ACTION_ATTR_CT:
> +        case OVS_ACTION_ATTR_CT_CLEAR:
>          case OVS_ACTION_ATTR_METER:
>          case OVS_ACTION_ATTR_SET_MASKED:
>          case OVS_ACTION_ATTR_SET:
> diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c
> index ccf89645bd66..039ac4c4d892 100644
> --- a/ofproto/ofproto-dpif-sflow.c
> +++ b/ofproto/ofproto-dpif-sflow.c
> @@ -1160,6 +1160,7 @@ dpif_sflow_read_actions(const struct flow *flow,
>  	case OVS_ACTION_ATTR_RECIRC:
>  	case OVS_ACTION_ATTR_HASH:
>          case OVS_ACTION_ATTR_CT:
> +    case OVS_ACTION_ATTR_CT_CLEAR:
>          case OVS_ACTION_ATTR_METER:
>  	    break;
>  
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 468cd160c60e..6d6fcaaca453 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4382,6 +4382,7 @@ xlate_fixup_actions(struct ofpbuf *b, const struct nlattr *actions,
>          case OVS_ACTION_ATTR_USERSPACE:
>          case OVS_ACTION_ATTR_RECIRC:
>          case OVS_ACTION_ATTR_CT:
> +        case OVS_ACTION_ATTR_CT_CLEAR:
>          case OVS_ACTION_ATTR_PUSH_ETH:
>          case OVS_ACTION_ATTR_POP_ETH:
>          case OVS_ACTION_ATTR_ENCAP_NSH:
> @@ -5795,6 +5796,17 @@ compose_conntrack_action(struct xlate_ctx *ctx, struct ofpact_conntrack *ofc,
>  }
>  
>  static void
> +compose_ct_clear_action(struct xlate_ctx *ctx)
> +{
> +    clear_conntrack(ctx);
> +    /* This action originally existed without dpif support. So to preserve
> +     * compatibility, only append it if the dpif supports it. */
> +    if (ctx->xbridge->support.ct_clear) {
> +        nl_msg_put_flag(ctx->odp_actions,  OVS_ACTION_ATTR_CT_CLEAR);
> +    }
> +}
> +
> +static void
>  rewrite_flow_encap_ethernet(struct xlate_ctx *ctx,
>                              struct flow *flow,
>                              struct flow_wildcards *wc)
> @@ -6542,7 +6554,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
>              break;
>  
>          case OFPACT_CT_CLEAR:
> -            clear_conntrack(ctx);
> +            compose_ct_clear_action(ctx);
>              break;
>  
>          case OFPACT_NAT:
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index b14b339a962a..de0cf78b8f3b 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -1322,6 +1322,37 @@ check_ct_eventmask(struct dpif_backer *backer)
>      return !error;
>  }
>  
> +static bool
> +check_ct_clear(struct dpif_backer *backer)
> +{
> +    struct odputil_keybuf keybuf;
> +    uint8_t actbuf[NL_A_FLAG_SIZE];
> +    struct ofpbuf actions;
> +    struct ofpbuf key;
> +    struct flow flow;
> +    bool supported;
> +
> +    struct odp_flow_key_parms odp_parms = {
> +        .flow = &flow,
> +        .probe = true,
> +    };
> +
> +    memset(&flow, 0, sizeof flow);
> +    ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> +    odp_flow_key_from_flow(&odp_parms, &key);
> +
> +    ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf);
> +    nl_msg_put_flag(&actions, OVS_ACTION_ATTR_CT_CLEAR);
> +
> +    supported = dpif_probe_feature(backer->dpif, "ct_clear", &key,
> +                                   &actions, NULL);
> +
> +    VLOG_INFO("%s: Datapath %s ct_clear action",
> +              dpif_name(backer->dpif), (supported) ? "supports"
> +                                                   : "does not support");
> +    return supported;
> +}
> +
>  #define CHECK_FEATURE__(NAME, SUPPORT, FIELD, VALUE, ETHTYPE)               \
>  static bool                                                                 \
>  check_##NAME(struct dpif_backer *backer)                                    \
> @@ -1386,6 +1417,7 @@ check_support(struct dpif_backer *backer)
>      backer->rt_support.clone = check_clone(backer);
>      backer->rt_support.sample_nesting = check_max_sample_nesting(backer);
>      backer->rt_support.ct_eventmask = check_ct_eventmask(backer);
> +    backer->rt_support.ct_clear = check_ct_clear(backer);
>  
>      /* 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 0857c070c8ac..8752061d6439 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -178,7 +178,10 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *,
>      DPIF_SUPPORT_FIELD(size_t, sample_nesting, "Sample nesting")            \
>                                                                              \
>      /* OVS_CT_ATTR_EVENTMASK supported by OVS_ACTION_ATTR_CT action. */     \
> -    DPIF_SUPPORT_FIELD(bool, ct_eventmask, "Conntrack eventmask")
> +    DPIF_SUPPORT_FIELD(bool, ct_eventmask, "Conntrack eventmask")           \
> +                                                                            \
> +    /* True if the datapath supports OVS_ACTION_ATTR_CT_CLEAR action. */    \
> +    DPIF_SUPPORT_FIELD(bool, ct_clear, "Conntrack clear")
>  
>  /* Stores the various features which the corresponding backer supports. */
>  struct dpif_backer_support {
> diff --git a/tests/odp.at b/tests/odp.at
> index cd01b32d72ef..f1ab30727ddb 100644
> --- a/tests/odp.at
> +++ b/tests/odp.at
> @@ -354,6 +354,7 @@ ct(force_commit,nat(src=fe80::20c:29ff:fe88:a18b,random))
>  ct(force_commit,nat(src=fe80::20c:29ff:fe88:1-fe80::20c:29ff:fe88:a18b,random))
>  ct(force_commit,nat(src=[[fe80::20c:29ff:fe88:1]]-[[fe80::20c:29ff:fe88:a18b]]:255-4096,random))
>  ct(force_commit,helper=ftp,nat(src=10.1.1.240-10.1.1.255))
> +ct_clear
>  trunc(100)
>  clone(1)
>  clone(clone(push_vlan(vid=12,pcp=0),2),1)


Acked-by: Flavio Leitner <fbl at sysclose.org>
Thanks!

-- 
Flavio



More information about the dev mailing list