[ovs-dev] [next 35/35] ofproto: Break apart into generic and hardware-specific parts.

Ethan Jackson ethan at nicira.com
Wed May 11 01:18:08 UTC 2011


Since this patch was so big, I did my review as a diff against the source.
Overall the patch is good.  Only some minor comments and questions.

Ethan

---
 ofproto/ofproto-dpif.c |   19 +++++++++++++++++++
 ofproto/ofproto.c      |   12 +++++++++++-
 ofproto/private.h      |   25 +++++++++++++++++++------
 3 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c2361f8..a31fd1b 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -174,6 +174,11 @@ struct action_xlate_ctx {
      * its actions.  If special_cb returns false on 'flow' rendered
      * uninstallable and no actions will be executed. */
     bool check_special;
+    /* This is probably not the patch to fix it, but I'm pretty sure that
+     * check_special is dead code now.  Doing some research, it apparently has
+     * been since ofproto_send_packet() no longer had to call xlate_actions().
+     * I don't think this is worth fixing on master, but it may be worth adding
+     * a patch to the end of the next branch which cleans it up. */
 
 /* xlate_actions() initializes and uses these members.  The client might want
  * to look at them after it returns. */
@@ -1400,6 +1405,9 @@ port_del(struct ofproto *ofproto_, uint16_t ofp_port)
          * it from the bond before closing the netdev.  The client will need to
          * reconfigure everything after deleting ports, so then the slave will
          * get re-added. */
+        /* I don't fully understand this comment.  You don't ever close the
+         * netdev in this function.  Do you intend to, or does it happen
+         * somewhere else? */
         struct ofport_dpif *ofport = get_ofp_port(ofproto, ofp_port);
         if (ofport) {
             bundle_remove(&ofport->up);
@@ -1714,6 +1722,8 @@ update_stats(struct ofproto_dpif *p)
 
             if (stats->n_packets >= facet->dp_packet_count) {
                 facet->packet_count += stats->n_packets - facet->dp_packet_count;
+                /* The above line is too long.  It was too long in the old
+                 * version as well, but may as well fix it. */
             } else {
                 VLOG_WARN_RL(&rl, "unexpected packet count from the datapath");
             }
@@ -2472,6 +2482,10 @@ rule_destruct(struct rule *rule_)
     LIST_FOR_EACH_SAFE (facet, next_facet, list_node, &rule->facets) {
         facet_revalidate(ofproto, facet);
     }
+    /* I may be thinking about this incorrectly.  I think rule destruction
+     * requires ofproto->need_revalidate = true because of resubmits.  Theres
+     * no way to predict which facets resubmit into 'rule_' and thus need their
+     * actions recalculation. */
 }
 
 static void
@@ -2545,6 +2559,10 @@ rule_execute(struct rule *rule_, struct flow *flow, struct ofpbuf *packet)
     ofpbuf_delete(odp_actions);
 }
 
+/* The name of this function makes one think that it should actually be setting
+ * the ofp_actions in 'rule_'.  Apparently, it only validates that the actions
+ * are reasonable.  I can't tell if this is a mistake in the implementation, or
+ * just a case of insufficient documentation. */
 static int
 rule_modify_actions(struct rule *rule_,
                     const union ofp_action *actions, size_t n_actions)
