> static void
> bond_choose_active_iface(struct port *port)
> {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> + struct iface *old_active_iface = port->active_iface;
>
> port->active_iface = bond_choose_iface(port);
> if (port->active_iface) {
> - VLOG_INFO_RL(&rl, "port %s: active interface is now %s",
> - port->name, port->active_iface->name);
> - } else {
> - VLOG_WARN_RL(&rl, "port %s: all ports disabled, no active interface",
> - port->name);
> - }
> -}
> -
> -static void
> -bond_enable_slave(struct iface *iface, bool enable)
> -{
> - struct port *port = iface->port;
> - struct bridge *br = port->bridge;
> -
> - /* This acts as a recursion check. If the act of disabling a slave
> - * causes a different slave to be enabled, the flag will allow us to
> - * skip redundant work when we reenter this function. It must be
> - * cleared on exit to keep things safe with multiple bonds. */
> - static bool moving_active_iface = false;
> -
> - iface->delay_expires = LLONG_MAX;
> - if (enable == iface->enabled) {
> - return;
> - }
> + if (port->active_iface->enabled) {
> + VLOG_INFO_RL(&rl, "port %s: active interface is now %s",
> + port->name, port->active_iface->name);
> + } else {
> + VLOG_INFO_RL(&rl, "port %s: active interface is now %s, skipping
> "
> + "remaining %lld ms updelay (since no interface was "
> + "enabled)", port->name, port->active_iface->name,
> + port->active_iface->delay_expires - time_msec());
> + bond_enable_slave(port->active_iface, true);
> + }
>
> - iface->enabled = enable;
> - if (!iface->enabled) {
> - VLOG_WARN("interface %s: disabled", iface->name);
> - ofproto_revalidate(br->ofproto, iface->tag);
> - if (iface == port->active_iface) {
> - /* Disabling a slave can lead to another slave being immediately
> - * enabled if there will be no active slaves but one is waiting
> - * on an updelay. In this case we do not need to run most of the
> - * code for the newly enabled slave since there was no period
> - * without an active slave and it is redundant with the disabling
> - * path. */
> - moving_active_iface = true;
> - bond_choose_active_iface(port);
> + if (!old_active_iface) {
> + ofproto_revalidate(port->bridge->ofproto, port->no_ifaces_tag);
> }
> bond_send_learning_packets(port);
> } else {
> - VLOG_WARN("interface %s: enabled", iface->name);
> - if (!port->active_iface && !moving_active_iface) {
> - ofproto_revalidate(br->ofproto, port->no_ifaces_tag);
> - bond_choose_active_iface(port);
> - bond_send_learning_packets(port);
> - }
> - iface->tag = tag_create_random();
> + VLOG_WARN_RL(&rl, "port %s: all ports disabled, no active interface",
> + port->name);
> }
> -
> - moving_active_iface = false;
> }
I don't have my mind completely wrapped around this. But it seems
like it might be a good idea to ofproto_revalidate old_active_iface if
it exists. Similarly we may want to revalidate the new active_iface.
This may be automatically done since active_iface will only be changed
during slave enabling and disabling. It might be a defensive strategy
though.
Looks Good to me.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
