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

2017-11-18 Thread Eric Garver 
On Sat, Nov 18, 2017 at 02:23:22AM -0200, Flavio Leitner wrote:
> On Thu, 26 Oct 2017 15:30:42 -0400
> Eric Garver  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 
> > Acked-by: William Tu 
> > ---
[...]
> > 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.

Right. Thanks.

[...]
> > --- 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()?

Indeed. Thanks for pointing that out. I also updated the "OVS datapath
actions parsing and formatting - valid forms" test case, which makes
this blatantly obvious. :)

[...]
> > 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 = ,
> > +.probe = true,
> > +};
> > +
> > +memset(, 0, sizeof flow);
> > +ofpbuf_use_stack(, , sizeof keybuf);
> > +odp_flow_key_from_flow(_parms, );
> > +
> > +ofpbuf_use_stack(, , sizeof actbuf);
> > +nl_msg_put_flag(, OVS_ACTION_ATTR_CT_CLEAR);
> > +
> > +supported = dpif_probe_feature(backer->dpif, "ct_clear", ,
> > +   , 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

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

2017-11-17 Thread Flavio Leitner 
On Thu, 26 Oct 2017 15:30:42 -0400
Eric Garver  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 
> Acked-by: William Tu 
> ---
>  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) {
> +

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

2017-10-26 Thread Eric Garver 
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 
Acked-by: William Tu 
---
 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. */
 
 #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