@@ -2846,6 +2864,7 @@ xlate_autopath(struct action_xlate_ctx *ctx,
 {
     uint16_t ofp_port = ntohl(naa->id);
     struct ofport_dpif *port = get_ofp_port(ctx->ofproto, ofp_port);
+
     if (!port || !port->bundle) {
         ofp_port = OFPP_NONE;
     } else if (port->bundle->bond) {
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 57d4116..ec07895 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -200,6 +200,8 @@ ofproto_class_unregister(const struct ofproto_class *class)
 {
     size_t i;
 
+    /* Why do we need to maintaion the order of ofproto_classes?  Seems fine,
+     * just curious. */
     for (i = 0; i < n_ofproto_classes; i++) {
         if (ofproto_classes[i] == class) {
             for (i++; i < n_ofproto_classes; i++) {
@@ -541,7 +543,10 @@ ofproto_port_is_lacp_current(struct ofproto *ofproto, uint16_t ofp_port)
  * to 's'.  Otherwise, this function registers a new bundle.
  *
  * Bundles affect only the treatment of packets output to the OFPP_NORMAL
- * port.  */
+ * port.
+ *
+ * This isn't true.  Bundles affect the autopath action as well.
+ */
 int
 ofproto_bundle_register(struct ofproto *ofproto, void *aux,
                         const struct ofproto_bundle_settings *s)
@@ -1271,6 +1276,10 @@ rule_create(struct ofproto *ofproto, const struct cls_rule *cls_rule,
     if (n_actions > 0) {
         rule->n_actions = n_actions;
         rule->actions = xmemdup(actions, n_actions * sizeof *actions);
+    } else {
+        /* I think this is required because rule_alloc() doesn't zero. */
+        rule->n_actions = 0;
+        rule->actions = NULL;
     }
 
     error = ofproto->ofproto_class->rule_construct(rule);
@@ -1960,6 +1969,7 @@ flow_stats_ds(struct rule *rule, struct ds *results)
 
     ds_put_format(results, "duration=%llds, ",
                   (time_msec() - rule->created) / 1000);
+    /* I'm not sure if you intend to delete this line or uncomment it. */
     //ds_put_format(results, "idle=%.3fs, ", (time_msec() - rule->used) / 1000.0);
     ds_put_format(results, "priority=%u, ", rule->cr.priority);
     ds_put_format(results, "n_packets=%"PRIu64", ", packet_count);
diff --git a/ofproto/private.h b/ofproto/private.h
index d79ed81..1ab8c6c 100644
--- a/ofproto/private.h
+++ b/ofproto/private.h
@@ -14,6 +14,11 @@
  * limitations under the License.
  */
 
+/* I think it would be a good idea to have someone with hardware experience to
+ * read over this file in addition to me to make sure it's relatively easy to
+ * port.  I don't see a reason why it wouldn't be, but I have no experience
+ * with hardware so it might be worth having another pair of eyes on it. */
+
 #ifndef OFPROTO_PRIVATE_H
 #define OFPROTO_PRIVATE_H 1
 
@@ -100,6 +105,7 @@ struct rule *ofproto_rule_lookup(struct ofproto *, const struct flow *);
 void ofproto_rule_expire(struct rule *, uint8_t reason);
 void ofproto_rule_destroy(struct rule *);
 
+/* This comment is awesome. */
 /* ofproto class structure, to be defined by each ofproto implementation.
  *
  *
@@ -168,7 +174,7 @@ void ofproto_rule_destroy(struct rule *);
  * use of the new data structure, so it cannot perform much initialization.
  * Its purpose is just to ensure that the new data structure has enough room
  * for base and derived state.  It may return a null pointer if memory is not
- * available, in which case none of the other functions is called.
+ * available, in which case none of the other functions are called.
  *
  * Each "construct" function initializes derived state in its respective data
  * structure.  When "construct" is called, all of the base state has already
@@ -246,7 +252,7 @@ struct ofproto_class {
      * poll-loop.h.  */
     void (*wait)(struct ofproto *ofproto);
 
-    /* Every "struct rule"s in 'ofproto' is about to be deleted, one by one.
+    /* Every "struct rule" in 'ofproto' is about to be deleted, one by one.
      * This function may prepare for that, for example by clearing state in
      * advance.  It should *not* actually delete any "struct rule"s from
      * 'ofproto', only prepare for it.
@@ -387,6 +393,10 @@ struct ofproto_class {
 
     void (*rule_remove)(struct rule *rule);
 
+    /* It's not clear to me why the next couple of functions, and
+     * port_is_lacp_current() above don't get comments.  When reading this, I
+     * would have found a comment on rule_execute() and rule_modify_actions()
+     * useful in particular. */
     void (*rule_get_stats)(struct rule *rule, uint64_t *packet_count,
                            uint64_t *byte_count);
 
@@ -458,9 +468,12 @@ struct ofproto_class {
      * has been registered, this has no effect.
      *
      * This function affects only the behavior of the OFPP_NORMAL action.  An
-     * implementation that does not to support it at all may set it to NULL or
+     * implementation that does not support it at all may set it to NULL or
      * return EOPNOTSUPP.  An implementation that supports only a subset of the
-     * functionality should implement what it can and return 0. */
+     * functionality should implement what it can and return 0.
+     *
+     * It affects the behavior of the autopath action as well.
+     * */
     int (*bundle_set)(struct ofproto *ofproto, void *aux,
                       const struct ofproto_bundle_settings *s);
 
@@ -480,7 +493,7 @@ struct ofproto_class {
      * has been registered, this has no effect.
      *
      * This function affects only the behavior of the OFPP_NORMAL action.  An
-     * implementation that does not to support it at all may set it to NULL or
+     * implementation that does not support it at all may set it to NULL or
      * return EOPNOTSUPP.  An implementation that supports only a subset of the
      * functionality should implement what it can and return 0. */
     int (*mirror_set)(struct ofproto *ofproto, void *aux,
@@ -491,7 +504,7 @@ struct ofproto_class {
      * 'flood_vlans' is NULL, then MAC learning applies to all VLANs.
      *
      * This function affects only the behavior of the OFPP_NORMAL action.  An
-     * implementation that does not to support it may set it to NULL or return
+     * implementation that does not support it may set it to NULL or return
      * EOPNOTSUPP. */
     int (*set_flood_vlans)(struct ofproto *ofproto,
                            unsigned long *flood_vlans);
-- 
1.7.4.4




More information about the dev mailing list