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
