[ovs-dev] [Single DP 08/15] Separate OpenFlow port numbers from datapath ones.
Justin Pettit
jpettit at nicira.com
Tue Oct 30 22:34:32 UTC 2012
On Oct 22, 2012, at 3:11 PM, Ben Pfaff <blp at nicira.com> wrote:
> "git am" says:
>
> Applying: Separate OpenFlow port numbers from datapath ones.
> /home/blp/ovs/.git/rebase-apply/patch:230: trailing whitespace.
>
> warning: 1 line adds whitespace errors.
> Press any key to continue…
Whoops.
> In the declaration of struct ofproto, 'ofp_requests' doesn't say what
> each shash element points to, so the reader has to hunt for it. I
> think that it points to a uint16_t; if so, then I think you might as
> well use the specialized string to integer map type in simap.[ch],
> which should also be more memory efficient.
Ah, much nicer. I looked quickly, but had missed that we had a library for that.
> In the declaration of struct ofport_dpif, I'd consider naming
> 'hmap_node' something more explicit, like 'odp_port_node'. Otherwise
> I think there's a risk of confusion later with struct ofport's own
> 'hmap_node'.
Makes sense.
> We want to avoid quickly reusing OpenFlow port numbers. Previously,
> that was indirectly implemented through dpif-linux, which makes an
> effort to avoid quickly reusing kernel datapath port numbers. This
> commit defeats that, I think, by generally allocating the
> lowest-numbered available OpenFlow port number. Presumably we should
> remove the reuse-avoidance logic from dpif-linux and move it to
> ofproto?
Good point. I added some logic to ofproto, but I didn't remove the logic from dpif-linux, since it's not required for this patch. Do you think it's worth creating a new patch that always looks for the lowest numbered free port instead?
> In ofproto-dpif.c, in port_construct(), I'd consider checking that
> odp_port_to_ofp_port() returns OFPP_NONE for the new ODP port number.
> Otherwise it seems at least remotely possible that messing about with
> ovs-dpctl on a datapath could cause some kind of race that would
> result in two entries for the same ODP port number in the internal
> map, and I'd rather catch that early.
Okay.
> In ofproto-dpif.c, in port_del(), it might be a good idea to avoid
> calling dpif_port_del() if ofp_port_to_odp_port() returns OVSP_NONE.
Done.
> Should port_dump_next() skip and warn about ports for which
> odp_port_to_ofp_port() returns OFPP_NONE?
I don't know that that would work properly, since init_ports() calls the port iterator on startup--before we've assigned OpenFlow port numbers. We could work around this, but I'm not sure it's worth it for a sanity-check. Thoughts?
> ofp_port_to_odp_in_port() seems really unneeded now, since there
> should be no port numbered OFPP_CONTROLLER or OFPP_NONE.
Yep. I dropped it in an earlier patch.
> I'm a little uncomfortable with this but I can't quite put a finger on
> the issue, so I'll defer further review of it for the moment.
Do you have any new thoughts on the patch?
Below, you should find an incremental. Let me know if you'd prefer a respun patch.
--Justin
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index f887abe..e576716 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -485,7 +485,7 @@ static void facet_account(struct facet *);
static bool facet_is_controller_flow(struct facet *);
struct ofport_dpif {
- struct hmap_node hmap_node; /* In ofproto-dpif's "odp_to_ofport_map". */
+ struct hmap_node odp_port_node; /* In ofproto-dpif's "odp_to_ofport_map". *
struct ofport up;
uint32_t odp_port;
@@ -1228,9 +1228,18 @@ port_construct(struct ofport *port_)
if (error) {
return error;
}
-
+
port->odp_port = dpif_port.port_no;
- hmap_insert(&ofproto->odp_to_ofport_map, &port->hmap_node,
+
+ /* Sanity-check that a mapping doesn't already exist. This
+ * shouldn't happen. */
+ if (odp_port_to_ofp_port(ofproto, port->odp_port) != OFPP_NONE) {
+ VLOG_ERR("port %s already has an OpenFlow port number\n",
+ dpif_port.name);
+ return EBUSY;
+ }
+
+ hmap_insert(&ofproto->odp_to_ofport_map, &port->odp_port_node,
hash_int(port->odp_port, 0));
if (ofproto->sflow) {
@@ -1246,7 +1255,7 @@ port_destruct(struct ofport *port_)
struct ofport_dpif *port = ofport_dpif_cast(port_);
struct ofproto_dpif *ofproto = ofproto_dpif_cast(port->up.ofproto);
- hmap_remove(&ofproto->odp_to_ofport_map, &port->hmap_node);
+ hmap_remove(&ofproto->odp_to_ofport_map, &port->odp_port_node);
ofproto->need_revalidate = REV_RECONFIGURE;
bundle_remove(port_);
set_cfm(port_, NULL);
@@ -2573,9 +2582,11 @@ port_del(struct ofproto *ofproto_, uint16_t ofp_port)
{
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
uint32_t odp_port = ofp_port_to_odp_port(ofproto, ofp_port);
- int error;
+ int error = 0;
- error = dpif_port_del(ofproto->dpif, odp_port);
+ if (odp_port != OFPP_NONE) {
+ error = dpif_port_del(ofproto->dpif, odp_port);
+ }
if (!error) {
struct ofport_dpif *ofport = get_ofp_port(ofproto, ofp_port);
if (ofport) {
@@ -7252,7 +7263,7 @@ odp_port_to_ofp_port(const struct ofproto_dpif *ofproto, u
{
struct ofport_dpif *port;
- HMAP_FOR_EACH_IN_BUCKET (port, hmap_node,
+ HMAP_FOR_EACH_IN_BUCKET (port, odp_port_node,
hash_int(odp_port, 0),
&ofproto->odp_to_ofport_map) {
if (port->odp_port == odp_port) {
diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 1abe6aa..6286d81 100644
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -27,12 +27,12 @@
#include "ofp-errors.h"
#include "ofp-util.h"
#include "shash.h"
+#include "simap.h"
#include "timeval.h"
struct match;
struct ofpact;
struct ofputil_flow_mod;
-struct simap;
/* An OpenFlow switch.
*
@@ -63,7 +63,8 @@ struct ofproto {
struct hmap ports; /* Contains "struct ofport"s. */
struct shash port_by_name;
unsigned long *ofp_port_ids;/* Bitmap of used OpenFlow port numbers. */
- struct shash ofp_requests; /* OpenFlow port number requests. */
+ struct simap ofp_requests; /* OpenFlow port number requests. */
+ uint16_t alloc_port_no; /* Last allocated OpenFlow port number. */
uint16_t max_ports; /* Max possible OpenFlow port num, plus one. */
/* Flow tables. */
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 42467dd..cd09bbd 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -414,7 +414,7 @@ ofproto_create(const char *datapath_name, const char *datapa
ofproto->frag_handling = OFPC_FRAG_NORMAL;
hmap_init(&ofproto->ports);
shash_init(&ofproto->port_by_name);
- shash_init(&ofproto->ofp_requests);
+ simap_init(&ofproto->ofp_requests);
ofproto->max_ports = OFPP_MAX;
ofproto->tables = NULL;
ofproto->n_tables = 0;
@@ -1039,7 +1039,7 @@ ofproto_destroy__(struct ofproto *ofproto)
hmap_destroy(&ofproto->ports);
shash_destroy(&ofproto->port_by_name);
bitmap_free(ofproto->ofp_port_ids);
- shash_destroy_free_data(&ofproto->ofp_requests);
+ simap_destroy(&ofproto->ofp_requests);
OFPROTO_FOR_EACH_TABLE (table, ofproto) {
oftable_destroy(table);
@@ -1394,11 +1394,9 @@ ofproto_port_add(struct ofproto *ofproto, struct netdev *
error = ofproto->ofproto_class->port_add(ofproto, netdev);
if (!error) {
- uint16_t *ofp_request = xmalloc(sizeof *ofp_request);
const char *netdev_name = netdev_get_name(netdev);
- *ofp_request = ofp_port;
- shash_add(&ofproto->ofp_requests, netdev_name, ofp_request);
+ simap_put(&ofproto->ofp_requests, netdev_name, ofp_port);
update_port(ofproto, netdev_name);
}
if (ofp_portp) {
@@ -1438,12 +1436,12 @@ ofproto_port_del(struct ofproto *ofproto, uint16_t ofp_p
{
struct ofport *ofport = ofproto_get_port(ofproto, ofp_port);
const char *name = ofport ? netdev_get_name(ofport->netdev) : "<unknown>";
- uint16_t *ofp_request;
+ struct simap_node *ofp_request_node;
int error;
- ofp_request = shash_find_and_delete(&ofproto->ofp_requests, name);
- if (ofp_request) {
- free(ofp_request);
+ ofp_request_node = simap_find(&ofproto->ofp_requests, name);
+ if (ofp_request_node) {
+ simap_delete(&ofproto->ofp_requests, ofp_request_node);
}
error = ofproto->ofproto_class->port_del(ofproto, ofp_port);
@@ -1572,24 +1570,37 @@ reinit_ports(struct ofproto *p)
}
static uint16_t
-alloc_ofp_port(const struct ofproto *ofproto, const char *netdev_name)
+alloc_ofp_port(struct ofproto *ofproto, const char *netdev_name)
{
- uint16_t *ofp_request;
uint16_t ofp_port;
- ofp_request = shash_find_data(&ofproto->ofp_requests, netdev_name);
- ofp_port = ofp_request ? *ofp_request : OFPP_NONE;
+ ofp_port = simap_get(&ofproto->ofp_requests, netdev_name);
+ ofp_port = ofp_port ? ofp_port : OFPP_NONE;
if (ofp_port >= ofproto->max_ports
|| bitmap_is_set(ofproto->ofp_port_ids, ofp_port)) {
- /* Search for a free OpenFlow port number. */
- for (ofp_port = 1; ofp_port < ofproto->max_ports; ofp_port++) {
- if (!bitmap_is_set(ofproto->ofp_port_ids, ofp_port)) {
- break;
+ bool retry = ofproto->alloc_port_no ? true : false;
+
+ /* Search for a free OpenFlow port number. We try not to
+ * immediately reuse them to prevent problems due to old
+ * flows. */
+ while (ofp_port >= ofproto->max_ports) {
+ for (ofproto->alloc_port_no++;
+ ofproto->alloc_port_no < ofproto->max_ports; ) {
+ if (!bitmap_is_set(ofproto->ofp_port_ids,
+ ofproto->alloc_port_no)) {
+ ofp_port = ofproto->alloc_port_no;
+ break;
+ }
+ }
+ if (ofproto->alloc_port_no >= ofproto->max_ports) {
+ if (retry) {
+ ofproto->alloc_port_no = 0;
+ retry = false;
+ } else {
+ return OFPP_NONE;
+ }
}
- }
- if (ofp_port >= ofproto->max_ports) {
- return OFPP_NONE;
}
}
@@ -1607,7 +1618,7 @@ dealloc_ofp_port(const struct ofproto *ofproto, uint16_t o
* pointer if the netdev cannot be opened. On success, also fills in
* 'opp'. */
static struct netdev *
-ofport_open(const struct ofproto *ofproto,
+ofport_open(struct ofproto *ofproto,
struct ofproto_port *ofproto_port,
struct ofputil_phy_port *pp)
{
@@ -1912,10 +1923,7 @@ init_ports(struct ofproto *p)
node = shash_find(&init_ofp_ports, name);
if (node) {
const struct iface_hint *iface_hint = node->data;
- uint16_t *ofp_request = xmalloc(sizeof *ofp_request);
-
- *ofp_request = iface_hint->ofp_port;
- shash_add(&p->ofp_requests, name, ofp_request);
+ simap_put(&p->ofp_requests, name, iface_hint->ofp_port);
}
netdev = ofport_open(p, &ofproto_port, &pp);
diff --git a/tests/learn.at b/tests/learn.at
index da82f51..b2bec02 100644
--- a/tests/learn.at
+++ b/tests/learn.at
@@ -161,7 +161,8 @@ AT_SETUP([learning action - fin_timeout feature])
# This is a totally artificial use of the "learn" action. The only purpose
# is to check that specifying fin_idle_timeout or fin_hard_timeout causes
# a corresponding fin_timeout action to end up in the learned flows.
-OVS_VSWITCHD_START
+OVS_VSWITCHD_START(
+ [add-port br0 p1 -- set Interface p1 type=dummy ofport_request=1])
AT_CHECK([[ovs-ofctl add-flow br0 'actions=learn(fin_hard_timeout=10, fin_idle_
AT_CHECK([ovs-appctl ofproto/trace br0 'in_port(1),eth(src=50:54:00:00:00:05,ds
AT_CHECK([ovs-ofctl dump-flows br0 table=1 | ofctl_strip], [0],
diff --git a/tests/ofproto.at b/tests/ofproto.at
index affc702..3c270d5 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -676,7 +676,7 @@ priority=0,udp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=0
fi
# OFPT_PORT_STATUS, OFPPR_ADD
- ovs-vsctl add-port br0 test -- set Interface test type=dummy
+ ovs-vsctl add-port br0 test -- set Interface test type=dummy ofport_request
if test X"$1" = X"OFPPR_ADD"; then shift;
echo >>expout "OFPT_PORT_STATUS: ADD: 1(test): addr:aa:55:aa:55:00:0x
config: PORT_DOWN
More information about the dev
mailing list