[ovs-dev] [PATCH 6/9] More compact ofpact_set_field.

Jarno Rajahalme jrajahalme at nicira.com
Wed Oct 16 23:16:08 UTC 2013


Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
---
 lib/meta-flow.h              |    2 +-
 lib/ofp-actions.c            |   44 +++++++++++++++++++++++++++-----------
 lib/ofp-actions.h            |   48 +++++++++++++++++++++++++++++++++++++-----
 lib/ofp-parse.c              |   14 ++++++------
 lib/ofp-util.c               |   12 +++++------
 ofproto/ofproto-dpif-xlate.c |    5 +++--
 6 files changed, 91 insertions(+), 34 deletions(-)

diff --git a/lib/meta-flow.h b/lib/meta-flow.h
index 03d7794..29dbe48 100644
--- a/lib/meta-flow.h
+++ b/lib/meta-flow.h
@@ -31,7 +31,7 @@ struct match;
 
 /* The comment on each of these indicates the member in "union mf_value" used
  * to represent its value. */
-enum mf_field_id {
+enum OVS_PACKED_ENUM mf_field_id {
     /* Metadata. */
     MFF_TUN_ID,                 /* be64 */
     MFF_TUN_SRC,                /* be32 */
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index c666069..8c9dd8c 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -744,7 +744,6 @@ oxm_set_field_from_openflow12(
     uint8_t oxm_length = NXM_LENGTH(oxm_header);
     struct ofpact_set_field *sf;
     const struct mf_field *mf;
-    size_t prune;
 
     /* ofp12_action_set_field is padded to 64 bits by zero */
     if (oasf_len != ROUND_UP(sizeof(*oasf) + oxm_length, 8)) {
@@ -770,15 +769,9 @@ oxm_set_field_from_openflow12(
         return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
     }
 
-    sf = ofpact_put_SET_FIELD(ofpacts);
-    sf->field = mf->id;
+    sf = ofpact_set_field_from_be_value(mf, oasf + 1, ofpacts);
 
-    memcpy(&sf->value.u8, oasf + 1, mf->n_bytes);
-    prune = sizeof(union mf_value) - mf->n_bytes;
-    sf->ofpact.len -= prune;
-    ofpacts->size -= ROUND_DOWN(prune, OFPACT_ALIGNTO);
-
-    if (!mf_is_value_valid(mf, &sf->value)) {
+    if (!mf_is_value_valid(mf, ofpact_set_field_get_const_value(sf))) {
         return OFPERR_OFPBAC_BAD_SET_ARGUMENT;
     }
     return 0;
@@ -1582,7 +1575,7 @@ ofpact_check__(const struct ofpact *a, struct flow *flow, ofp_port_t max_ports,
         return nxm_reg_load_check(ofpact_get_REG_LOAD(a), flow);
 
     case OFPACT_SET_FIELD:
-        mf = mf_from_id(ofpact_get_SET_FIELD(a)->field);
+        mf = ofpact_set_field_get_field(ofpact_get_SET_FIELD(a));
         if (!mf_are_prereqs_ok(mf, flow)) {
             VLOG_WARN_RL(&rl, "set_field %s lacks correct prerequisites",
                          mf->name);
@@ -2717,9 +2710,9 @@ ofpact_format(const struct ofpact *a, struct ds *s)
 
     case OFPACT_SET_FIELD:
         set_field = ofpact_get_SET_FIELD(a);
-        mf = mf_from_id(set_field->field);
+        mf = ofpact_set_field_get_field(set_field);
         ds_put_format(s, "set_field:");
-        mf_format(mf, &set_field->value, NULL, s);
+        mf_format(mf, ofpact_set_field_get_const_value(set_field), NULL, s);
         ds_put_format(s, "->%s", mf->name);
         break;
 
@@ -2940,3 +2933,30 @@ ofpact_pad(struct ofpbuf *ofpacts)
         ofpbuf_put_zeros(ofpacts, OFPACT_ALIGNTO - rem);
     }
 }
+
+/* Depends on the network byte order 'value' having the size 'mf->n_bytes'. */
+struct ofpact_set_field *
+ofpact_set_field_from_be_value(const struct mf_field *mf, const void *value,
+                               struct ofpbuf *ofpacts)
+{
+    struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts);
+    size_t offset;
+
+    /* Set the field type. */
+    sf->ofpact.field = mf->id;
+
+    /* Get the data offset depending on the data size. */
+    offset = (mf->n_bytes <= 4)
+        ? offsetof(struct ofpact_set_field, be32)
+        : offsetof(struct ofpact_set_field, be64);
+
+    /* Trim the ofpact of extra trailing space. */
+    sf->ofpact.len = offset + mf->n_bytes;
+    ofpacts->size -= ROUND_DOWN(sizeof *sf - sf->ofpact.len, OFPACT_ALIGNTO);
+
+    /* Copy the value. */
+    memcpy((char *)sf + offset, value, mf->n_bytes);
+
+    /* Any padding left was cleared by ofpacts_put_SET_FIELD(). */
+    return sf;
+}
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 128e2ab..b97d14b 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -154,7 +154,10 @@ enum {
  */
 struct ofpact {
     enum ofpact_type type;      /* OFPACT_*. */
-    enum ofputil_action_code compat; /* Original type when added, if any. */
+    union {
+        enum ofputil_action_code compat; /* Orig. type when added, if any. */
+        enum mf_field_id field;   /* Target field, used by set field only. */
+    };
     uint16_t len;               /* Length of the action, in bytes, including
                                  * struct ofpact, excluding padding. */
 };
@@ -355,11 +358,46 @@ struct ofpact_reg_load {
  * never referenced.  'ofpact.len' tells the correct size, and is never
  * less than needed for the field in question. */
 struct ofpact_set_field {
-    struct ofpact ofpact;
-    enum mf_field_id field;
-    union mf_value value; /* Most-significant bits are used, the rest
-                           * might not be here. */
+    struct ofpact ofpact;  /* has enum mf_field_id field */
+    union {
+        uint8_t u8;
+        ovs_be16 be16;
+        ovs_be32 be32;
+    };
+    union {
+        uint8_t mac[ETH_ADDR_LEN];
+        ovs_be64 be64;
+        struct in6_addr ipv6;
+    };
 };
+#ifdef __GNUC__
+BUILD_ASSERT_DECL(sizeof(struct ofpact_set_field) == 24);
+#endif
+
+static inline const struct mf_field *
+ofpact_set_field_get_field(const struct ofpact_set_field *sf)
+{
+    return mf_from_id(sf->ofpact.field);
+}
+
+static inline enum mf_field_id
+ofpact_set_field_get_field_id(const struct ofpact_set_field *sf)
+{
+    return sf->ofpact.field;
+}
+
+/* Return the data pointer for a properly trimmed set field. */
+static inline const void *
+ofpact_set_field_get_const_value(const struct ofpact_set_field *sf)
+{
+    return (const char *)sf + sf->ofpact.len
+        - ofpact_set_field_get_field(sf)->n_bytes;
+}
+
+struct ofpact_set_field *
+ofpact_set_field_from_be_value(const struct mf_field *,
+                               const void *value,
+                               struct ofpbuf *ofpacts);
 
 /* OFPACT_PUSH_VLAN/MPLS/PBB
  *
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index bf7f56f..e06d1dd 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -464,13 +464,13 @@ static char * WARN_UNUSED_RESULT
 set_field_parse__(char *arg, struct ofpbuf *ofpacts,
                   enum ofputil_protocol *usable_protocols)
 {
-    struct ofpact_set_field *sf = ofpact_put_SET_FIELD(ofpacts);
+    struct ofpact_set_field *sf;
     char *value;
     char *delim;
     char *key;
     const struct mf_field *mf;
     char *error;
-    size_t prune;
+    union mf_value mf_value;
 
     value = arg;
     delim = strstr(arg, "->");
@@ -489,17 +489,15 @@ set_field_parse__(char *arg, struct ofpbuf *ofpacts,
     if (!mf->writable) {
         return xasprintf("%s is read-only", key);
     }
-    sf->field = mf->id;
     delim[0] = '\0';
-    error = mf_parse_value(mf, value, &sf->value);
+    error = mf_parse_value(mf, value, &mf_value);
     if (error) {
         return error;
     }
-    prune = sizeof(union mf_value) - mf->n_bytes;
-    sf->ofpact.len -= prune;
-    ofpacts->size -= ROUND_DOWN(prune, OFPACT_ALIGNTO);
 
-    if (!mf_is_value_valid(mf, &sf->value)) {
+    sf = ofpact_set_field_from_be_value(mf, &mf_value, ofpacts);
+
+    if (!mf_is_value_valid(mf, ofpact_set_field_get_const_value(sf))) {
         return xasprintf("%s is not a valid value for field %s", value, key);
     }
 
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index f689d1d..c9d42a9 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -6137,7 +6137,7 @@ static void
 set_field_to_ofast(const struct ofpact_set_field *sf,
                    struct ofpbuf *openflow)
 {
-    const struct mf_field *mf = mf_from_id(sf->field);
+    const struct mf_field *mf = ofpact_set_field_get_field(sf);
     uint16_t padded_value_len = ROUND_UP(mf->n_bytes, 8);
     struct ofp12_action_set_field *oasf;
     char *value;
@@ -6149,7 +6149,7 @@ set_field_to_ofast(const struct ofpact_set_field *sf,
     oasf->len = htons(ntohs(oasf->len) + padded_value_len);
 
     value = ofpbuf_put_zeros(openflow, padded_value_len);
-    memcpy(value, &sf->value, mf->n_bytes);
+    memcpy(value, ofpact_set_field_get_const_value(sf), mf->n_bytes);
 }
 
 void
@@ -6165,7 +6165,7 @@ ofputil_set_field_to_openflow(const struct ofpact_set_field *sf,
         return;
     }
 
-    mf = mf_from_id(sf->field);
+    mf = ofpact_set_field_get_field(sf);
 
     /* Convert to one or two REG_LOADs */
 
@@ -6176,18 +6176,18 @@ ofputil_set_field_to_openflow(const struct ofpact_set_field *sf,
         narl = ofputil_put_NXAST_REG_LOAD(openflow);
         narl->ofs_nbits = nxm_encode_ofs_nbits(0, 64);
         narl->dst = htonl(mf->nxm_header);
-        narl->value = *(&sf->value.be64 + 1);
+        narl->value = *(&sf->be64 + 1);
         /* Higher bits next. */
         narl = ofputil_put_NXAST_REG_LOAD(openflow);
         narl->ofs_nbits = nxm_encode_ofs_nbits(64, mf->n_bits - 64);
         narl->dst = htonl(mf->nxm_header);
-        narl->value = sf->value.be64;
+        narl->value = sf->be64;
     } else {
         narl = ofputil_put_NXAST_REG_LOAD(openflow);
         narl->ofs_nbits = nxm_encode_ofs_nbits(0, mf->n_bits);
         narl->dst = htonl(mf->nxm_header);
         memset(&narl->value, 0, 8 - mf->n_bytes);
         memcpy((char*)&narl->value + (8 - mf->n_bytes),
-               &sf->value, mf->n_bytes);
+               ofpact_set_field_get_const_value(sf), mf->n_bytes);
     }
 }
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 65ee8e2..d10c601 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2425,9 +2425,10 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
 
         case OFPACT_SET_FIELD:
             set_field = ofpact_get_SET_FIELD(a);
-            mf = mf_from_id(set_field->field);
+            mf = ofpact_set_field_get_field(set_field);
             mf_mask_field_and_prereqs(mf, &wc->masks);
-            mf_set_flow_value(mf, &set_field->value, flow);
+            mf_set_flow_value(mf, ofpact_set_field_get_const_value(set_field),
+                              flow);
             break;
 
         case OFPACT_STACK_PUSH:
-- 
1.7.10.4




More information about the dev mailing list