Sounds good, thanks. Ethan
On Wed, May 11, 2011 at 12:13, Ben Pfaff <[email protected]> wrote: > On Tue, May 10, 2011 at 06:18:08PM -0700, Ethan Jackson wrote: >> @@ -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. */ >> > > OK, I sent out a patch (and you already reviewed it). > >> /* 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); > > The comment is wrong, but the issue is real. > > I rewrote it like this: > > error = dpif_port_del(ofproto->dpif, ofp_port_to_odp_port(ofp_port)); > if (!error) { > struct ofport_dpif *ofport = get_ofp_port(ofproto, ofp_port); > if (ofport) { > /* The caller is going to close ofport->up.netdev. If this is a > * bonded port, then the bond is using that netdev, so remove it > * from the bond. The client will need to reconfigure everything > * after deleting ports, so then the slave will get re-added. */ > 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. */ > > OK, I added a temporary. > >> } 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. */ >> } > > I think you're right. I changed this to set need_revalidate. > > In a later patch this rule_destruct() and rule_remove() get combined, > IIRC. rule_remove() already sets need_revalidate, so this becomes a > bit more natural at that point. > >> 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. */ > > It's just insufficient documentation in this case. A patch in next2 > adds documentation: > > + /* Validates that the 'n' elements in 'actions' are well-formed OpenFlow > + * actions that can be correctly implemented by the datapath. If not, > then > + * return an OpenFlow error code (as returned by ofp_mkerr()). If so, > + * then update the datapath to implement the new actions and return 0. > + * > + * When this function runs, 'rule' still has its original actions. If > this > + * function returns 0, then the caller will update 'rule' with the new > + * actions and free the old ones. */ > > Sorry about that. Lack of documentation probably made this patch > harder to review than necessary. > >> @@ -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) { > > OK, I added a blank line here. > >> 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. */ > > Order doesn't matter with other OVS classes (e.g. netdev, dpif) > because each one can only implement a single type, and duplicate types > are not allowed. So, ordering doesn't matter, because there can be no > overlap. > > An ofproto_class is different: it can implement several types, the > types that it implements are allowed to change > (e.g. ofproto_dpif_class's implemented types will change as dpif > classes are registered and unregistered), and there's no attempt to > ensure that they don't overlap. Therefore, order can matter, so I > decided that we might as well maintain the existing order on > unregister to avoid weird surprises. > >> @@ -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. >> + */ > > OK, I added a mention. > >> 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; >> } > > Good catch. I changed this code to: > if (n_actions > 0) { > rule->actions = xmemdup(actions, n_actions * sizeof *actions); > } else { > rule->actions = NULL; > } > rule->n_actions = n_actions; > >> @@ -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); > > I wasn't really sure what to do here. OpenFlow doesn't provide any > way to find out how long a flow has been idle, so I didn't have an a > priori reason to add such an interface to the ofproto_class. > > For now I've deleted it. > >> 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. */ > > Yeah, I've been asking for someone with hardware experience to comment > on various interfaces I proposed literally for *YEARS*. But hardware > people apparently don't have opinions, or at least they don't share > them. > > I'm not holding my breath. > >> #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. */ > > Thanks. I've dealt with far too many libraries that just don't > document expectations. I didn't want to make the same mistake. > >> /* 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. > > I'm pretty sure that "none" is singular. > >> * 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. > > Thanks, fixed. > >> * 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); > > I just ran out of time. Commit "ofproto: Better document the > ofproto_class interface" in next2 adds the missing comments. > >> @@ -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); >> > > OK, I revised this comment and the others that you pointed out. > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
