[ovs-dev] [PATCH] ofproto: Add relaxed group_mod command ADD_OR_MOD

[Jan Scheurich]

This patch adds support for a new Group Mod command OFPGC_ADD_OR_MOD to OVS
for all OpenFlow versions supporting groups (OF11 and higher). The new
ADD_OR_MOD creates a group that does not yet exist (like ADD) or modifies
an existing group (like MODIFY).

Rational: In OpenFlow 1.x the Group Mod commands OFPGC_ADD and OFPGC_MODIFY
have strict semantics: ADD fails if the group exists, while MODIFY fails if
the group does not exist. This requires a controller to exactly know the
state of the switch when programming a group in order not run the risk of
getting an OFP Error message in response. This is hard to achieve and
maintain at all times in view of possible switch and controller restarts or
other connection losses between switch and controller.

Due to the un-acknowledged nature of the Group Mod message programming groups
safely and efficiently at the same time is virtually impossible as the
controller has to either query the existence of the group prior to each Group
Mod message or to insert a Barrier Request/Reply after every group to be sure
that no Error can be received at a later stage and require a complicated
roll-back of any dependent actions taken between the failed Group Mod and the
Error.

In the ovs-ofctl command line the new Group Mod command is made available in
form of new write-group(s) commands that can be used in the same way as
add-group(s) but do not lead to an error when a groups exists:

ovs-ofctl -Oopenflow13 del-groups br-int group_id=100

ovs-ofctl -Oopenflow13 write-group br-int group_id=100,type=indirect,bucket=actions=2
ovs-ofctl -Oopenflow13 dump-groups br-int
OFPST_GROUP_DESC reply (OF1.3) (xid=0x2):
group_id=100,type=indirect,bucket=actions=output:2

ovs-ofctl -Oopenflow13 write-group br-int group_id=100,type=indirect,bucket=actions=3
ovs-ofctl -Oopenflow13 dump-groups br-int
OFPST_GROUP_DESC reply (OF1.3) (xid=0x2):
group_id=100,type=indirect,bucket=actions=output:3


Signed-off-by: Jan Scheurich <jan.scheurich at web.de>

To aid review this patch is available at:

tree: https://github.com/JScheurich/openvswitch
branch: dev/group_add_modify
tag: group_add_modify_v1

---

include/openflow/openflow-1.1.h | 1 +
include/openflow/openflow-1.5.h | 1 +
ofproto/ofproto.c | 25 +++++++++++++++++++++++++
lib/ofp-parse.c | 4 ++++
lib/ofp-print.c | 4 ++++
lib/ofp-util.c | 10 +++++++++-
utilities/ovs-ofctl.c | 16 ++++++++++++++++
7 files changed, 60 insertions(+), 1 deletion(-)


diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h
index 805f222..de28475
--- a/include/openflow/openflow-1.1.h
+++ b/include/openflow/openflow-1.1.h
@@ -172,6 +172,7 @@ enum ofp11_group_mod_command {
OFPGC11_ADD, /* New group. */
OFPGC11_MODIFY, /* Modify all matching groups. */
OFPGC11_DELETE, /* Delete all matching groups. */
+ OFPGC11_ADD_OR_MOD = 0x8000, /* Create new or modify existing group. */
};

/* OpenFlow 1.1 specific capabilities supported by the datapath (struct

diff --git a/include/openflow/openflow-1.5.h b/include/openflow/openflow-1.5.h
index 0c478d1..3649e6c
--- a/include/openflow/openflow-1.5.h
+++ b/include/openflow/openflow-1.5.h
@@ -59,6 +59,7 @@ enum ofp15_group_mod_command {
/* OFPGCXX_YYY = 4, */ /* Reserved for future use. */
OFPGC15_REMOVE_BUCKET = 5,/* Remove all action buckets or any specific
action bucket from matching group */
+ OFPGC15_ADD_OR_MOD = 0x8000, /* Create new or modify existing group. */
};

/* Group bucket property types. */

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index ae527a4..c7bd5aa
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -6612,6 +6612,27 @@ out:
return error;
}

+/* Implements the OFPGC11_ADD_OR_MOD command which creates the group when it does not
+ * exist yet and modifies it otherwise */
+static enum ofperr
+add_or_modify_group(struct ofproto *ofproto, const struct ofputil_group_mod *gm)
+{
+ struct ofgroup *ofgroup;
+ enum ofperr error;
+ bool exists;
+
+ ovs_rwlock_rdlock(&ofproto->groups_rwlock);
+ exists = ofproto_group_lookup__(ofproto, gm->group_id, &ofgroup);
+ ovs_rwlock_unlock(&ofproto->groups_rwlock);
+
+ if (!exists) {
+ error = add_group(ofproto, gm);
+ } else {
+ error = modify_group(ofproto, gm);
+ }
+ return error;
+}
+
static void
delete_group__(struct ofproto *ofproto, struct ofgroup *ofgroup)
OVS_RELEASES(ofproto->groups_rwlock)
@@ -6701,6 +6722,10 @@ handle_group_mod(struct ofconn *ofconn, const struct ofp_header *oh)
error = modify_group(ofproto, &gm);
break;

