[PATCH/RFC repost 0/8] Open vSwtich ODP Select Group Action

[PATCH/RFC repost 0/8] Open vSwtich ODP Select Group Action

[Simon Horman]

[repost with correct dev@openvswitch.org address]

Hi,

the purpose of this patch-set is to provide a prototype of a select group
action in the Open vSwitch datapath. And the motivation for that is to
allow offloading of selection either in the datapath or by any hooks
provided by the datapath for hardware offloads (a topic of quite some
discussion elsewhere).

This proposal is also designed to tie in with another proposal we have
made to allow the selection method of a select group to be configured using
Open Flow. As such the selection method included in this patchset is
not the focus of this work: any method may be implemented. Rather,
the focus is on the ability to do selection in the datapath.

There are several implementation limitations of this prototype:

* It does not address per-bucket statistics.
  - We believe that the datapath can track per-bucket statistics and;
  - Expose them to user-space using new netlink attributes
* It assumes the select group comes last as the resulting packet
  may vary depending on the bucket that is chosen. Some possibilities
  for handling this include:
  - Performing selection in userspace for such cases
  - Using recirculation
* It seems that if recirculation may occur in more than one bucket
  then separate recirculation ids would be required. This prototype
  does not implement that.

This series is based on the Open vSwitch and its datapath maintained at
https://github.com/openvswitch/ovs.git

