[ovs-dev] [PATCH 2/9] ofp-actions: Add action bitmap abstraction.

Ben Pfaff blp at nicira.com
Mon Aug 11 18:00:24 UTC 2014


On Thu, Aug 07, 2014 at 04:03:21PM -0700, Jarno Rajahalme wrote:
> Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
> 
> Three minor comments below,
> 
>   Jarno
> 
> On Aug 4, 2014, at 9:21 AM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > Until now, sets of actions have been abstracted separately outside
> > ofp-actions, as enum ofputil_action_bitmap.  Drawing sets of actions into
> > ofp-actions, as done in this commit, makes for a better overall
> > abstraction of actions, with better consistency.
> > 
> > A big part of this commit is shifting from using ofp12_table_stats as if
> > it were an abstraction for OpenFlow table stats, toward using a new
> > struct ofputil_table_stats, which is what we generally do with other
> > OpenFlow structures and fits better with the rest of the code.
> > 
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> > lib/ofp-actions.c          | 156 +++++++++++++++++++++++++
> > lib/ofp-actions.h          | 145 +++++++++++------------
> > lib/ofp-print.c            | 105 ++++-------------
> > lib/ofp-util.c             | 280 ++++++++++++++++++++++-----------------------
> > lib/ofp-util.h             |  66 ++++-------
> > ofproto/ofproto-dpif.c     |  23 +---
> > ofproto/ofproto-provider.h |  22 ++--
> > ofproto/ofproto.c          |  85 ++++++--------
> > tests/ofp-print.at         |  26 +++--
> > tests/ofproto.at           |  20 ++--
> > 10 files changed, 495 insertions(+), 433 deletions(-)
> > 
> > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> > index e8fd922..3818b2d 100644
> > --- a/lib/ofp-actions.c
> > +++ b/lib/ofp-actions.c
> > @@ -3091,6 +3091,151 @@ ofpacts_put_openflow_instructions(const struct ofpact ofpacts[],
> >     }
> > }
> > 
> > +/* Sets of supported actions. */
> > +
> > +struct ofpact_xlate {
> > +    enum ofpact_type ofpact;
> > +    int ofpat;
> 
> This would benefit from some comments:
> - ?ofpact? OVS internal enumeration of all supported action types.
> - ?ofpat' the corresponding action number specified in a given version of the OpenFlow specification.
> 
> Also, this has nothing to do with ofproto-dpif-xlate, but ?xlate? made me initially think this would be related.

I guess that "map" is better than "xlate" from that point of view.  I
did a search-and-replace.  Here is what I ended up with on the
comments:

/* Two-way translation between OVS's internal "OFPACT_*" representation of
 * actions and the "OFPAT_*" representation used in some OpenFlow version.
 * (OFPAT_* numbering varies from one OpenFlow version to another, so a given
 * instance is specific to one OpenFlow version.) */
struct ofpact_map {
    enum ofpact_type ofpact;    /* Internal name for action type. */
    int ofpat;                  /* OFPAT_* number from OpenFlow spec. */
};

> > +};
> > +
> > +static const struct ofpact_xlate *
> > +get_ofpact_xlate(enum ofp_version version)
> > +{
> > +    /* OpenFlow 1.0 actions. */
> > +    static const struct ofpact_xlate of10[] = {
> > +        { OFPACT_OUTPUT, 0 },
> > +        { OFPACT_SET_VLAN_VID, 1 },
> > +        { OFPACT_SET_VLAN_PCP, 2 },
> > +        { OFPACT_STRIP_VLAN, 3 },
> > +        { OFPACT_SET_ETH_SRC, 4 },
> > +        { OFPACT_SET_ETH_DST, 5 },
> > +        { OFPACT_SET_IPV4_SRC, 6 },
> > +        { OFPACT_SET_IPV4_DST, 7 },
> > +        { OFPACT_SET_IP_DSCP, 8 },
> > +        { OFPACT_SET_L4_SRC_PORT, 9 },
> > +        { OFPACT_SET_L4_DST_PORT, 10 },
> > +        { OFPACT_ENQUEUE, 11 },
> > +        { 0, -1 },
> > +    };
> > +
> 
> (snip)
> 
> > diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
> > index 8cb8862..5877151 100644
> > --- a/lib/ofp-actions.h
> > +++ b/lib/ofp-actions.h
> > @@ -31,7 +31,7 @@
> >  * This macro is used directly only internally by this header, but the list is
> >  * still of interest to developers.
> >  *
> > - * Each DEFINE_OFPACT invocation has the following parameters:
> > + * Each OFPACT invocation has the following parameters:
> >  *
> >  * 1. <ENUM>, used below in the enum definition of OFPACT_<ENUM>, and
> >  *    elsewhere.
> 
> Add: 4. string name of the action.

Thanks, added:

 * 4. <NAME>, a quoted string that gives the name of the action, for use in
 *    parsing actions from text.


> > -    if (features.types & (1u << OFPGT11_FF)) {
> > -        ds_put_format(string, "    Fast Failover group :\n");
> > -        ds_put_format(string, "        max_groups = %#"PRIx32" "
> > -                      "actions=0x%08"PRIx32"\n",
> > -                      features.max_groups[3], features.actions[3]);
> > +    for (i = 0; i < 4; i++) {
> 
> Using literal 4 here and the OVS_NOT_REACHED in group_type_to_string
> seems a bit fragile, we did not abort before if the features.types
> has unknown bits set. I see that the number 4 is hardwired also in
> the abstract group features structure. Maybe define a macro/enum for
> ?4??

It still does not abort if features.types has unknown bits: the bits
that are understood (bits 0, 1, 2, 3) are the same as the bits that
the loop checks for.

A macro is a good idea.  I folded that in:

diff --git a/include/openflow/openflow-1.2.h b/include/openflow/openflow-1.2.h
index 1069f4e..c7ba6fd 100644
--- a/include/openflow/openflow-1.2.h
+++ b/include/openflow/openflow-1.2.h
@@ -306,12 +306,20 @@ struct ofp12_table_stats {
 };
 OFP_ASSERT(sizeof(struct ofp12_table_stats) == 128);
 
+/* Number of types of groups supported by ofp12_group_features_stats. */
+#define OFPGT12_N_TYPES 4
+
 /* Body of reply to OFPST12_GROUP_FEATURES request. Group features. */
 struct ofp12_group_features_stats {
     ovs_be32  types;           /* Bitmap of OFPGT11_* values supported. */
     ovs_be32  capabilities;    /* Bitmap of OFPGFC12_* capability supported. */
-    ovs_be32  max_groups[4];   /* Maximum number of groups for each type. */
-    ovs_be32  actions[4];      /* Bitmaps of OFPAT_* that are supported. */
+
+    /* Each element in the following arrays corresponds to the group type with
+     * the same number, e.g. max_groups[0] is the maximum number of OFPGT11_ALL
+     * groups, actions[2] is the actions supported by OFPGT11_INDIRECT
+     * groups. */
+    ovs_be32  max_groups[OFPGT12_N_TYPES]; /* Max number of groups. */
+    ovs_be32  actions[OFPGT12_N_TYPES];    /* Bitmaps of supported OFPAT_*. */
 };
 OFP_ASSERT(sizeof(struct ofp12_group_features_stats) == 40);
 
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -2443,7 +2443,7 @@ ofp_print_group_features(struct ds *string, const struct ofp_header *oh)
     ds_put_format(string, "    Capabilities:  0x%"PRIx32"\n",
                   features.capabilities);
 
-    for (i = 0; i < 4; i++) {
+    for (i = 0; i < OFPGT12_N_TYPES; i++) {
         if (features.types & (1u << i)) {
             ds_put_format(string, "    %s group:\n", group_type_to_string(i));
             ds_put_format(string, "       max_groups=%#"PRIx32"\n",
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 0ef4070..9a8a868 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -6799,7 +6799,7 @@ ofputil_encode_group_features_reply(
     ogf = ofpbuf_put_zeros(reply, sizeof *ogf);
     ogf->types = htonl(features->types);
     ogf->capabilities = htonl(features->capabilities);
-    for (i = 0; i < 4; i++) {
+    for (i = 0; i < OFPGT12_N_TYPES; i++) {
         ogf->max_groups[i] = htonl(features->max_groups[i]);
         ogf->actions[i] = ofpact_bitmap_to_openflow(features->ofpacts[i],
                                                     request->version);
@@ -6818,7 +6818,7 @@ ofputil_decode_group_features_reply(const struct ofp_header *oh,
 
     features->types = ntohl(ogf->types);
     features->capabilities = ntohl(ogf->capabilities);
-    for (i = 0; i < 4; i++) {
+    for (i = 0; i < OFPGT12_N_TYPES; i++) {
         features->max_groups[i] = ntohl(ogf->max_groups[i]);
         features->ofpacts[i] = ofpact_bitmap_from_openflow(
             ogf->actions[i], oh->version);

I pushed this to master.

Thank you for the review!



More information about the dev mailing list