On Thu, 26 Oct 2017 15:30:42 -0400
Eric Garver <[email protected]> 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 <[email protected]>
> Acked-by: William Tu <[email protected]>
> ---
> datapath/linux/compat/include/linux/openvswitch.h | 1 +
> 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 | 4 +++
> 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 +++-
> 13 files changed, 77 insertions(+), 2 deletions(-)
>
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h
> b/datapath/linux/compat/include/linux/openvswitch.h
> index bc6c94b8d52d..28f20103af81 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -924,6 +924,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. */
Missing to update the comment above the definition.
>
> #ifndef __KERNEL__
> OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index e555b5501da9..ddd6de4daff8 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 d5eb8305c8a2..a3046b259c2e 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -5640,6 +5640,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 79b2e6c97305..febeb816e4c4 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1273,6 +1273,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 5f4d23a91a3e..01ac62b25bca 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
> @@ -654,6 +655,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:
> @@ -837,6 +839,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 6304b3dd299a..83b936d2a0fd 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -126,6 +126,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;
> @@ -1054,6 +1055,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;
Aren't we missing parse_odp_action()?
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index 71eb70c3c239..ab0138f250e1 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -7316,6 +7316,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 472c27281d5f..7056897f7467 100644
> --- a/ofproto/ofproto-dpif-ipfix.c
> +++ b/ofproto/ofproto-dpif-ipfix.c
> @@ -2814,6 +2814,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 65a2003a7a8e..c5ee6739ab04 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 ddcaf059ded2..06e213ff72f4 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4381,6 +4381,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:
> @@ -5793,6 +5794,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)
> @@ -6541,7 +6553,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 43d670a15c3f..88fd9d5c8c8f 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 {
Perhaps mention this change in NEWS just in case?
Thanks,
--
Flavio
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