It is based on commit 5545e7826896e861c ("lib/odp-util: Add tunnel tp_src,
tp_dst parsing and formatting") of that tree.

Simon Horman (8):
  odp: select group action attributes
  netlink: Allow suppression of warnings for duplicate attributes
  odp-util: formatting of datapath select group action
  datapath: execution of select group action
  datapath: Move last_action() helper to datapath.h
  datapath: validation of select group action
  ofproto: translate datapath select group action
  hack: ofproto: enable odp select action

 datapath/actions.c                                |  74 ++++++++++++++-
 datapath/datapath.h                               |   5 +
 datapath/flow_netlink.c                           | 102 ++++++++++++++++++++
 datapath/linux/compat/include/linux/openvswitch.h |  31 +++++++
 lib/dpif-netdev.c                                 |   1 +
 lib/dpif.c                                        |   1 +
 lib/netlink.c                                     |   2 +-
 lib/netlink.h                                     |   1 +
 lib/odp-execute.c                                 |   1 +
 lib/odp-util.c                                    |  69 ++++++++++++++
 ofproto/ofproto-dpif-xlate.c                      | 108 +++++++++++++++++++++-
 11 files changed, 388 insertions(+), 7 deletions(-)

-- 
2.0.1

[PATCH/RFC repost 1/8] odp: select group action attributes

[Simon Horman]

This is the core of a proposed ODP select group action.
It models OpenFlow select groups without any extensions.

Further work:

We believe there is scope to add OVS_SELECT_GROUP_ATTR_* attributes
to supply parameters to the selection method: for example which
selection algorithm to use. This relates to a proposed
Open Flow extension that we have made.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 datapath/linux/compat/include/linux/openvswitch.h | 31 +++++++++++++++++++++++
 lib/dpif-netdev.c                                 |  1 +
 lib/dpif.c                                        |  1 +
 lib/odp-execute.c                                 |  1 +
 lib/odp-util.c                                    |  2 ++
 5 files changed, 36 insertions(+)

diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
index 6910dc4..91ff939 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -510,6 +510,35 @@ enum ovs_sample_attr {
 #define OVS_SAMPLE_ATTR_MAX (__OVS_SAMPLE_ATTR_MAX - 1)
 
 /**
+ * enum ovs_bucket_attr - Bucket for * %OVS_ACTION_ATTR_SELECT_GROUP action.
+ * @OVS_BUCKET_ATTR_WEIGHT.   Relative weight of bucket.
+ * @OVS_BUCKET_ATTR_ACTIONS.  Set of actions to execute.
+ */
+enum ovs_bucket_attr {
+	OVS_BUCKET_ATTR_UNSPEC,
+	OVS_BUCKET_ATTR_WEIGHT,  /* u16. Relative weight of bucket. */
+	OVS_BUCKET_ATTR_ACTIONS, /* Nested OVS_BUCKET_ATTR_* attributes. */
+	__OVS_BUCKET_ATTR_MAX,
+};
+
+#define OVS_BUCKET_ATTR_MAX (__OVS_BUCKET_ATTR_MAX - 1)
+
+/**
+ * enum ovs_select_group_attr - Attributes for * %OVS_ACTION_ATTR_SELECT_GROUP action.
+ * @OVS_SELECT_GROUP_ATTR_BUCKET.  A bucket whose actions will be executed
+ * if the bucket is selected. One ore more buckets may be present.
+ *
+ * Selects a bucket and executes its actions.
+ */
+enum ovs_select_group_attr {
+	OVS_SELECT_GROUP_ATTR_UNSPEC,
+	OVS_SELECT_GROUP_ATTR_BUCKET, /* Nested OVS_BUCKET_ATTR_* attributes. */
+	__OVS_SELECT_GROUP_ATTR_MAX,
+};
+
+#define OVS_SELECT_GROUP_ATTR_MAX (__OVS_SELECT_GROUP_ATTR_MAX - 1)
+
+/**
  * enum ovs_userspace_attr - Attributes for %OVS_ACTION_ATTR_USERSPACE action.
  * @OVS_USERSPACE_ATTR_PID: u32 Netlink PID to which the %OVS_PACKET_CMD_ACTION
  * message should be sent.  Required.
@@ -609,6 +638,7 @@ struct ovs_action_hash {
  * indicate the new packet contents. This could potentially still be
  * %ETH_P_MPLS if the resulting MPLS label stack is not empty.  If there
  * is no MPLS label stack, as determined by ethertype, no action is taken.
+ * @OVS_ACTION_ATTR_SELECT_GROUP: Select a bucket and execute its actions.
  *
  * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
  * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -631,6 +661,7 @@ enum ovs_action_attr {
 				       * data immediately followed by a mask.
 				       * The data must be zero for the unmasked
 				       * bits. */
+	OVS_ACTION_ATTR_SELECT_GROUP, /* Nested OVS_SELECT_GROUP_*. */
 	__OVS_ACTION_ATTR_MAX
 };
 
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 409c9bf..bfcfd8c 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2560,6 +2560,7 @@ dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
     case OVS_ACTION_ATTR_SET:
     case OVS_ACTION_ATTR_SET_MASKED:
     case OVS_ACTION_ATTR_SAMPLE:
+    case OVS_ACTION_ATTR_SELECT_GROUP:
     case OVS_ACTION_ATTR_UNSPEC:
     case __OVS_ACTION_ATTR_MAX:
         OVS_NOT_REACHED();
diff --git a/lib/dpif.c b/lib/dpif.c
index bf2c5f9..90b561f 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -1043,6 +1043,7 @@ dpif_execute_helper_cb(void *aux_, struct dpif_packet **packets, int cnt,
     case OVS_ACTION_ATTR_SET:
     case OVS_ACTION_ATTR_SET_MASKED:
     case OVS_ACTION_ATTR_SAMPLE:
+    case OVS_ACTION_ATTR_SELECT_GROUP:
     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 e4bee18..c0ba868 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -518,6 +518,7 @@ odp_execute_actions__(void *dp, struct dpif_packet **packets, int cnt,
             }
             break;
 
+        case OVS_ACTION_ATTR_SELECT_GROUP:
         case OVS_ACTION_ATTR_UNSPEC:
         case __OVS_ACTION_ATTR_MAX:
             OVS_NOT_REACHED();
diff --git a/lib/odp-util.c b/lib/odp-util.c
index d205473..77b456f 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -83,6 +83,7 @@ odp_action_len(uint16_t type)
     case OVS_ACTION_ATTR_HASH: return sizeof(struct ovs_action_hash);
     case OVS_ACTION_ATTR_SET: return -2;
     case OVS_ACTION_ATTR_SET_MASKED: return -2;
+    case OVS_ACTION_ATTR_SELECT_GROUP: return -2;
     case OVS_ACTION_ATTR_SAMPLE: return -2;
 
     case OVS_ACTION_ATTR_UNSPEC:
@@ -581,6 +582,7 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
     case OVS_ACTION_ATTR_SAMPLE:
         format_odp_sample_action(ds, a);
         break;
+    case OVS_ACTION_ATTR_SELECT_GROUP:
     case OVS_ACTION_ATTR_UNSPEC:
     case __OVS_ACTION_ATTR_MAX:
     default:
-- 
2.0.1

[PATCH/RFC repost 2/8] netlink: Allow suppression of warnings for duplicate attributes

[Simon Horman]

Add a multiple field to struct nl_policy which if set suppresses
warning of duplicate attributes in nl_parse_nested().

As is the case without this patch only the last occurrence of an
attribute is stored in attrs by nl_parse_nested(). As such
if the multiple field of struct nl_policy is set then it
is up to the caller to parse the message to extract all the attributes.

This is in preparation for allowing multiple OVS_SELECT_GROUP_ATTR_BUCKET
attributes in a nested OVS_ACTION_ATTR_SELECT_GROUP attribute.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 lib/netlink.c | 2 +-
 lib/netlink.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/netlink.c b/lib/netlink.c
index 24b2168..bc30248 100644
--- a/lib/netlink.c
+++ b/lib/netlink.c
@@ -743,7 +743,7 @@ nl_policy_parse(const struct ofpbuf *msg, size_t nla_offset,
             if (!nl_attr_validate(nla, e)) {
                 return false;
             }
-            if (attrs[type]) {
+            if (attrs[type] && !e->multiple) {
                 VLOG_DBG_RL(&rl, "duplicate attr %"PRIu16, type);
             }
             attrs[type] = nla;
diff --git a/lib/netlink.h b/lib/netlink.h
index f9234da..b0a72fd 100644
--- a/lib/netlink.h
+++ b/lib/netlink.h
@@ -195,6 +195,7 @@ struct nl_policy
     enum nl_attr_type type;
     size_t min_len, max_len;
     bool optional;
+    bool multiple;
 };
 
 #define NL_POLICY_FOR(TYPE) \
-- 
2.0.1

[PATCH/RFC repost 3/8] odp-util: formatting of datapath select group action

[Simon Horman]

Allow formatting of select group action. This is used
when pretty-printing datapath flows. Subsequent patches
will add support for the select group action to the datapath
and ovs-vswtichd.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 lib/odp-util.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 77b456f..4c8dd39 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -182,6 +182,71 @@ format_odp_sample_action(struct ds *ds, const struct nlattr *attr)
     ds_put_format(ds, "))");
 }
 
+static bool
+format_odp_bucket(struct ds *ds, const struct nlattr *attr)
+{
+    static const struct nl_policy ovs_sample_policy[] = {
+        [OVS_BUCKET_ATTR_WEIGHT] = { .type = NL_A_U16 },
+        [OVS_BUCKET_ATTR_ACTIONS] = { .type = NL_A_NESTED }
+    };
+    struct nlattr *a[ARRAY_SIZE(ovs_sample_policy)];
+    const struct nlattr *nla_acts;
+    int len;
+
+    ds_put_cstr(ds, "bucket");
+
+    if (!nl_parse_nested(attr, ovs_sample_policy, a, ARRAY_SIZE(a))) {
+        ds_put_cstr(ds, "(error)");
+        return false;
+    }
+
+    ds_put_format(ds, "(weight=%d,",
+                  nl_attr_get_u16(a[OVS_BUCKET_ATTR_WEIGHT]));
+
+    ds_put_cstr(ds, "actions(");
+    nla_acts = nl_attr_get(a[OVS_SAMPLE_ATTR_ACTIONS]);
+    len = nl_attr_get_size(a[OVS_SAMPLE_ATTR_ACTIONS]);
+    format_odp_actions(ds, nla_acts, len);
+    ds_put_format(ds, "))");
+
+    return true;
+}
+
+static void
+format_odp_select_group_action(struct ds *ds, const struct nlattr *attr)
+{
+    static const struct nl_policy ovs_sample_policy[] = {
+        [OVS_SELECT_GROUP_ATTR_BUCKET] = { .type = NL_A_NESTED,
+                                           .multiple = true }
+    };
+    struct nlattr *a[ARRAY_SIZE(ovs_sample_policy)];
+    struct nlattr *nla;
+    struct ofpbuf buf;
+    size_t left;
+
+    ds_put_cstr(ds, "select_group");
+
+    if (!nl_parse_nested(attr, ovs_sample_policy, a, ARRAY_SIZE(a))) {
+        ds_put_cstr(ds, "(error)");
+        return;
+    }
+
+    ds_put_cstr(ds, "(actions(");
+
+    nl_attr_get_nested(attr, &buf);
+    NL_ATTR_FOR_EACH (nla, left, ofpbuf_data(&buf), ofpbuf_size(&buf))
+    {
+        uint16_t type = nl_attr_type(nla);
+
+        ovs_assert(type == OVS_SELECT_GROUP_ATTR_BUCKET);
+        if (!format_odp_bucket(ds, nla)) {
+            break;
+        }
+    }
+
+    ds_put_format(ds, "))");
+}
+
 static const char *
 slow_path_reason_to_string(uint32_t reason)
 {
@@ -583,6 +648,8 @@ format_odp_action(struct ds *ds, const struct nlattr *a)
         format_odp_sample_action(ds, a);
         break;
     case OVS_ACTION_ATTR_SELECT_GROUP:
+        format_odp_select_group_action(ds, a);
+        break;
     case OVS_ACTION_ATTR_UNSPEC:
     case __OVS_ACTION_ATTR_MAX:
     default:
-- 
2.0.1

[PATCH/RFC repost 4/8] datapath: execution of select group action

[Simon Horman]

Allow execution of select group action in the datapath.
A subsequent patch will add validation and copying of
the select group action in the datapath.

The selection algorithm used is based on the RSS hash.
This was chosen because it resembles the algorithm currently
used by the implementation of select groups in ovs-vswitchd.

It may well be that in this case it is more efficient to handle
things in ovs-vswitchd, avoiding the cost of hashing on each packet.
Or that the hashing mechanism used can be optimised somehow. However,
we would like to avoid focusing on these questions of this particular
implementation.

The purpose of this patch is to form part of a prototype for a select
group action. The current Open Flow specification allows for the
implementation to select an algorithm. And we have made a separate
proposal to allow the selection algorithm to be configured via Open Flow.
Thus the algorithm and used and its implementation are not central
to the prototype.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 datapath/actions.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/datapath/actions.c b/datapath/actions.c
index 8d18848..51ca40b 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -809,6 +809,72 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
 	return 0;
 }
 
+const struct nlattr *bucket_actions(const struct nlattr *attr)
+{
+	const struct nlattr *a;
+	int rem;
+
+	for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
+	     a = nla_next(a, &rem)) {
+		if (nla_type(a) == OVS_BUCKET_ATTR_ACTIONS) {
+			return a;
+		}
+	}
+
+	return NULL;
+}
+
+static u16 bucket_weight(const struct nlattr *attr)
+{
+	const struct nlattr *weight;
+
+	/* validate_and_copy_bucket() ensures that the first
+	 * attribute is OVS_BUCKET_ATTR_WEIGHT */
+	weight = nla_data(attr);
+	BUG_ON(nla_type(weight) != OVS_BUCKET_ATTR_WEIGHT);
+	return nla_get_u16(weight);
+}
+
+static int select_group(struct datapath *dp, struct sk_buff *skb,
+			const struct nlattr *attr)
+{
+	const struct nlattr *best_bucket = NULL;
+	const struct nlattr *acts_list;
+	const struct nlattr *bucket;
+	struct sk_buff *sample_skb;
+	u32 best_score = 0;
+	u32 basis;
+	u32 i = 0;
+	int rem;
+
+	basis = skb_get_hash(skb);
+
+	/* Only possible type of attributes is OVS_SELECT_GROUP_ATTR_BUCKET */
+	for (bucket = nla_data(attr), rem = nla_len(attr); rem > 0;
+	     bucket = nla_next(bucket, &rem)) {
+		uint16_t weight = bucket_weight(bucket);
+		// XXX: This hashing seems expensive
+		u32 score = (jhash_1word(i, basis) & 0xffff) * weight;
+
+		if (score >= best_score) {
+			best_bucket = bucket;
+			best_score = score;
+		}
+		i++;
+	}
+
+	acts_list = bucket_actions(best_bucket);
+
+	/* A select group action is always the final action so
+	 * there is no need to clone the skb in case of side effects.
+	 * Instead just take a reference to it which will be released
+	 * by do_execute_actions(). */
+	skb_get(skb);
+
+	return do_execute_actions(dp, skb, nla_data(acts_list),
+				  nla_len(acts_list));
+}
+
 static void execute_hash(struct sk_buff *skb, const struct nlattr *attr)
 {
 	struct sw_flow_key *key = OVS_CB(skb)->pkt_key;
@@ -986,6 +1052,10 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 		case OVS_ACTION_ATTR_SAMPLE:
 			err = sample(dp, skb, a);
 			break;
+
+		case OVS_ACTION_ATTR_SELECT_GROUP:
+			err = select_group(dp, skb, a);
+			break;
 		}
 
 		if (unlikely(err)) {
-- 
2.0.1

[PATCH/RFC repost 5/8] datapath: Move last_action() helper to datapath.h

[Simon Horman]

This is in preparation for using last_action() from
more than one C file as part of supporting an odp select group action.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 datapath/actions.c  | 6 ------
 datapath/datapath.h | 5 +++++
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/datapath/actions.c b/datapath/actions.c
index 51ca40b..9d27234 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -752,11 +752,6 @@ static int output_userspace(struct datapath *dp, struct sk_buff *skb,
 	return ovs_dp_upcall(dp, skb, &upcall);
 }
 
-static bool last_action(const struct nlattr *a, int rem)
-{
-	return a->nla_len == rem;
-}
-
 static int sample(struct datapath *dp, struct sk_buff *skb,
 		  const struct nlattr *attr)
 {
@@ -841,7 +836,6 @@ static int select_group(struct datapath *dp, struct sk_buff *skb,
 	const struct nlattr *best_bucket = NULL;
 	const struct nlattr *acts_list;
 	const struct nlattr *bucket;
-	struct sk_buff *sample_skb;
 	u32 best_score = 0;
 	u32 basis;
 	u32 i = 0;
diff --git a/datapath/datapath.h b/datapath/datapath.h
index c5d3c86..74a15e6 100644
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -209,4 +209,9 @@ do {								\
 	if (net_ratelimit())					\
 		pr_info("netlink: " fmt, ##__VA_ARGS__);	\
 } while (0)
+
+static inline bool last_action(const struct nlattr *a, int rem)
+{
+	return a->nla_len == rem;
+}
 #endif /* datapath.h */
-- 
2.0.1

[PATCH/RFC repost 6/8] datapath: validation of select group action

[Simon Horman]

Allow validation and copying of select group actions.
This completes the prototype select group action implementation
in the datapath. Subsequent patches will add support to ovs-vswtichd.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 datapath/flow_netlink.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
index 6c74841..90eddba 100644
--- a/datapath/flow_netlink.c
+++ b/datapath/flow_netlink.c
@@ -1497,6 +1497,94 @@ static int validate_and_copy_sample(const struct nlattr *attr,
 	return 0;
 }
 
+static int validate_and_copy_bucket(const struct nlattr *attr,
+				    const struct sw_flow_key *key, int depth,
+				    struct sw_flow_actions **sfa,
+				    __be16 eth_type, __be16 vlan_tci)
+{
+	const struct nlattr *attrs[OVS_BUCKET_ATTR_MAX + 1];
+	const struct nlattr *weight, *actions;
+	const struct nlattr *a;
+	int rem, start, err, st_acts;
+
+	memset(attrs, 0, sizeof(attrs));
+	nla_for_each_nested(a, attr, rem) {
+		int type = nla_type(a);
+		if (!type || type > OVS_BUCKET_ATTR_MAX || attrs[type])
+			return -EINVAL;
+		attrs[type] = a;
+	}
+	if (rem)
+		return -EINVAL;
+
+	weight = attrs[OVS_BUCKET_ATTR_WEIGHT];
+	if (!weight || nla_len(weight) != sizeof(u16))
+		return -EINVAL;
+
+	actions = attrs[OVS_BUCKET_ATTR_ACTIONS];
+	if (!actions || (nla_len(actions) && nla_len(actions) < NLA_HDRLEN))
+		return -EINVAL;
+
+	/* validation done, copy sample action. */
+	start = add_nested_action_start(sfa, OVS_SELECT_GROUP_ATTR_BUCKET);
+	if (start < 0)
+		return start;
+	err = add_action(sfa, OVS_BUCKET_ATTR_WEIGHT,
+			 nla_data(weight), sizeof(u16));
+	if (err)
+		return err;
+	st_acts = add_nested_action_start(sfa, OVS_SAMPLE_ATTR_ACTIONS);
+	if (st_acts < 0)
+		return st_acts;
+
+	err = __ovs_nla_copy_actions(actions, key, depth + 1, sfa,
+				     eth_type, vlan_tci);
+	if (err)
+		return err;
+
+	add_nested_action_end(*sfa, st_acts);
+	add_nested_action_end(*sfa, start);
+
+	return 0;
+}
+
+static int validate_and_copy_select_group(const struct nlattr *attr,
+					  const struct sw_flow_key *key,
+					  int depth,
+					  struct sw_flow_actions **sfa,
+					  __be16 eth_type, __be16 vlan_tci)
+{
+	bool have_bucket = false;
+	const struct nlattr *a;
+	int rem, start, err;
+
+	start = add_nested_action_start(sfa, OVS_ACTION_ATTR_SELECT_GROUP);
+	if (start < 0)
+		return start;
+
+	nla_for_each_nested(a, attr, rem) {
+		int type = nla_type(a);
+
+		if (!type || type > OVS_SAMPLE_ATTR_MAX)
+			return -EINVAL;
+
+		/* Only possible type is OVS_SELECT_GROUP_ATTR_BUCKET */
+		if ((nla_len(a) && nla_len(a) < NLA_HDRLEN))
+			return -EINVAL;
+		err = validate_and_copy_bucket(a, key, depth, sfa,
+					       eth_type, vlan_tci);
+		if (err < 0)
+			return err;
+		have_bucket = true;
+	}
+	if (rem || !have_bucket)
+		return -EINVAL;
+
+	add_nested_action_end(*sfa, start);
+
+	return 0;
+}
+
 static int validate_tp_port(const struct sw_flow_key *flow_key,
 			    __be16 eth_type)
 {
@@ -1750,6 +1838,7 @@ static int __ovs_nla_copy_actions(const struct nlattr *attr,
 			[OVS_ACTION_ATTR_POP_VLAN] = 0,
 			[OVS_ACTION_ATTR_SET] = (u32)-1,
 			[OVS_ACTION_ATTR_SAMPLE] = (u32)-1,
+			[OVS_ACTION_ATTR_SELECT_GROUP] = (u32)-1,
 			[OVS_ACTION_ATTR_HASH] = sizeof(struct ovs_action_hash)
 		};
 		const struct ovs_action_push_vlan *vlan;
@@ -1856,6 +1945,19 @@ static int __ovs_nla_copy_actions(const struct nlattr *attr,
 			skip_copy = true;
 			break;
 
+		case OVS_ACTION_ATTR_SELECT_GROUP:
+			/* Nothing may come after a select group */
+			if (!last_action(a, rem))
+				return -EINVAL;
+
+			err = validate_and_copy_select_group(a, key, depth,
+							     sfa, eth_type,
+							     vlan_tci);
+			if (err)
+				return err;
+			skip_copy = true;
+			break;
+
 		default:
 			return -EINVAL;
 		}
-- 
2.0.1

[PATCH/RFC repost 7/8] ofproto: translate datapath select group action

[Simon Horman]

This add support for the select group action to ovs-vswtichd.

This new feature is currently disabled in xlate_select_group()
because ctx->xbridge->dp_select_group is always false.

This patch is a prototype and has several limitations:

* It assumes that no actions follow a select group action
  because the resulting packet after a select group action may
  differ depending on the bucket used. It may be possible
  to address this problem using recirculation. Or to not use
  the datapath select group in such situations. In any case
  this patch does not solve this problem or even prevent it
  from occurring.

* If recirculation occurs inside more than one bucket then
  it seems that separate recirculation ids would be required
  for each occurrence. This patch does not solve this problem or even
  prevent it from occurring.

* No attempt is made to handle per-bucket statistics.
  This would most likely require further datapath modifications.

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 ofproto/ofproto-dpif-xlate.c | 108 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 107 insertions(+), 1 deletion(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 6e33a27..b08a821 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -103,6 +103,9 @@ struct xbridge {
      * False if the datapath supports only 8-byte (or shorter) userdata. */
     bool variable_length_userdata;
 
+    /* True if datapath supports select group action */
+    bool dp_select_group;
+
     /* Number of MPLS label stack entries that the datapath supports
      * in matches. */
     size_t max_mpls_depth;
@@ -329,6 +332,9 @@ static void xlate_report(struct xlate_ctx *, const char *);
 static void xlate_table_action(struct xlate_ctx *, ofp_port_t in_port,
                                uint8_t table_id, bool may_packet_in,
                                bool honor_table_miss);
+static void xlate_group_stats(struct xlate_ctx *, struct group_dpif *,
+                              struct ofputil_bucket *);
+static void xlate_group_bucket(struct xlate_ctx *, struct ofputil_bucket *);
 static bool input_vid_is_valid(uint16_t vid, struct xbundle *, bool warn);
 static uint16_t input_vid_to_vlan(const struct xbundle *, uint16_t vid);
 static void output_normal(struct xlate_ctx *, const struct xbundle *,
@@ -2353,6 +2359,61 @@ fix_sflow_action(struct xlate_ctx *ctx)
                          ctx->sflow_odp_port, ctx->sflow_n_outputs, cookie);
 }
 
+/* Compose bucket for SELECT_GROUP action. */
+static void
+compose_bucket_action(struct xlate_ctx *ctx, struct group_dpif *group,
+                      struct ofputil_bucket *bucket)
+{
+    size_t bucket_offset, actions_offset;
+    struct ofpbuf *odp_actions = ctx->xout->odp_actions;
+
+    bucket_offset = nl_msg_start_nested(odp_actions,
+                                        OVS_SELECT_GROUP_ATTR_BUCKET);
+
+    nl_msg_put_u16(odp_actions, OVS_BUCKET_ATTR_WEIGHT, bucket->weight);
+
+    actions_offset = nl_msg_start_nested(odp_actions, OVS_BUCKET_ATTR_ACTIONS);
+
+    xlate_group_bucket(ctx, bucket);
+    xlate_group_stats(ctx, group, bucket);
+
+    nl_msg_end_nested(odp_actions, actions_offset);
+    nl_msg_end_nested(odp_actions, bucket_offset);
+
+    ctx->xout->slow |= commit_odp_actions(&ctx->xin->flow, &ctx->base_flow,
+                                          odp_actions, &ctx->xout->wc);
+}
+
+/* Compose SELECT_GROUP action.
+ * May be called multiple times to add multiple buckets.
+ * The first call should pass SIZE_MAX as the value for offset_
+ * and subsequent calls should pass returned by the previous call.
+ * The last call should pass NULL as the bucket parameter
+ * and previous calls should pass a valid bucket to add to the
+ * select group. */
+static size_t
+compose_select_group_action(struct xlate_ctx *ctx, struct group_dpif *group,
+                            struct ofputil_bucket *bucket,
+                            const size_t offset_)
+{
+    size_t offset = offset_;
+
+    if (bucket != NULL) {
+        if (offset == SIZE_MAX) {
+            offset = nl_msg_start_nested(ctx->xout->odp_actions,
+                                         OVS_ACTION_ATTR_SELECT_GROUP);
+        }
+
+        compose_bucket_action(ctx, group, bucket);
+    } else {
+        if (offset != SIZE_MAX) {
+            nl_msg_end_nested(ctx->xout->odp_actions, offset);
+        }
+    }
+
+    return offset;
+}
+
 static enum slow_path_reason
 process_special(struct xlate_ctx *ctx, const struct flow *flow,
                 const struct xport *xport, const struct ofpbuf *packet)
@@ -2775,7 +2836,40 @@ xlate_ff_group(struct xlate_ctx *ctx, struct group_dpif *group)
 }
 
 static void
-xlate_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
+xlate_select_group_datapath(struct xlate_ctx *ctx, struct group_dpif *group)
+{
+    struct flow_wildcards *wc = &ctx->xout->wc;
+    struct ofputil_bucket *bucket;
+    const struct list *buckets;
+    struct flow old_flow = ctx->xin->flow;
+    size_t offset = SIZE_MAX;
+
+    group_dpif_get_buckets(group, &buckets);
+    LIST_FOR_EACH (bucket, list_node, buckets) {
+        if (bucket_is_alive(ctx, bucket, 0)) {
+            offset = compose_select_group_action(ctx, group, bucket, offset);
+
+            /* Roll back flow to previous state.
+             *
+             * This allows composition of the actions for subsequent
+             * buckets in such a way that they apply to the state of the
+             * packet present before the select group action.
+             *
+             * XXX: Assumes no actions follow the select group action.
+             * Handle with recirculation?
+             */
+            ctx->xin->flow = old_flow;
+        }
+    }
+
+    if (offset != SIZE_MAX) {
+        memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
+        compose_select_group_action(ctx, group, NULL, offset);
+    }
+}
+
+static void
+xlate_select_group_userspace(struct xlate_ctx *ctx, struct group_dpif *group)
 {
     struct flow_wildcards *wc = &ctx->xout->wc;
     struct ofputil_bucket *bucket;
@@ -2800,6 +2894,16 @@ xlate_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
 }
 
 static void
+xlate_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
+{
+    if (ctx->xbridge->dp_select_group) {
+        xlate_select_group_datapath(ctx, group);
+    } else {
+        xlate_select_group_userspace(ctx, group);
+    }
+}
+
+static void
 xlate_group_action__(struct xlate_ctx *ctx, struct group_dpif *group)
 {
     ctx->in_group = true;
@@ -2990,6 +3094,8 @@ compose_recirculate_action(struct xlate_ctx *ctx,
     ofpacts_len = ofpacts_base_len -
         ((uint8_t *)ofpact_current - (uint8_t *)ofpacts_base);
 
+    /* XXX: If recirculation occurs inside buckets of a select group action
+     * then multiple recirculation ids may be required. */
     if (ctx->rule) {
         id = rule_dpif_get_recirc_id(ctx->rule);
     } else {
-- 
2.0.1

[PATCH/RFC repost 8/8] hack: ofproto: enable odp select action

[Simon Horman]

This is a quick hack to enable the datapath group select action.
It is in lieu of some combination of:
* probing
* run-time configuration by the end-use.
* run-time heuristic to use the action as appropriate

Signed-off-by: Simon Horman <simon.horman@netronome.com>
---
 ofproto/ofproto-dpif-xlate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index b08a821..eca617d 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2896,7 +2896,7 @@ xlate_select_group_userspace(struct xlate_ctx *ctx, struct group_dpif *group)
 static void
 xlate_select_group(struct xlate_ctx *ctx, struct group_dpif *group)
 {
-    if (ctx->xbridge->dp_select_group) {
+    if (true /*ctx->xbridge->dp_select_group*/ ) {
         xlate_select_group_datapath(ctx, group);
     } else {
         xlate_select_group_userspace(ctx, group);
-- 
2.0.1

Re: [PATCH/RFC repost 3/8] odp-util: formatting of datapath select group action

[Thomas Graf]

On 09/18/14 at 10:55am, Simon Horman wrote:
> Allow formatting of select group action. This is used
> when pretty-printing datapath flows. Subsequent patches
> will add support for the select group action to the datapath
> and ovs-vswtichd.
> 
> Signed-off-by: Simon Horman <simon.horman-wFxRvT7yatFl57MIdRCFDg@public.gmane.org>
> ---
>  lib/odp-util.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
> 
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 77b456f..4c8dd39 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -182,6 +182,71 @@ format_odp_sample_action(struct ds *ds, const struct nlattr *attr)
>      ds_put_format(ds, "))");
>  }
>  
> +static bool
> +format_odp_bucket(struct ds *ds, const struct nlattr *attr)
> +{
> +    static const struct nl_policy ovs_sample_policy[] = {

Not that it would matter much but you might want to rename that to
ovs_bucket_policy[]. Same below.

Re: [PATCH/RFC repost 4/8] datapath: execution of select group action

[Thomas Graf]

On 09/18/14 at 10:55am, Simon Horman wrote:
> +const struct nlattr *bucket_actions(const struct nlattr *attr)
> +{
> +	const struct nlattr *a;
> +	int rem;
> +
> +	for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
> +	     a = nla_next(a, &rem)) {
> +		if (nla_type(a) == OVS_BUCKET_ATTR_ACTIONS) {
> +			return a;
> +		}
> +	}
> +
> +	return NULL;
> +}

This is identical to nla_find(). I realize this is not the only
example but I think we should stop replicating existing Netlink
functionality and add missing pieces to lib/nlattr.c.

> +static u16 bucket_weight(const struct nlattr *attr)
> +{
> +	const struct nlattr *weight;
> +
> +	/* validate_and_copy_bucket() ensures that the first
> +	 * attribute is OVS_BUCKET_ATTR_WEIGHT */
> +	weight = nla_data(attr);
> +	BUG_ON(nla_type(weight) != OVS_BUCKET_ATTR_WEIGHT);
> +	return nla_get_u16(weight);
> +}
> +
> +static int select_group(struct datapath *dp, struct sk_buff *skb,
> +			const struct nlattr *attr)
> +{
> +	const struct nlattr *best_bucket = NULL;
> +	const struct nlattr *acts_list;
> +	const struct nlattr *bucket;
> +	struct sk_buff *sample_skb;
> +	u32 best_score = 0;
> +	u32 basis;
> +	u32 i = 0;
> +	int rem;
> +
> +	basis = skb_get_hash(skb);
> +
> +	/* Only possible type of attributes is OVS_SELECT_GROUP_ATTR_BUCKET */
> +	for (bucket = nla_data(attr), rem = nla_len(attr); rem > 0;
> +	     bucket = nla_next(bucket, &rem)) {
> +		uint16_t weight = bucket_weight(bucket);

I think we should validate only once when we copy then assume it is
correct.

> +		// XXX: This hashing seems expensive
> +		u32 score = (jhash_1word(i, basis) & 0xffff) * weight;

Maybe just calculate a weighted distribution table pointing to the
buckets which you index with 8 bits of the hash.

> +		if (score >= best_score) {
> +			best_bucket = bucket;
> +			best_score = score;
> +		}
> +		i++;
> +	}
> +
> +	acts_list = bucket_actions(best_bucket);
> +
> +	/* A select group action is always the final action so
> +	 * there is no need to clone the skb in case of side effects.
> +	 * Instead just take a reference to it which will be released
> +	 * by do_execute_actions(). */
> +	skb_get(skb);
> +
> +	return do_execute_actions(dp, skb, nla_data(acts_list),
> +				  nla_len(acts_list));

Do we need a recursion limit here?

> +}

Re: [PATCH/RFC repost 5/8] datapath: Move last_action() helper to datapath.h

[Thomas Graf]

On 09/18/14 at 10:55am, Simon Horman wrote:
> diff --git a/datapath/datapath.h b/datapath/datapath.h
> index c5d3c86..74a15e6 100644
> --- a/datapath/datapath.h
> +++ b/datapath/datapath.h
> @@ -209,4 +209,9 @@ do {								\
>  	if (net_ratelimit())					\
>  		pr_info("netlink: " fmt, ##__VA_ARGS__);	\
>  } while (0)
> +
> +static inline bool last_action(const struct nlattr *a, int rem)
> +{
> +	return a->nla_len == rem;
> +}
>  #endif /* datapath.h */

Can we rename & move this to <net/netlink.h> instead?

Re: [PATCH/RFC repost 3/8] odp-util: formatting of datapath select group action

[Simon Horman]

On Fri, Sep 19, 2014 at 02:44:49PM +0100, Thomas Graf wrote:
> On 09/18/14 at 10:55am, Simon Horman wrote:
> > Allow formatting of select group action. This is used
> > when pretty-printing datapath flows. Subsequent patches
> > will add support for the select group action to the datapath
> > and ovs-vswtichd.
> > 
> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> > ---
> >  lib/odp-util.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> > 
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 77b456f..4c8dd39 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -182,6 +182,71 @@ format_odp_sample_action(struct ds *ds, const struct nlattr *attr)
> >      ds_put_format(ds, "))");
> >  }
> >  
> > +static bool
> > +format_odp_bucket(struct ds *ds, const struct nlattr *attr)
> > +{
> > +    static const struct nl_policy ovs_sample_policy[] = {
> 
> Not that it would matter much but you might want to rename that to
> ovs_bucket_policy[]. Same below.

Thanks, I renamed all the new instances of ovs_sample_policy
(which I cut and pasted from the implementation of sample actions).

Re: [PATCH/RFC repost 5/8] datapath: Move last_action() helper to datapath.h

[Simon Horman]

On Fri, Sep 19, 2014 at 03:06:38PM +0100, Thomas Graf wrote:
> On 09/18/14 at 10:55am, Simon Horman wrote:
> > diff --git a/datapath/datapath.h b/datapath/datapath.h
> > index c5d3c86..74a15e6 100644
> > --- a/datapath/datapath.h
> > +++ b/datapath/datapath.h
> > @@ -209,4 +209,9 @@ do {								\
> >  	if (net_ratelimit())					\
> >  		pr_info("netlink: " fmt, ##__VA_ARGS__);	\
> >  } while (0)
> > +
> > +static inline bool last_action(const struct nlattr *a, int rem)
> > +{
> > +	return a->nla_len == rem;
> > +}
> >  #endif /* datapath.h */
> 
> Can we rename & move this to <net/netlink.h> instead?

Sure, how about nla_is_last()?

Re: [PATCH/RFC repost 4/8] datapath: execution of select group action

[Simon Horman]

On Fri, Sep 19, 2014 at 03:05:27PM +0100, Thomas Graf wrote:
> On 09/18/14 at 10:55am, Simon Horman wrote:
> > +const struct nlattr *bucket_actions(const struct nlattr *attr)
> > +{
> > +	const struct nlattr *a;
> > +	int rem;
> > +
> > +	for (a = nla_data(attr), rem = nla_len(attr); rem > 0;
> > +	     a = nla_next(a, &rem)) {
> > +		if (nla_type(a) == OVS_BUCKET_ATTR_ACTIONS) {
> > +			return a;
> > +		}
> > +	}
> > +
> > +	return NULL;
> > +}
> 
> This is identical to nla_find(). I realize this is not the only
> example but I think we should stop replicating existing Netlink
> functionality and add missing pieces to lib/nlattr.c.

Thanks. For starters I have removed bucket_actions() and replaced
its usage with a call to nla_find().

> > +static u16 bucket_weight(const struct nlattr *attr)
> > +{
> > +	const struct nlattr *weight;
> > +
> > +	/* validate_and_copy_bucket() ensures that the first
> > +	 * attribute is OVS_BUCKET_ATTR_WEIGHT */
> > +	weight = nla_data(attr);
> > +	BUG_ON(nla_type(weight) != OVS_BUCKET_ATTR_WEIGHT);
> > +	return nla_get_u16(weight);
> > +}
> > +
> > +static int select_group(struct datapath *dp, struct sk_buff *skb,
> > +			const struct nlattr *attr)
> > +{
> > +	const struct nlattr *best_bucket = NULL;
> > +	const struct nlattr *acts_list;
> > +	const struct nlattr *bucket;
> > +	struct sk_buff *sample_skb;
> > +	u32 best_score = 0;
> > +	u32 basis;
> > +	u32 i = 0;
> > +	int rem;
> > +
> > +	basis = skb_get_hash(skb);
> > +
> > +	/* Only possible type of attributes is OVS_SELECT_GROUP_ATTR_BUCKET */
> > +	for (bucket = nla_data(attr), rem = nla_len(attr); rem > 0;
> > +	     bucket = nla_next(bucket, &rem)) {
> > +		uint16_t weight = bucket_weight(bucket);
> 
> I think we should validate only once when we copy then assume it is
> correct.

That is the intention of this code, is it doing something else?

I think there is some scope to store the bucket in a more efficient
form for execution. But I'm not sure that any other actions
receive such treatment. So I postponed inventing that wheel.

> > +		// XXX: This hashing seems expensive
> > +		u32 score = (jhash_1word(i, basis) & 0xffff) * weight;
> 
> Maybe just calculate a weighted distribution table pointing to the
> buckets which you index with 8 bits of the hash.

Nice idea. I think that would work out quite well.

The main question for me would be where to store such a table,
which comes back to my remark above about more storing a more
efficient efficient form of the action.

> > +		if (score >= best_score) {
> > +			best_bucket = bucket;
> > +			best_score = score;
> > +		}
> > +		i++;
> > +	}
> > +
> > +	acts_list = bucket_actions(best_bucket);
> > +
> > +	/* A select group action is always the final action so
> > +	 * there is no need to clone the skb in case of side effects.
> > +	 * Instead just take a reference to it which will be released
> > +	 * by do_execute_actions(). */
> > +	skb_get(skb);
> > +
> > +	return do_execute_actions(dp, skb, nla_data(acts_list),
> > +				  nla_len(acts_list));
> 
> Do we need a recursion limit here?

I believe that is already handled by the depth check that occurs
when the actions are copied.

Re: [PATCH/RFC repost 4/8] datapath: execution of select group action

[Thomas Graf]

On 09/24/14 at 03:01pm, Simon Horman wrote:
> > > +	/* Only possible type of attributes is OVS_SELECT_GROUP_ATTR_BUCKET */
> > > +	for (bucket = nla_data(attr), rem = nla_len(attr); rem > 0;
> > > +	     bucket = nla_next(bucket, &rem)) {
> > > +		uint16_t weight = bucket_weight(bucket);
> > 
> > I think we should validate only once when we copy then assume it is
> > correct.

> That is the intention of this code, is it doing something else?

I must have treated the BUG_ON as validation when I wrote that
comment ;-)

> Nice idea. I think that would work out quite well.
> 
> The main question for me would be where to store such a table,
> which comes back to my remark above about more storing a more
> efficient efficient form of the action.

I had the same issue when working on zone support for the 
conntrack action that is in the works. It needs to keep a ref to
a ct template which is registered with the conntrack engine. I
ended up converting the Netlink attributes to a struct and storing
that in an attribute in sf_acts then converting it back.

You can find the WIP code in the following branch:
https://github.com/tgraf/ovs/tree/nat

datapath: Use central function to free sw_flow_actions 
RFC: Add support for connection tracking. 
datapath: Add zone support

> > Do we need a recursion limit here?
> 
> I believe that is already handled by the depth check that occurs
> when the actions are copied.

Right, I can see it now. Thanks.

Re: [PATCH/RFC repost 5/8] datapath: Move last_action() helper to datapath.h

[Thomas Graf]

On 09/24/14 at 03:00pm, Simon Horman wrote:
> On Fri, Sep 19, 2014 at 03:06:38PM +0100, Thomas Graf wrote:
> > Can we rename & move this to <net/netlink.h> instead?
> 
> Sure, how about nla_is_last()?

Sounds great

Re: [PATCH/RFC repost 5/8] datapath: Move last_action() helper to datapath.h

[Simon Horman]

On Wed, Sep 24, 2014 at 09:20:15AM +0100, Thomas Graf wrote:
> On 09/24/14 at 03:00pm, Simon Horman wrote:
> > On Fri, Sep 19, 2014 at 03:06:38PM +0100, Thomas Graf wrote:
> > > Can we rename & move this to <net/netlink.h> instead?
> > 
> > Sure, how about nla_is_last()?
> 
> Sounds great

Done. I have posted the result as a patch separate to the select group series.

Re: [PATCH/RFC repost 4/8] datapath: execution of select group action

[Simon Horman]

On Wed, Sep 24, 2014 at 09:19:42AM +0100, Thomas Graf wrote:
> On 09/24/14 at 03:01pm, Simon Horman wrote:
> > > > +	/* Only possible type of attributes is OVS_SELECT_GROUP_ATTR_BUCKET */
> > > > +	for (bucket = nla_data(attr), rem = nla_len(attr); rem > 0;
> > > > +	     bucket = nla_next(bucket, &rem)) {
> > > > +		uint16_t weight = bucket_weight(bucket);
> > > 
> > > I think we should validate only once when we copy then assume it is
> > > correct.
> 
> > That is the intention of this code, is it doing something else?
> 
> I must have treated the BUG_ON as validation when I wrote that
> comment ;-)
> 
> > Nice idea. I think that would work out quite well.
> > 
> > The main question for me would be where to store such a table,
> > which comes back to my remark above about more storing a more
> > efficient efficient form of the action.
> 
> I had the same issue when working on zone support for the 
> conntrack action that is in the works. It needs to keep a ref to
> a ct template which is registered with the conntrack engine. I
> ended up converting the Netlink attributes to a struct and storing
> that in an attribute in sf_acts then converting it back.
> 
> You can find the WIP code in the following branch:
> https://github.com/tgraf/ovs/tree/nat
> 
> datapath: Use central function to free sw_flow_actions 
> RFC: Add support for connection tracking. 
> datapath: Add zone support

Thanks, I think something along those lines could work out
quite well for a select group action.

> > > Do we need a recursion limit here?
> > 
> > I believe that is already handled by the depth check that occurs
> > when the actions are copied.
> 
> Right, I can see it now. Thanks.
> 

Re: [PATCH/RFC repost 2/8] netlink: Allow suppression of warnings for duplicate attributes

[Ben Pfaff]

On Thu, Sep 18, 2014 at 10:55:05AM +0900, Simon Horman wrote:
> Add a multiple field to struct nl_policy which if set suppresses
> warning of duplicate attributes in nl_parse_nested().
> 
> As is the case without this patch only the last occurrence of an
> attribute is stored in attrs by nl_parse_nested(). As such
> if the multiple field of struct nl_policy is set then it
> is up to the caller to parse the message to extract all the attributes.
> 
> This is in preparation for allowing multiple OVS_SELECT_GROUP_ATTR_BUCKET
> attributes in a nested OVS_ACTION_ATTR_SELECT_GROUP attribute.
> 
> Signed-off-by: Simon Horman <simon.horman-wFxRvT7yatFl57MIdRCFDg@public.gmane.org>

In the other case where we have duplicate attributes, it doesn't make
sense to process them with the policy functions, because we want to
see all of the instances of the duplicate attributes and policy
doesn't allow us to do that.  I'm a little surprised that the new
attributes work differently.  What's the idea?

Re: [PATCH/RFC repost 7/8] ofproto: translate datapath select group action

[Ben Pfaff]

On Thu, Sep 18, 2014 at 10:55:10AM +0900, Simon Horman wrote:
> This patch is a prototype and has several limitations:
> 
> * It assumes that no actions follow a select group action
>   because the resulting packet after a select group action may
>   differ depending on the bucket used. It may be possible
>   to address this problem using recirculation. Or to not use
>   the datapath select group in such situations. In any case
>   this patch does not solve this problem or even prevent it
>   from occurring.

It seems like this limitation in particular is a pretty big one.  Do
you have a good plan in mind for how to resolve it?

Re: [ovs-dev] [PATCH/RFC repost 7/8] ofproto: translate datapath select group action

[Simon Horman]

On Fri, Sep 26, 2014 at 04:57:25PM -0700, Ben Pfaff wrote:
> On Thu, Sep 18, 2014 at 10:55:10AM +0900, Simon Horman wrote:
> > This patch is a prototype and has several limitations:
> > 
> > * It assumes that no actions follow a select group action
> >   because the resulting packet after a select group action may
> >   differ depending on the bucket used. It may be possible
> >   to address this problem using recirculation. Or to not use
> >   the datapath select group in such situations. In any case
> >   this patch does not solve this problem or even prevent it
> >   from occurring.
> 
> It seems like this limitation in particular is a pretty big one.  Do
> you have a good plan in mind for how to resolve it?

Hi Ben,

it seems to me that this would be somewhat difficult to resolve in the
datapath so I propose not doing so. And I have two ideas on how to
resolve this problem outside of the datapath.

1. Recirculation

   It seems to me that it ought to be possible to handle this by
   recirculating if actions occur after an ODP select group action.

   This could be made slightly more selective by only recirculating
   if the execution different buckets may result in different packet
   contents and the actions after the ODP select group action rely on
   the packet contents (e.g. set actions do but output actions do not).

   My feeling is that this could be implemented by adding a small amount
   of extra state to action translation in ovs-vswitchd.

2. Fall back to selecting buckets in ovs-vswtichd

   The idea here is to detect cases where there would be a problem
   executing actions after an ODP select group action and in that
   case to select buckets in ovs-vswtichd: that is use the existing bucket
   translation code in ovs-vswtichd.

   Though this seems conceptually simpler than recirculation it
   seems to me that it would be somewhat more difficult to implement
   as it implies a two stage translation process: e.g. one stage to
   determine if an ODP select group may be used; and one to perform
   the translation.

   I seem to recall trying various two stage translation processes
   as part some earlier unrelated work. And my recollection is that
   the result of my previous efforts were not pretty.

Both of the above more or less negate any benefits of ODP select group
action. In particular lowering flow setup cost and potentially allowing
complete offload of select groups from the datapath to hardware. However I
think that this case is not a common one as it requires both of the
following. And I think they are both not usual use cases.

* Different buckets modifying packets in different ways
  - My expectation is that it is common for buckets to be homogeneous in
    regards to packet modifications. But perhaps this is naïve in the
    context of VLANs, MPLS, and similar tags that can be pushed and popped.
* Actions that rely on packet contents after
  - My expectation is that it is common to use a select group to output
    packets and that is the final action performed.

Re: [ovs-dev] [PATCH/RFC repost 2/8] netlink: Allow suppression of warnings for duplicate attributes

[Simon Horman]

On Fri, Sep 26, 2014 at 04:55:42PM -0700, Ben Pfaff wrote:
> On Thu, Sep 18, 2014 at 10:55:05AM +0900, Simon Horman wrote:
> > Add a multiple field to struct nl_policy which if set suppresses
> > warning of duplicate attributes in nl_parse_nested().
> > 
> > As is the case without this patch only the last occurrence of an
> > attribute is stored in attrs by nl_parse_nested(). As such
> > if the multiple field of struct nl_policy is set then it
> > is up to the caller to parse the message to extract all the attributes.
> > 
> > This is in preparation for allowing multiple OVS_SELECT_GROUP_ATTR_BUCKET
> > attributes in a nested OVS_ACTION_ATTR_SELECT_GROUP attribute.
> > 
> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> 
> In the other case where we have duplicate attributes, it doesn't make
> sense to process them with the policy functions, because we want to
> see all of the instances of the duplicate attributes and policy
> doesn't allow us to do that.  I'm a little surprised that the new
> attributes work differently.  What's the idea?

My idea was to use the policy to obtain the attributes that
may not be duplicated. And then custom code to pick up all the
instances of attributes that may be duplicated.

I'm don't feel strongly about that approach and I'd be just has
happy to drop this patch and rework things a little so that
all the attributes are picked out by custom code. It sounds
like that would match the approach taken elsewhere. Sorry for
not noticing that earlier.

Re: [ovs-dev] [PATCH/RFC repost 2/8] netlink: Allow suppression of warnings for duplicate attributes

[Ben Pfaff]

On Thu, Oct 09, 2014 at 10:18:32AM +0900, Simon Horman wrote:
> On Fri, Sep 26, 2014 at 04:55:42PM -0700, Ben Pfaff wrote:
> > On Thu, Sep 18, 2014 at 10:55:05AM +0900, Simon Horman wrote:
> > > Add a multiple field to struct nl_policy which if set suppresses
> > > warning of duplicate attributes in nl_parse_nested().
> > > 
> > > As is the case without this patch only the last occurrence of an
> > > attribute is stored in attrs by nl_parse_nested(). As such
> > > if the multiple field of struct nl_policy is set then it
> > > is up to the caller to parse the message to extract all the attributes.
> > > 
> > > This is in preparation for allowing multiple OVS_SELECT_GROUP_ATTR_BUCKET
> > > attributes in a nested OVS_ACTION_ATTR_SELECT_GROUP attribute.
> > > 
> > > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> > 
> > In the other case where we have duplicate attributes, it doesn't make
> > sense to process them with the policy functions, because we want to
> > see all of the instances of the duplicate attributes and policy
> > doesn't allow us to do that.  I'm a little surprised that the new
> > attributes work differently.  What's the idea?
> 
> My idea was to use the policy to obtain the attributes that
> may not be duplicated. And then custom code to pick up all the
> instances of attributes that may be duplicated.
> 
> I'm don't feel strongly about that approach and I'd be just has
> happy to drop this patch and rework things a little so that
> all the attributes are picked out by custom code. It sounds
> like that would match the approach taken elsewhere. Sorry for
> not noticing that earlier.

I see.

That's more like the approach used elsewhere, yes, but in those cases
there wasn't anything (if I recall correctly) that could be usefully
done with the policy parser, so it wasn't considered as an option.  If
the group buckets are different then maybe a different treatment is
warranted.

Re: [ovs-dev] [PATCH/RFC repost 7/8] ofproto: translate datapath select group action

[Ben Pfaff]

On Thu, Oct 09, 2014 at 10:14:36AM +0900, Simon Horman wrote:
> On Fri, Sep 26, 2014 at 04:57:25PM -0700, Ben Pfaff wrote:
> > On Thu, Sep 18, 2014 at 10:55:10AM +0900, Simon Horman wrote:
> > > This patch is a prototype and has several limitations:
> > > 
> > > * It assumes that no actions follow a select group action
> > >   because the resulting packet after a select group action may
> > >   differ depending on the bucket used. It may be possible
> > >   to address this problem using recirculation. Or to not use
> > >   the datapath select group in such situations. In any case
> > >   this patch does not solve this problem or even prevent it
> > >   from occurring.
> > 
> > It seems like this limitation in particular is a pretty big one.  Do
> > you have a good plan in mind for how to resolve it?
> 
> Hi Ben,
> 
> it seems to me that this would be somewhat difficult to resolve in the
> datapath so I propose not doing so. And I have two ideas on how to
> resolve this problem outside of the datapath.
> 
> 1. Recirculation
> 
>    It seems to me that it ought to be possible to handle this by
>    recirculating if actions occur after an ODP select group action.
> 
>    This could be made slightly more selective by only recirculating
>    if the execution different buckets may result in different packet
>    contents and the actions after the ODP select group action rely on
>    the packet contents (e.g. set actions do but output actions do not).
> 
>    My feeling is that this could be implemented by adding a small amount
>    of extra state to action translation in ovs-vswitchd.
> 
> 2. Fall back to selecting buckets in ovs-vswtichd
> 
>    The idea here is to detect cases where there would be a problem
>    executing actions after an ODP select group action and in that
>    case to select buckets in ovs-vswtichd: that is use the existing bucket
>    translation code in ovs-vswtichd.
> 
>    Though this seems conceptually simpler than recirculation it
>    seems to me that it would be somewhat more difficult to implement
>    as it implies a two stage translation process: e.g. one stage to
>    determine if an ODP select group may be used; and one to perform
>    the translation.
> 
>    I seem to recall trying various two stage translation processes
>    as part some earlier unrelated work. And my recollection is that
>    the result of my previous efforts were not pretty.
> 
> Both of the above more or less negate any benefits of ODP select group
> action. In particular lowering flow setup cost and potentially allowing
> complete offload of select groups from the datapath to hardware. However I
> think that this case is not a common one as it requires both of the
> following. And I think they are both not usual use cases.
> 
> * Different buckets modifying packets in different ways
>   - My expectation is that it is common for buckets to be homogeneous in
>     regards to packet modifications. But perhaps this is na??ve in the
>     context of VLANs, MPLS, and similar tags that can be pushed and popped.
> * Actions that rely on packet contents after
>   - My expectation is that it is common to use a select group to output
>     packets and that is the final action performed.

I am glad that you have thought about it.  Your ideas seem like a good
start to me.  Personally, approach #2, of falling back to selecting
buckets in ovs-vswitchd, seems like a clean solution to me, although
if it really takes multiple stages in translation then that is
undesirable, so I hope that some clean and simple approach works out.

I think that we probably need a solution before we can apply the patch
series, because otherwise we end up with half-working code.

Re: [ovs-dev] [PATCH/RFC repost 7/8] ofproto: translate datapath select group action

[Simon Horman]

On Mon, Oct 13, 2014 at 01:46:24PM -0700, Ben Pfaff wrote:
> On Thu, Oct 09, 2014 at 10:14:36AM +0900, Simon Horman wrote:
> > On Fri, Sep 26, 2014 at 04:57:25PM -0700, Ben Pfaff wrote:
> > > On Thu, Sep 18, 2014 at 10:55:10AM +0900, Simon Horman wrote:
> > > > This patch is a prototype and has several limitations:
> > > > 
> > > > * It assumes that no actions follow a select group action
> > > >   because the resulting packet after a select group action may
> > > >   differ depending on the bucket used. It may be possible
> > > >   to address this problem using recirculation. Or to not use
> > > >   the datapath select group in such situations. In any case
> > > >   this patch does not solve this problem or even prevent it
> > > >   from occurring.
> > > 
> > > It seems like this limitation in particular is a pretty big one.  Do
> > > you have a good plan in mind for how to resolve it?
> > 
> > Hi Ben,
> > 
> > it seems to me that this would be somewhat difficult to resolve in the
> > datapath so I propose not doing so. And I have two ideas on how to
> > resolve this problem outside of the datapath.
> > 
> > 1. Recirculation
> > 
> >    It seems to me that it ought to be possible to handle this by
> >    recirculating if actions occur after an ODP select group action.
> > 
> >    This could be made slightly more selective by only recirculating
> >    if the execution different buckets may result in different packet
> >    contents and the actions after the ODP select group action rely on
> >    the packet contents (e.g. set actions do but output actions do not).
> > 
> >    My feeling is that this could be implemented by adding a small amount
> >    of extra state to action translation in ovs-vswitchd.
> > 
> > 2. Fall back to selecting buckets in ovs-vswtichd
> > 
> >    The idea here is to detect cases where there would be a problem
> >    executing actions after an ODP select group action and in that
> >    case to select buckets in ovs-vswtichd: that is use the existing bucket
> >    translation code in ovs-vswtichd.
> > 
> >    Though this seems conceptually simpler than recirculation it
> >    seems to me that it would be somewhat more difficult to implement
> >    as it implies a two stage translation process: e.g. one stage to
> >    determine if an ODP select group may be used; and one to perform
> >    the translation.
> > 
> >    I seem to recall trying various two stage translation processes
> >    as part some earlier unrelated work. And my recollection is that
> >    the result of my previous efforts were not pretty.
> > 
> > Both of the above more or less negate any benefits of ODP select group
> > action. In particular lowering flow setup cost and potentially allowing
> > complete offload of select groups from the datapath to hardware. However I
> > think that this case is not a common one as it requires both of the
> > following. And I think they are both not usual use cases.
> > 
> > * Different buckets modifying packets in different ways
> >   - My expectation is that it is common for buckets to be homogeneous in
> >     regards to packet modifications. But perhaps this is na??ve in the
> >     context of VLANs, MPLS, and similar tags that can be pushed and popped.
> > * Actions that rely on packet contents after
> >   - My expectation is that it is common to use a select group to output
> >     packets and that is the final action performed.
> 
> I am glad that you have thought about it.  Your ideas seem like a good
> start to me.  Personally, approach #2, of falling back to selecting
> buckets in ovs-vswitchd, seems like a clean solution to me, although
> if it really takes multiple stages in translation then that is
> undesirable, so I hope that some clean and simple approach works out.

Thanks. I'll focus on #2 and see how far I can get.

> I think that we probably need a solution before we can apply the patch
> series, because otherwise we end up with half-working code.

Yes, I agree. It needs to be solved before it can be merged.