On Nov 1, 2012, at 11:24 AM, Ben Pfaff <[email protected]> wrote:
>> @@ -47,6 +47,11 @@ v1.9.0 - xx xxx xxxx
>> - The ofproto library is now responsible for assigning OpenFlow port
>> numbers. An ofproto implementation should assign them when
>> port_construct() is called.
>> + - All dpif-based bridges of a particular type share a common
>> + datapath called "ovs-<type>", where "<type>" is the type of dpif.
>
> It seems likely to me that many users aren't really aware of types.
> So perhaps substitute an example, like this:
>> + - All dpif-based bridges of a particular type share a common
>> + datapath called "ovs-<type>", e.g. "ovs-system".
Okay.
> I think that the new #include "svec.h" is not necessary, because this
> version does not use svecs.
Thanks.
> It looks like "dpif-provider.h" is necessary only to access
> ->full_name and ->base_name in struct dpif. I think it would be
> better to use dpif_name() and dpif_base_name() instead, to avoid a
> minor layering violation.
We also need to get the class. I sent out a patch for a new dpif_type()
function to get that information, which you've already approved.
> The update to set_sflow() made me wonder what the dpif_sflow code does
> with the dpif. The answer is that it does nothing with the dpif, it
> just stores away a pointer and never references it again. So it would
> be better to update ofproto-dpif-sflow.c to just remove that parameter
> from dpif_sflow_create(), probably in a preparatory patch.
Okay, I took care of that in a patch you already approved.
> I think that the code in port_dump_next() should be made a little more
> robust. Currently, I believe that if there is some discrepancy
> between ofproto->ports and what is in the kernel datapath (e.g. some
> admin meddling with "ovs-dpctl del-port"), then that discrepancy will
> be returned as an error to the caller, who will interpret it as
> meaning that there are no more ports and thus not call back to find
> out any more. So I think that port_dump_next() probably should have a
> retry loop for when port_query_by_name() fails.
Makes sense. Done.
> update_stats() ends up calling odp_port_to_ofport() twice, once via
> odp_port_to_ofproto_dpif(), once via odp_port_to_ofp_port(). It would
> be more efficient to do this just once, directly. I don't think the
> code would look any worse for it, and perhaps even a little better.
Good point. More on that in the next comment...
> Similarly, odp_port_to_ofproto_dpif() does the same thing, through
> odp_port_to_ofproto_dpif() and ofproto_dpif_vsp_adjust(). This one is
> more important because it is on the flow setup fast path. I think
> that doing this lookup twice is likely to show up in "ovs-benchmark
> rate" results.
As we discussed off-line, I think you meant handle_miss_upcalls(). It looks
like handle_sflow_upcall() is also affected. Making these changes allowed me
to get rid of odp_port_to_ofproto_dpif(), and I think has a couple other
performance-related benefits. Thanks!
Below, you should find an incremental.
--Justin
-=-=-=-=-=-=-=-=-=-
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ae2c14e..283aea9 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -618,8 +618,8 @@ struct dpif_backer {
/* All existing ofproto_backer instances, indexed by ofproto->up.type. */
static struct shash all_dpif_backers = SHASH_INITIALIZER(&all_dpif_backers);
-static struct ofproto_dpif *
-odp_port_to_ofproto_dpif(const struct dpif *, uint32_t odp_port);
+static struct ofport_dpif *
+odp_port_to_ofport(const struct dpif_backer *, uint32_t odp_port);
struct ofproto_dpif {
struct hmap_node all_ofproto_dpifs_node; /* In 'all_ofproto_dpifs'. */
@@ -703,7 +703,7 @@ static void update_learning_table(struct ofproto_dpif *,
struct ofbundle *);
/* Upcalls. */
#define FLOW_MISS_MAX_BATCH 50
-static int handle_upcalls(struct dpif *, unsigned int max_batch);
+static int handle_upcalls(struct dpif_backer *, unsigned int max_batch);
/* Flow expiration. */
static int expire(struct dpif_backer *);
@@ -881,7 +881,7 @@ type_run_fast(const char *type)
* presumably for real traffic as well. */
work = 0;
while (work < FLOW_MISS_MAX_BATCH) {
- int retval = handle_upcalls(backer->dpif, FLOW_MISS_MAX_BATCH - work);
+ int retval = handle_upcalls(backer, FLOW_MISS_MAX_BATCH - work);
if (retval <= 0) {
return -retval;
}
@@ -2980,13 +2980,17 @@ port_dump_next(const struct ofproto *ofproto_ OVS_UNUSED
struct port_dump_state *state = state_;
struct sset_node *node;
- node = sset_at_position(&ofproto->ports, &state->bucket,
- &state->offset);
- if (!node) {
- return EOF;
+ while ((node = sset_at_position(&ofproto->ports, &state->bucket,
+ &state->offset))) {
+ int error;
+
+ error = port_query_by_name(ofproto_, node->name, port);
+ if (error != ENODEV) {
+ return error;
+ }
}
- return port_query_by_name(ofproto_, node->name, port);
+ return EOF;
}
static int
@@ -3375,7 +3379,8 @@ handle_flow_miss(struct flow_miss *miss, struct flow_miss_
* port, sets flow->vlan_tci correctly for the VLAN of the VLAN splinter
* port, and pushes a VLAN header onto 'packet' (if it is nonnull). The
* caller must have called odp_flow_key_to_flow() and supply 'fitness' and
- * 'flow' from its output.
+ * 'flow' from its output. The 'flow' argument must have had the "in_port"
+ * member converted to the OpenFlow number.
*
* Sets '*initial_tci' to the VLAN TCI with which the packet was really
* received, that is, the actual VLAN TCI extracted by odp_flow_key_to_flow().
@@ -3387,7 +3392,6 @@ ofproto_dpif_vsp_adjust(const struct ofproto_dpif *ofproto
struct flow *flow, ovs_be16 *initial_tci,
struct ofpbuf *packet)
{
- flow->in_port = odp_port_to_ofp_port(ofproto, flow->in_port);
if (fitness == ODP_FIT_ERROR) {
return fitness;
}
@@ -3422,7 +3426,7 @@ ofproto_dpif_vsp_adjust(const struct ofproto_dpif *ofproto
}
static void
-handle_miss_upcalls(struct dpif *dpif, struct dpif_upcall *upcalls,
+handle_miss_upcalls(struct dpif_backer *backer, struct dpif_upcall *upcalls,
size_t n_upcalls)
{
struct dpif_upcall *upcall;
@@ -3451,12 +3455,13 @@ handle_miss_upcalls(struct dpif *dpif, struct dpif_upcal
struct flow_miss *existing_miss;
enum odp_key_fitness fitness;
struct ofproto_dpif *ofproto;
+ struct ofport_dpif *port;
struct flow flow;
uint32_t hash;
fitness = odp_flow_key_to_flow(upcall->key, upcall->key_len, &flow);
- ofproto = odp_port_to_ofproto_dpif(dpif, flow.in_port);
- if (!ofproto) {
+ port = odp_port_to_ofport(backer, flow.in_port);
+ if (!port) {
/* Received packet on port for which we couldn't associate
* an ofproto. This can happen if a port is removed while
* traffic is being received. Print a rate-limited message
@@ -3465,6 +3470,8 @@ handle_miss_upcalls(struct dpif *dpif, struct dpif_upcall
flow.in_port);
continue;
}
+ ofproto = ofproto_dpif_cast(port->up.ofproto);
+ flow.in_port = port->up.ofp_port;
/* Obtain metadata and check userspace/kernel agreement on flow match,
* then set 'flow''s header pointers. */
@@ -3506,7 +3513,7 @@ handle_miss_upcalls(struct dpif *dpif, struct dpif_upcall
for (i = 0; i < n_ops; i++) {
dpif_ops[i] = &flow_miss_ops[i].dpif_op;
}
- dpif_operate(dpif, dpif_ops, n_ops);
+ dpif_operate(backer->dpif, dpif_ops, n_ops);
/* Free memory and update facets. */
for (i = 0; i < n_ops; i++) {
@@ -3567,22 +3574,31 @@ classify_upcall(const struct dpif_upcall *upcall)
}
static void
-handle_sflow_upcall(struct dpif *dpif, const struct dpif_upcall *upcall)
+handle_sflow_upcall(struct dpif_backer *backer,
+ const struct dpif_upcall *upcall)
{
struct ofproto_dpif *ofproto;
union user_action_cookie cookie;
enum odp_key_fitness fitness;
+ struct ofport_dpif *port;
ovs_be16 initial_tci;
struct flow flow;
uint32_t odp_in_port;
fitness = odp_flow_key_to_flow(upcall->key, upcall->key_len, &flow);
- odp_in_port = flow.in_port;
- ofproto = odp_port_to_ofproto_dpif(dpif, odp_in_port);
- if (!ofproto || !ofproto->sflow) {
+
+ port = odp_port_to_ofport(backer, flow.in_port);
+ if (!port) {
+ return;
+ }
+
+ ofproto = ofproto_dpif_cast(port->up.ofproto);
+ if (!ofproto->sflow) {
return;
}
+ odp_in_port = flow.in_port;
+ flow.in_port = port->up.ofp_port;
fitness = ofproto_dpif_vsp_adjust(ofproto, fitness, &flow,
&initial_tci, upcall->packet);
if (fitness == ODP_FIT_ERROR) {
@@ -3595,7 +3611,7 @@ handle_sflow_upcall(struct dpif *dpif, const struct dpif_u
}
static int
-handle_upcalls(struct dpif *dpif, unsigned int max_batch)
+handle_upcalls(struct dpif_backer *backer, unsigned int max_batch)
{
struct dpif_upcall misses[FLOW_MISS_MAX_BATCH];
struct ofpbuf miss_bufs[FLOW_MISS_MAX_BATCH];
@@ -3614,7 +3630,7 @@ handle_upcalls(struct dpif *dpif, unsigned int max_batch)
ofpbuf_use_stub(buf, miss_buf_stubs[n_misses],
sizeof miss_buf_stubs[n_misses]);
- error = dpif_recv(dpif, upcall, buf);
+ error = dpif_recv(backer->dpif, upcall, buf);
if (error) {
ofpbuf_uninit(buf);
break;
@@ -3627,7 +3643,7 @@ handle_upcalls(struct dpif *dpif, unsigned int max_batch)
break;
case SFLOW_UPCALL:
- handle_sflow_upcall(dpif, upcall);
+ handle_sflow_upcall(backer, upcall);
ofpbuf_uninit(buf);
break;
@@ -3638,7 +3654,7 @@ handle_upcalls(struct dpif *dpif, unsigned int max_batch)
}
/* Handle deferred MISS_UPCALL processing. */
- handle_miss_upcalls(dpif, misses, n_misses);
+ handle_miss_upcalls(backer, misses, n_misses);
for (i = 0; i < n_misses; i++) {
ofpbuf_uninit(&miss_bufs[i]);
}
@@ -3789,6 +3805,7 @@ update_stats(struct dpif_backer *backer)
struct subfacet *subfacet;
enum odp_key_fitness fitness;
struct ofproto_dpif *ofproto;
+ struct ofport_dpif *port;
uint32_t key_hash;
fitness = odp_flow_key_to_flow(key, key_len, &flow);
@@ -3796,8 +3813,8 @@ update_stats(struct dpif_backer *backer)
continue;
}
- ofproto = odp_port_to_ofproto_dpif(backer->dpif, flow.in_port);
- if (!ofproto) {
+ port = odp_port_to_ofport(backer, flow.in_port);
+ if (!port) {
/* This flow is for a port for which we couldn't associate an
* ofproto. This can happen if a port is removed while
* traffic is being received. Print a rate-limited message
@@ -3808,8 +3825,9 @@ update_stats(struct dpif_backer *backer)
continue;
}
+ ofproto = ofproto_dpif_cast(port->up.ofproto);
+ flow.in_port = port->up.ofp_port;
key_hash = odp_flow_key_hash(key, key_len);
- flow.in_port = odp_port_to_ofp_port(ofproto, flow.in_port);
subfacet = subfacet_find(ofproto, key, key_len, key_hash, &flow);
switch (subfacet ? subfacet->path : SF_NOT_INSTALLED) {
@@ -7156,6 +7174,7 @@ ofproto_unixctl_trace(struct unixctl_conn *conn, int argc,
}
fitness = odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow);
+ flow.in_port = odp_port_to_ofp_port(ofproto, flow.in_port);
/* Convert odp_key to flow. */
error = ofproto_dpif_vsp_adjust(ofproto, fitness, &flow,
@@ -7841,25 +7860,6 @@ odp_port_to_ofp_port(const struct ofproto_dpif *ofproto,
}
}
-static struct ofproto_dpif *
-odp_port_to_ofproto_dpif(const struct dpif *dpif, uint32_t odp_port)
-{
- struct dpif_backer *backer;
- struct ofport_dpif *port;
-
- backer = shash_find_data(&all_dpif_backers, dpif_type(dpif));
- if (!backer) {
- return NULL;
- }
-
- port = odp_port_to_ofport(backer, odp_port);
- if (!port) {
- return NULL;
- }
-
- return ofproto_dpif_cast(port->up.ofproto);
-}
-
const struct ofproto_class ofproto_dpif_class = {
init,
enumerate_types,
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
