[ovs-dev] [bridge 05/15] bridge: Change struct port's active_iface member from index to pointer.
Ben Pfaff
blp at nicira.com
Mon Mar 21 17:59:46 UTC 2011
This makes the code easier to understand.
As a historical note, the "bridge" code was originally written in an
almighty hurry, and so some design decisions were made on the basis of
being unlikely to cause serious bugs instead of on the basis of being
easy to understand. That's why there are so many array indexes sprinkled
around the bridge data structures, and so much range checking of their
values, when it would be better to just have pointers that can be followed
directly. I figured that getting the wrong index would at least do
something half-reasonable in most cases, whereas dereferencing a freed
pointer was likely to segfault sooner or later and cause immediate failure.
But now I think it's time to improve the code. The code is mature enough
now that we should be able to thoroughly understand the data lifetime
issues.
---
vswitchd/bridge.c | 70 +++++++++++++++++++++++++++-------------------------
1 files changed, 36 insertions(+), 34 deletions(-)
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index d07f591..3140469 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -174,7 +174,7 @@ struct port {
/* Bonding info. */
enum bond_mode bond_mode; /* Type of the bond. BM_SLB is the default. */
- int active_iface; /* Ifidx on which bcasts accepted, or -1. */
+ struct iface *active_iface; /* iface on which bcasts accepted, or NULL. */
tag_type no_ifaces_tag; /* Tag for flows when all ifaces disabled. */
int updelay, downdelay; /* Delay before iface goes up/down, in ms. */
bool bond_fake_iface; /* Fake a bond interface for legacy compat? */
@@ -2178,32 +2178,32 @@ lookup_bond_entry(const struct port *port, const struct flow *flow,
}
}
-static int
+static struct iface *
bond_choose_iface(const struct port *port)
{
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
- size_t i, best_down_slave = -1;
- long long next_delay_expiration = LLONG_MAX;
+ struct iface *best_down_slave;
+ size_t i;
+ best_down_slave = NULL;
for (i = 0; i < port->n_ifaces; i++) {
struct iface *iface = port->ifaces[i];
if (iface->enabled) {
- return i;
- } else if (iface->delay_expires < next_delay_expiration
+ return iface;
+ } else if ((!best_down_slave
+ || iface->delay_expires < best_down_slave->delay_expires)
&& lacp_slave_may_enable(port->lacp, iface)) {
- best_down_slave = i;
- next_delay_expiration = iface->delay_expires;
+ best_down_slave = iface;
}
}
- if (best_down_slave != -1) {
- struct iface *iface = port->ifaces[best_down_slave];
-
+ if (best_down_slave) {
VLOG_INFO_RL(&rl, "interface %s: skipping remaining %lli ms updelay "
- "since no other interface is up", iface->name,
- iface->delay_expires - time_msec());
- bond_enable_slave(iface, true);
+ "since no other interface is up",
+ best_down_slave->name,
+ best_down_slave->delay_expires - time_msec());
+ bond_enable_slave(best_down_slave, true);
}
return best_down_slave;
@@ -2219,22 +2219,24 @@ choose_output_iface(const struct port *port, const struct flow *flow,
if (port->n_ifaces == 1) {
iface = port->ifaces[0];
} else if (port->bond_mode == BM_AB) {
- if (port->active_iface < 0) {
+ iface = port->active_iface;
+ if (!iface) {
*tags |= port->no_ifaces_tag;
return false;
}
- iface = port->ifaces[port->active_iface];
} else {
struct bond_entry *e = lookup_bond_entry(port, flow, vlan);
if (e->iface_idx < 0 || e->iface_idx >= port->n_ifaces
|| !port->ifaces[e->iface_idx]->enabled) {
/* XXX select interface properly. The current interface selection
* is only good for testing the rebalancing code. */
- e->iface_idx = bond_choose_iface(port);
- if (e->iface_idx < 0) {
+ iface = bond_choose_iface(port);
+ if (!iface) {
+ e->iface_idx = -1;
*tags |= port->no_ifaces_tag;
return false;
}
+ e->iface_idx = iface->port_ifidx;
e->iface_tag = tag_create_random();
}
*tags |= e->iface_tag;
@@ -2271,7 +2273,7 @@ bond_link_status_update(struct iface *iface)
iface->delay_expires = LLONG_MAX;
VLOG_INFO_RL(&rl, "interface %s: will not be %s",
iface->name, up ? "disabled" : "enabled");
- } else if (up && port->active_iface < 0) {
+ } else if (up && !port->active_iface) {
bond_enable_slave(iface, true);
if (updelay) {
VLOG_INFO_RL(&rl, "interface %s: skipping %d ms updelay since no "
@@ -2297,9 +2299,9 @@ bond_choose_active_iface(struct port *port)
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
port->active_iface = bond_choose_iface(port);
- if (port->active_iface >= 0) {
+ if (port->active_iface) {
VLOG_INFO_RL(&rl, "port %s: active interface is now %s",
- port->name, port->ifaces[port->active_iface]->name);
+ port->name, port->active_iface->name);
} else {
VLOG_WARN_RL(&rl, "port %s: all ports disabled, no active interface",
port->name);
@@ -2327,7 +2329,7 @@ bond_enable_slave(struct iface *iface, bool enable)
if (!iface->enabled) {
VLOG_WARN("interface %s: disabled", iface->name);
ofproto_revalidate(br->ofproto, iface->tag);
- if (iface->port_ifidx == port->active_iface) {
+ 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
@@ -2340,7 +2342,7 @@ bond_enable_slave(struct iface *iface, bool enable)
bond_send_learning_packets(port);
} else {
VLOG_WARN("interface %s: enabled", iface->name);
- if (port->active_iface < 0 && !moving_active_iface) {
+ 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);
@@ -2581,8 +2583,8 @@ port_is_floodable(const struct port *port)
static tag_type
port_get_active_iface_tag(const struct port *port)
{
- return (port->active_iface >= 0
- ? port->ifaces[port->active_iface]->tag
+ return (port->active_iface
+ ? port->active_iface->tag
: port->no_ifaces_tag);
}
@@ -2879,7 +2881,7 @@ is_admissible(struct bridge *br, const struct flow *flow, bool have_packet,
if (eth_addr_is_multicast(flow->dl_dst)) {
*tags |= port_get_active_iface_tag(in_port);
- if (in_port->active_iface != in_iface->port_ifidx) {
+ if (in_port->active_iface != in_iface) {
/* Drop all multicast packets on inactive slaves. */
return false;
}
@@ -3394,7 +3396,7 @@ bond_send_learning_packets(struct port *port)
struct ofpbuf packet;
int error, n_packets, n_errors;
- if (!port->n_ifaces || port->active_iface < 0 || bond_is_tcp_hash(port)) {
+ if (!port->n_ifaces || !port->active_iface || bond_is_tcp_hash(port)) {
return;
}
@@ -3547,7 +3549,7 @@ bond_unixctl_show(struct unixctl_conn *conn,
/* Basic info. */
ds_put_format(&ds, "\nslave %s: %s\n",
iface->name, iface->enabled ? "enabled" : "disabled");
- if (j == port->active_iface) {
+ if (iface == port->active_iface) {
ds_put_cstr(&ds, "\tactive slave\n");
}
if (iface->delay_expires != LLONG_MAX) {
@@ -3691,10 +3693,10 @@ bond_unixctl_set_active_slave(struct unixctl_conn *conn, const char *args_,
return;
}
- if (port->active_iface != iface->port_ifidx) {
+ if (port->active_iface != iface) {
ofproto_revalidate(port->bridge->ofproto,
port_get_active_iface_tag(port));
- port->active_iface = iface->port_ifidx;
+ port->active_iface = iface;
VLOG_INFO("port %s: active interface is now %s",
port->name, iface->name);
bond_send_learning_packets(port);
@@ -3894,7 +3896,7 @@ port_create(struct bridge *br, const char *name)
port->vlan = -1;
port->trunks = NULL;
port->name = xstrdup(name);
- port->active_iface = -1;
+ port->active_iface = NULL;
if (br->n_ports >= br->allocated_ports) {
br->ports = x2nrealloc(br->ports, &br->allocated_ports,
@@ -4251,7 +4253,7 @@ port_update_bonding(struct port *port)
free(port->bond_hash);
port->bond_hash = NULL;
port->bond_fake_iface = false;
- port->active_iface = -1;
+ port->active_iface = NULL;
port->no_ifaces_tag = 0;
} else {
size_t i;
@@ -4274,7 +4276,7 @@ port_update_bonding(struct port *port)
port->no_ifaces_tag = tag_create_random();
}
- if (port->active_iface < 0) {
+ if (!port->active_iface) {
bond_choose_active_iface(port);
}
@@ -4329,7 +4331,7 @@ iface_destroy(struct iface *iface)
if (iface) {
struct port *port = iface->port;
struct bridge *br = port->bridge;
- bool del_active = port->active_iface == iface->port_ifidx;
+ bool del_active = port->active_iface == iface;
struct iface *del;
if (iface->port->lacp) {
--
1.7.1
More information about the dev
mailing list