[ovs-dev] [PATCH 1/2] instruction: support goto-table action

Isaku Yamahata yamahata at valinux.co.jp
Fri Oct 5 06:56:56 UTC 2012


Signed-off-by: Isaku Yamahata <yamahata at valinux.co.jp>
---
v6
- manual rebase
- simplify logic
- remove all zero check
- update unit tests

v5
- manual rebase

v4
- squashed goto-table instruction part into single patch
- only introduce goto-table. Other instruction will be addressed
  by other patches.
- check zero padding
- man page
- unit test

v3
- introduce OFPACT_{CLEAR_ACTIONS, WRITE_ACTIONS, GOTO_TABLE}

v2
- changed for ofp_instruction
---
 lib/ofp-actions.c        |   89 +++++++++++++++++++++++++++-----
 lib/ofp-actions.h        |   21 +++++++-
 lib/ofp-parse.c          |  128 +++++++++++++++++++++++++++++++++++++++-------
 ofproto/ofproto-dpif.c   |    9 ++++
 tests/ofp-actions.at     |   23 +++++++--
 utilities/ovs-ofctl.8.in |    9 ++++
 6 files changed, 245 insertions(+), 34 deletions(-)

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index a58f8db..0370a31 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -958,9 +958,20 @@ ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
             goto exit;
         }
     }
+    /* TODO:XXX Clear-Actions */
+    /* TODO:XXX Write-Actions */
+    /* TODO:XXX Write-Metadata */
+    if (insts[OVSINST_OFPIT11_GOTO_TABLE]) {
+        const struct ofp11_instruction_goto_table *oigt;
+        struct ofpact_goto_table *ogt;
+
+        oigt = instruction_get_OFPIT11_GOTO_TABLE(
+            insts[OVSINST_OFPIT11_GOTO_TABLE]);
+        ogt = ofpact_put_GOTO_TABLE(ofpacts);
+        ogt->table_id = oigt->table_id;
+    }
 
-    if (insts[OVSINST_OFPIT11_GOTO_TABLE] ||
-        insts[OVSINST_OFPIT11_WRITE_METADATA] ||
+    if (insts[OVSINST_OFPIT11_WRITE_METADATA] ||
         insts[OVSINST_OFPIT11_WRITE_ACTIONS] ||
         insts[OVSINST_OFPIT11_CLEAR_ACTIONS]) {
         error = OFPERR_OFPBIC_UNSUP_INST;
@@ -1040,6 +1051,9 @@ ofpact_check__(const struct ofpact *a, const struct flow *flow, int max_ports)
     case OFPACT_EXIT:
         return 0;
 
+    case OFPACT_GOTO_TABLE:
+        return 0;
+
     default:
         NOT_REACHED();
     }
@@ -1255,6 +1269,7 @@ ofpact_to_nxast(const struct ofpact *a, struct ofpbuf *out)
     case OFPACT_SET_IPV4_DSCP:
     case OFPACT_SET_L4_SRC_PORT:
     case OFPACT_SET_L4_DST_PORT:
+    case OFPACT_GOTO_TABLE:
         NOT_REACHED();
     }
 }
@@ -1344,6 +1359,10 @@ ofpact_to_openflow10(const struct ofpact *a, struct ofpbuf *out)
             = htons(ofpact_get_SET_L4_DST_PORT(a)->port);
         break;
 
+    case OFPACT_GOTO_TABLE:
+        /* TODO:XXX */
+        break;
+
     case OFPACT_CONTROLLER:
     case OFPACT_OUTPUT_REG:
     case OFPACT_BUNDLE:
@@ -1452,6 +1471,9 @@ ofpact_to_openflow11(const struct ofpact *a, struct ofpbuf *out)
             = htons(ofpact_get_SET_L4_DST_PORT(a)->port);
         break;
 
+    case OFPACT_GOTO_TABLE:
+        NOT_REACHED();
+
     case OFPACT_CONTROLLER:
     case OFPACT_OUTPUT_REG:
     case OFPACT_BUNDLE:
@@ -1490,18 +1512,10 @@ ofpacts_put_openflow11_actions(const struct ofpact ofpacts[],
     return openflow->size - start_size;
 }
 
-void
-ofpacts_put_openflow11_instructions(const struct ofpact ofpacts[],
-                                    size_t ofpacts_len,
-                                    struct ofpbuf *openflow)
+static void
+ofpacts_update_instruction_actions(struct ofpbuf *openflow, size_t ofs)
 {
     struct ofp11_instruction_actions *oia;
-    size_t ofs;
-
-    /* Put an OFPIT11_APPLY_ACTIONS instruction and fill it in. */
-    ofs = openflow->size;
-    instruction_put_OFPIT11_APPLY_ACTIONS(openflow);
-    ofpacts_put_openflow11_actions(ofpacts, ofpacts_len, openflow);
 
     /* Update the instruction's length (or, if it's empty, delete it). */
     oia = ofpbuf_at_assert(openflow, ofs, sizeof *oia);
@@ -1511,6 +1525,45 @@ ofpacts_put_openflow11_instructions(const struct ofpact ofpacts[],
         openflow->size = ofs;
     }
 }
