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

Ben Pfaff blp at nicira.com
Tue Apr 20 21:14:25 UTC 2010


On Fri, Apr 16, 2010 at 07:40:33PM -0700, Justin Pettit wrote:
> Man, I get a bad feeling every time we touch in-band.  It's sort of
> like a lame version of Jenga*.

(Jenga can be made lamer?)

> > +/* 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.

You're blowing my mind, man.

> > +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.

Actually it shouldn't ever happen anymore that we have no IP address
here.  Before, if no controller was configured, then the in-band code
would still have an rconn, which wouldn't have a remote IP address.
Now, if no controller is configured, the in-band code won't have any
rconns.

I'll send out another patch.

> > +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.

This value can only if matter if there are no remotes, which shouldn't
happen.  I'll send out a patch to get rid of the magic number here.

> > +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?

This is really here just to support ovs-switchui, which doesn't know
what to do with more than one.  We need to transition to the database
for this kind of data, and then we can do it right.

> > +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.

Thanks, the code that I pushed does set and use distinct priorities for
different kinds of in-band 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?

I guess that we can really just inline clear_rules() into drop_flows().
I'll send a patch.

> > +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.

Done.

> > +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.

I'll send out a patch that factors this logic into a new function
any_rconns_changed(), which replaces the "goto" by a "return".

> > +    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.

Thanks, I'll send that out too.




More information about the dev mailing list