[ovs-dev] [PATCH 11/14] in-band: Support an arbitrary number of controllers.

Justin Pettit jpettit at nicira.com
Sat Apr 17 02:40:33 UTC 2010


Man, I get a bad feeling every time we touch in-band.  It's sort of like a lame version of Jenga*.

> +/* Priority used in classifier for in-band rules.  This value is higher than
> + * any that may be set with OpenFlow, and "18" kind of looks like "IB". */
> +#define IN_BAND_PRIORITY 18181818

I did not know that was the reason you chose that number.  This ranks up there with finding out that Arby's name is just the pronunciation of Roast Beef's initals.

> +static int
> +refresh_remote(struct in_band *ib, struct in_band_remote *r)
> {
> [Cut out code.]
> +    /* If we have an IP address but not a MAC address, then refresh quickly,
> +     * since we probably will get a MAC address soon (via ARP).  Otherwise, we
> +     * can afford to wait a little while. */
> +    return r->remote_ip && eth_addr_is_zero(r->remote_mac) ? 1 : 10;
> }

I don't know under what circumstances it would occur, but I would think that if we don't have a remote IP address that we would want to refresh more quickly than 10 seconds.

> +static bool
> +refresh_remotes(struct in_band *ib)
> {
> [Cut out code.]
> +    any_changes = false;
> +    min_refresh = 10;

It seems like it would be good to make the "min_refresh" value a #define at the top of the file.

> +in_band_status_cb(struct status_reply *sr, void *in_band_)
> {
> [Cut out code.]
> +    if (in_band->n_remotes
> +        && !eth_addr_is_zero(in_band->remotes[0].remote_mac)) {
> +        status_reply_put(sr, "remote-mac="ETH_ADDR_FMT,
> +                         ETH_ADDR_ARGS(in_band->remotes[0].remote_mac));
>     }
> }

Do you think we only should print a single remote MAC?

> +static void
> +init_rule(struct in_band_rule *rule)
> {
> +    rule->wildcards = OFPFW_ALL;
> +    /* Not strictly necessary but seems cleaner. */
> +    memset(&rule->flow, 0, sizeof rule->flow);
> +}

The "priority" field in the "in_band_rule" struct isn't currently being used (more on that later), but if you keep it around, it may be a good idea to set a default value.

> +static void
> +make_rules(struct in_band *ib,
> +           void (*cb)(struct in_band *, const struct in_band_rule *))
> +{
> ...
> }

This is a much cleaner way of setting up the rules!

> +static void
> +clear_rules(struct in_band *ib)
> +{
> +    memset(ib->installed_local_mac, 0, sizeof ib->installed_local_mac);
> +
> +    free(ib->remote_ips);
> +    ib->remote_ips = NULL;
> +    ib->n_remote_ips = 0;
> +
> +    free(ib->remote_macs);
> +    ib->remote_macs = NULL;
> +    ib->n_remote_macs = 0;
> +}

The function clear_rules() doesn't do what I'd expect based on the name.  Would a name like clear_rule_state() be better?

> +static void
> +add_rule(struct in_band *ib, const struct in_band_rule *rule)
> +{
> +    union ofp_action action;
> +
> +    action.type = htons(OFPAT_OUTPUT);
> +    action.output.len = htons(sizeof action);
> +    action.output.port = htons(OFPP_NORMAL);
> +    action.output.max_len = htons(0);
> +    ofproto_add_flow(ib->ofproto, &rule->flow, rule->wildcards,
> +                     IN_BAND_PRIORITY, &action, 1, 0);
> +}

It's not terribly important, but I actually found the different priorities for the different in-band rules helpful for debugging.  It makes it easy to see which counters are incrementing and then figuring out which rules aren't getting hit.  I realize that we could have an explosion of rules with multiple controllers, but even just using nine different priorities for the nine different types of in-band rules would be helpful.

If you don't want to do that, then the "priority" field in the "in_band_rule" struct should probably be removed since it's not being used.

> +void
> +in_band_set_remotes(struct in_band *ib, struct rconn **remotes, size_t n)
> +{
> +    size_t i;
> +
> +    /* Optimize the case where the rconns are the same as last time. */
> +    if (n == ib->n_remotes) {
> +        for (i = 0; i < n; i++) {
> +            if (ib->remotes[i].rconn != remotes[i]) {
> +                goto different;
> +            }
>         }
> +        return;
> 
> -        switch_status_unregister(in_band->ss_cat);
> -        netdev_close(in_band->local_netdev);
> -        netdev_close(in_band->remote_netdev);
> +    different:;
> +    }

We discussed this in person, but having the label in the block and ending in a semicolon looks odd to me. Although, I realize it's perfectly legal and valid syntax.

> +    ib->next_remote_refresh = TIME_MIN;
> +    ib->remotes = n ? xzalloc(n * sizeof *ib->remotes) : 0;

I think it would be clearer if 0 were NULL.

--Justin


----------------------------


* I was reading the Wikipedia article on Jenga, and they mention a variation called Jenga Truth or Dare.  Apparently, this is an official variant produce by Hasbro, which I suspect is a bit tamer than most games of Truth or Dare, but I digress.  In the article, I liked the part that reads: "The natural blocks have nothing printed on them and are played as in Jenga. However, it is permissible to write your own truths or dares on  the natural blocks if desired."  I'm glad to hear that this doesn't alter the ability of the game to be played or trigger some sort of DMCA violation.







More information about the dev mailing list