+
+void
+ofpacts_put_openflow11_instructions(const struct ofpact ofpacts[],
+                                    size_t ofpacts_len,
+                                    struct ofpbuf *openflow)
+{
+    const struct ofpact *a;
+
+    OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
+        /* TODO:XXX Clear-Actions */
+        /* TODO:XXX Write-Actions */
+        /* TODO:XXX Write-Metadata */
+        if (a->type == OFPACT_GOTO_TABLE) {
+            struct ofp11_instruction_goto_table *oigt;
+
+            oigt = instruction_put_OFPIT11_GOTO_TABLE(openflow);
+            oigt->table_id = ofpact_get_GOTO_TABLE(a)->table_id;
+            memset(oigt->pad, 0, sizeof oigt->pad);
+        } else if (!ofpact_is_instruction(a)) {
+            /* Apply-actions */
+            const size_t ofs = openflow->size;
+            const size_t ofpacts_len_left =
+                (uint8_t*)ofpact_end(ofpacts, ofpacts_len) - (uint8_t*)a;
+            const struct ofpact *action;
+            const struct ofpact *processed = a;
+
+            instruction_put_OFPIT11_APPLY_ACTIONS(openflow);
+            OFPACT_FOR_EACH(action, a, ofpacts_len_left) {
+                if (ofpact_is_instruction(action)) {
+                    break;
+                }
+                ofpact_to_openflow11(action, openflow);
+                processed = action;
+            }
+            ofpacts_update_instruction_actions(openflow, ofs);
+            a = processed;
+        }
+    }
+}
 
 /* Returns true if 'action' outputs to 'port', false otherwise. */
 static bool
@@ -1549,6 +1602,7 @@ ofpact_outputs_to_port(const struct ofpact *ofpact, uint16_t port)
     case OFPACT_AUTOPATH:
     case OFPACT_NOTE:
     case OFPACT_EXIT:
+    case OFPACT_GOTO_TABLE:
     default:
         return false;
     }
@@ -1815,6 +1869,13 @@ ofpact_format(const struct ofpact *a, struct ds *s)
     case OFPACT_EXIT:
         ds_put_cstr(s, "exit");
         break;
+
+    case OFPACT_GOTO_TABLE:
+        ds_put_format(s, "%s:%"PRIu8,
+                      ofpact_instruction_name_from_type(
+                          OVSINST_OFPIT11_GOTO_TABLE),
+                      ofpact_get_GOTO_TABLE(a)->table_id);
+        break;
     }
 }
 
@@ -1834,6 +1895,10 @@ ofpacts_format(const struct ofpact *ofpacts, size_t ofpacts_len,
             if (a != ofpacts) {
                 ds_put_cstr(string, ",");
             }
+
+            /* TODO:XXX clear-actions */
+            /* TODO:XXX write-actions */
+            /* TODO:XXX write-metadata */
             ofpact_format(a, string);
         }
     }
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index fd53e62..a811167 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -87,7 +87,11 @@
                                                                     \
     /* Other. */                                                    \
     DEFINE_OFPACT(NOTE,            ofpact_note,          data)      \
-    DEFINE_OFPACT(EXIT,            ofpact_null,          ofpact)
+    DEFINE_OFPACT(EXIT,            ofpact_null,          ofpact)    \
+                                                                    \
+    /* Instructions */                                              \
+    /* TODO:XXX Clear-Actions, Write-Actions, Write-Metadata */     \
+    DEFINE_OFPACT(GOTO_TABLE,      ofpact_goto_table,    ofpact)
 
 /* enum ofpact_type, with a member OFPACT_<ENUM> for each action. */
 enum OVS_PACKED_ENUM ofpact_type {
@@ -413,7 +417,14 @@ struct ofpact_cnt_ids {
     /* Controller ids. */
     unsigned int n_controllers;
     uint16_t cnt_ids[];
+};
 
+/* OFPACT_GOTO_TABLE
+ *
+ * Used for OFPIT11_GOTO_TABLE */
+struct ofpact_goto_table {
+    struct ofpact ofpact;
+    uint8_t table_id;
 };
 
 /* Converting OpenFlow to ofpacts. */
@@ -566,6 +577,14 @@ enum {
 #undef DEFINE_INST
 };
 
+
+static inline bool
+ofpact_is_instruction(const struct ofpact *a)
+{
+    /* TODO:XXX Clear-Actions, Write-Actions, Write-Metadata */
+    return a->type == OFPACT_GOTO_TABLE;
+}
+
 const char *ofpact_instruction_name_from_type(enum ovs_instruction_type type);
 int ofpact_instruction_type_from_name(const char *name);
 
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index 2d2daa4..557c95d 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -520,6 +520,34 @@ parse_named_action(enum ofputil_action_code code, const struct flow *flow,
     }
 }
 