+ case OFPGC11_ADD_OR_MOD:
+ error = add_or_modify_group(ofproto, &gm);
+ break;
+
case OFPGC11_DELETE:
delete_group(ofproto, gm.group_id);
error = 0;

diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 71c5cdf..4af6d9b
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -1380,6 +1380,10 @@ parse_ofp_group_mod_str__(struct ofputil_group_mod *gm, uint16_t command,
fields = F_GROUP_TYPE | F_BUCKETS;
break;

+ case OFPGC11_ADD_OR_MOD:
+ fields = F_GROUP_TYPE | F_BUCKETS;
+ break;
+
case OFPGC15_INSERT_BUCKET:
fields = F_BUCKETS | F_COMMAND_BUCKET_ID;
*usable_protocols &= OFPUTIL_P_OF15_UP;

diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index b21d76f..0627fbb
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2673,6 +2673,10 @@ ofp_print_group_mod__(struct ds *s, enum ofp_version ofp_version,
ds_put_cstr(s, "MOD");
break;

+ case OFPGC11_ADD_OR_MOD:
+ ds_put_cstr(s, "ADD_OR_MOD");
+ break;
+
case OFPGC11_DELETE:
ds_put_cstr(s, "DEL");
break;

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 2c6fb1f..9b76cad
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -8787,6 +8787,7 @@ parse_group_prop_ntr_selection_method(struct ofpbuf *payload,
switch (group_cmd) {
case OFPGC15_ADD:
case OFPGC15_MODIFY:
+ case OFPGC15_ADD_OR_MOD:
break;
case OFPGC15_DELETE:
case OFPGC15_INSERT_BUCKET:
@@ -9115,6 +9116,7 @@ bad_group_cmd(enum ofp15_group_mod_command cmd)
switch (cmd) {
case OFPGC15_ADD:
case OFPGC15_MODIFY:
+ case OFPGC15_ADD_OR_MOD:
case OFPGC15_DELETE:
version = "1.1";
opt_version = "11";
@@ -9139,6 +9141,10 @@ bad_group_cmd(enum ofp15_group_mod_command cmd)
cmd_str = "mod-group";
break;

+ case OFPGC15_ADD_OR_MOD:
+ cmd_str = "write-group";
+ break;
+
case OFPGC15_DELETE:
cmd_str = "del-group";
break;
@@ -9175,7 +9181,7 @@ ofputil_encode_group_mod(enum ofp_version ofp_version,
case OFP12_VERSION:
case OFP13_VERSION:
case OFP14_VERSION:
- if (gm->command > OFPGC11_DELETE) {
+ if (gm->command > OFPGC11_DELETE && gm->command != OFPGC11_ADD_OR_MOD) {
bad_group_cmd(gm->command);
}
return ofputil_encode_ofp11_group_mod(ofp_version, gm);
@@ -9246,6 +9252,7 @@ ofputil_pull_ofp15_group_mod(struct ofpbuf *msg, enum ofp_version ofp_version,

case OFPGC11_ADD:
case OFPGC11_MODIFY:
+ case OFPGC11_ADD_OR_MOD:
case OFPGC11_DELETE:
default:
if (gm->command_bucket_id == OFPG15_BUCKET_ALL) {
@@ -9324,6 +9331,7 @@ ofputil_decode_group_mod(const struct ofp_header *oh,
switch (gm->command) {
case OFPGC11_ADD:
case OFPGC11_MODIFY:
+ case OFPGC11_ADD_OR_MOD:
case OFPGC11_DELETE:
case OFPGC15_INSERT_BUCKET:
break;

diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index a8116d9..802b722
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -2524,6 +2524,18 @@ ofctl_mod_group(struct ovs_cmdl_context *ctx)
}

static void
+ofctl_add_mod_group(struct ovs_cmdl_context *ctx)
+{
+ ofctl_group_mod(ctx->argc, ctx->argv, OFPGC11_ADD_OR_MOD);
+}
+
+static void
+ofctl_add_mod_groups(struct ovs_cmdl_context *ctx)
+{
+ ofctl_group_mod_file(ctx->argc, ctx->argv, OFPGC11_ADD_OR_MOD);
+}
+
+static void
ofctl_del_groups(struct ovs_cmdl_context *ctx)
{
ofctl_group_mod(ctx->argc, ctx->argv, OFPGC11_DELETE);
@@ -4038,6 +4050,10 @@ static const struct ovs_cmdl_command all_commands[] = {
1, 2, ofctl_add_groups },
{ "mod-group", "switch group",
1, 2, ofctl_mod_group },
+ { "write-group", "switch group",
+ 1, 2, ofctl_add_mod_group },
+ { "write-groups", "switch group",
+ 1, 2, ofctl_add_mod_groups },
{ "del-groups", "switch [group]",
1, 2, ofctl_del_groups },
{ "insert-buckets", "switch [group]",