+static bool
+str_to_ofpact__(const struct flow *flow, char *pos, char *act, char *arg,
+                struct ofpbuf *ofpacts, int n_actions)
+{
+    int code = ofputil_action_code_from_name(act);
+    if (code >= 0) {
+        parse_named_action(code, flow, arg, ofpacts);
+    } else if (!strcasecmp(act, "drop")) {
+        if (n_actions) {
+            ovs_fatal(0, "Drop actions must not be preceded by other "
+                      "actions");
+        } else if (ofputil_parse_key_value(&pos, &act, &arg)) {
+            ovs_fatal(0, "Drop actions must not be followed by other "
+                      "actions");
+        }
+        return false;
+    } else {
+        uint16_t port = ofputil_port_from_string(act);
+        if (port) {
+            ofpact_put_OUTPUT(ofpacts)->port = port;
+        } else {
+            ovs_fatal(0, "Unknown action: %s", act);
+        }
+    }
+
+    return true;
+}
+
 static void
 str_to_ofpacts(const struct flow *flow, char *str, struct ofpbuf *ofpacts)
 {
@@ -529,26 +557,90 @@ str_to_ofpacts(const struct flow *flow, char *str, struct ofpbuf *ofpacts)
     pos = str;
     n_actions = 0;
     while (ofputil_parse_key_value(&pos, &act, &arg)) {
-        int code = ofputil_action_code_from_name(act);
-        if (code >= 0) {
-            parse_named_action(code, flow, arg, ofpacts);
-        } else if (!strcasecmp(act, "drop")) {
-            if (n_actions) {
-                ovs_fatal(0, "Drop actions must not be preceded by other "
-                          "actions");
-            } else if (ofputil_parse_key_value(&pos, &act, &arg)) {
-                ovs_fatal(0, "Drop actions must not be followed by other "
-                          "actions");
-            }
+        if (!str_to_ofpact__(flow, pos, act, arg, ofpacts, n_actions)) {
             break;
-        } else {
-            uint16_t port = ofputil_port_from_string(act);
-            if (port) {
-                ofpact_put_OUTPUT(ofpacts)->port = port;
-            } else {
-                ovs_fatal(0, "Unknown action: %s", act);
+        }
+        n_actions++;
+    }
+    ofpact_pad(ofpacts);
+}
+
+static void
+parse_named_instruction(enum ovs_instruction_type type,
+                        char *arg, struct ofpbuf *ofpacts)
+{
+    switch (type) {
+    case OVSINST_OFPIT11_APPLY_ACTIONS:
+        NOT_REACHED();  /* This case is handled by str_to_inst_ofpacts() */
+        break;
+
+    case OVSINST_OFPIT11_WRITE_ACTIONS:
+        /* TODO:XXX */
+        ovs_fatal(0, "instruction write-actions is not supported yet");
+        break;
+
+    case OVSINST_OFPIT11_CLEAR_ACTIONS:
+        /* TODO:XXX */
+        ovs_fatal(0, "instruction clear-actions is not supported yet");
+        break;
+
+    case OVSINST_OFPIT11_WRITE_METADATA:
+        /* TODO:XXX */
+        ovs_fatal(0, "instruction write-metadata is not supported yet");
+        break;
+
+    case OVSINST_OFPIT11_GOTO_TABLE: {
+        struct ofpact_goto_table *ogt = ofpact_put_GOTO_TABLE(ofpacts);
+        char *table_s = strsep(&arg, ",");
+        if (!table_s || !table_s[0]) {
+            ovs_fatal(0, "instruction goto-table needs table id");
+        }
+        ogt->table_id = str_to_table_id(table_s);
+        break;
+    }
+    }
+}
+
+static void
+str_to_inst_ofpacts(const struct flow *flow, char *str, struct ofpbuf *ofpacts)
+{
+    char *pos, *inst, *arg;
+    int type;
+    const char *prev_inst = NULL;
+    int prev_type = -1;
+    int n_actions = 0;
+
+    pos = str;
+    while (ofputil_parse_key_value(&pos, &inst, &arg)) {
+        type = ofpact_instruction_type_from_name(inst);
+        if (type < 0) {
+            if (!str_to_ofpact__(flow, pos, inst, arg, ofpacts, n_actions)) {
+                break;
+            }
+
+            type = OVSINST_OFPIT11_APPLY_ACTIONS;
+            if (prev_type == type) {
+                n_actions++;
+                continue;
             }
+        } else if (type == OVSINST_OFPIT11_APPLY_ACTIONS) {
+            ovs_fatal(0, "%s isn't supported. Just write actions then "
+                      "it is interpreted as apply_actions", inst);
+        } else {
+            parse_named_instruction(type, arg, ofpacts);
+        }
+
+        if (type == prev_type) {
+            ovs_fatal(0, "instruction can be specified at most once: %s",
+                      inst);
         }
+        if (type <= prev_type) {
+            ovs_fatal(0, "Instruction %s must be specified before %s",
+                      inst, prev_inst);
+        }
+
+        prev_inst = inst;
+        prev_type = type;
         n_actions++;
     }
     ofpact_pad(ofpacts);
@@ -777,7 +869,7 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
         struct ofpbuf ofpacts;
 
         ofpbuf_init(&ofpacts, 32);
-        str_to_ofpacts(&fm->match.flow, act_str, &ofpacts);
+        str_to_inst_ofpacts(&fm->match.flow, act_str, &ofpacts);
         fm->ofpacts_len = ofpacts.size;
         fm->ofpacts = ofpbuf_steal_data(&ofpacts);
     } else {
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index f73ca9d..68bfc0a 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5598,6 +5598,15 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
             ctx->has_fin_timeout = true;
             xlate_fin_timeout(ctx, ofpact_get_FIN_TIMEOUT(a));
             break;
+
+        case OFPACT_GOTO_TABLE: {
+            /* TODO:XXX remove recursion */
+            /* It is assumed that goto-table is last action */
+            struct ofpact_goto_table *ogt = ofpact_get_GOTO_TABLE(a);
+            assert(ctx->table_id < ogt->table_id);
+            xlate_table_action(ctx, ctx->flow.in_port, ogt->table_id, true);
+            break;
+        }
         }
     }
 
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 7ba9277..3e86841 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -243,12 +243,24 @@ AT_CHECK(
 AT_CLEANUP
 
 AT_SETUP([OpenFlow 1.1 instruction translation])
-AT_KEYWORDS([OF1.1])
+AT_KEYWORDS([OF1.1 instruction])
 AT_DATA([test-data], [dnl
 # actions=LOCAL
 0004 0018 00000000 dnl
 0000 0010 fffffffe 04d2 000000000000
 
+dnl Apply-Actions non-zero padding
+# actions=drop
+#  0: 00 -> (none)
+#  1: 04 -> (none)
+#  2: 00 -> (none)
+#  3: 08 -> (none)
+#  4: 00 -> (none)
+#  5: 00 -> (none)
+#  6: 00 -> (none)
+#  7: 01 -> (none)
+0004 0008 00000001
+
 dnl Check that an empty Apply-Actions instruction gets dropped.
 # actions=drop
 #  0: 00 -> (none)
@@ -274,8 +286,13 @@ dnl Goto-Table instruction too long.
 # bad OF1.1 instructions: OFPBIC_BAD_LEN
 0001 0010 01 000000 0000000000000000
 
-dnl Goto-Table not supported yet.
-# bad OF1.1 instructions: OFPBIC_UNSUP_INST
+dnl Goto-Table 1 instruction non-zero padding
+# actions=goto_table:1
+#  7: 01 -> 00
+0001 0008 01 000001
+
+dnl Goto-Table 1
+# actions=goto_table:1
 0001 0008 01 000000
 
 dnl Write-Metadata not supported yet.
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 4f62c66..eb6fb4d 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -1140,6 +1140,15 @@ possibly for a lowest-priority ``catch-all'' flow, that is, a flow
 with no match criteria.  (This is why the default \fBtable\fR is 1, to
 keep the learned flows separate from the primary flow table 0.)
 .
+.RS
+.IP \fBapply_actions(\fR[\fIaction\fR][\fB,\fIaction\fR...]\fB)
+Applies the specific action(s) immediately. The syntax of actions are same
+to \fBactions=\fR field.
+.
+.IP \fBgoto_table\fR:\fItable\fR
+Indicates the next table in the process pipeline.
+.RE
+.
 .IP "\fBfin_timeout(\fIargument\fR[\fB,\fIargument\fR]\fB)"
 This action changes the idle timeout or hard timeout, or both, of this
 OpenFlow rule when the rule matches a TCP packet with the FIN or RST
-- 
1.7.10.4




More information about the dev mailing list