On Mon, Nov 04, 2013 at 06:23:54AM -0800, Gurucharan Shetty wrote:
> Instead of an exact match flow table, we introduce a classifier.
> This enables mega-flows in userspace datapath.
>
> Signed-off-by: Gurucharan Shetty <gshe...@nicira.com>
dp_netdev_lookup_flow() now makes less sense to me than before. Can
the HMAP_FOR_EACH_WITH_HASH loop in there ever fail to find the
'dp_netdev_flow' that contains 'cr'?
In other words, can we equivalently fold in this?
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index bd6da5e..b7acbbb 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -749,23 +749,15 @@ dpif_netdev_port_poll_wait(const struct dpif *dpif_)
static struct dp_netdev_flow *
dp_netdev_lookup_flow(const struct dp_netdev *dp, const struct flow *flow)
{
- struct dp_netdev_flow *netdev_flow;
struct cls_rule *cr;
ovs_rwlock_wrlock(&dp->cls.rwlock);
cr = classifier_lookup(&dp->cls, flow, NULL);
ovs_rwlock_unlock(&dp->cls.rwlock);
- if (!cr) {
- return NULL;
- }
- HMAP_FOR_EACH_WITH_HASH (netdev_flow, node, cls_rule_hash(cr, 0),
- &dp->flow_table) {
- if (cls_rule_equal(&netdev_flow->cr, cr)) {
- return netdev_flow;
- }
- }
- return NULL;
+ return (cr
+ ? CONTAINER_OF(cr, struct dp_netdev_flow, cr)
+ : NULL);
}
static void
Once we do that, it makes me see that hashing based on the classifier
rule is kind of odd. We don't do any lookups based on that hmap, we
only use it for iteration. But the "get" and "del" operations could
usefully use that hmap for lookups, if the hmap actually contained the
flows instead of the cls_rules, if we fold in the following:
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index b7acbbb..3007d9f 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -123,10 +123,13 @@ struct dp_netdev_port {
/* A flow in dp_netdev's 'flow_table'. */
struct dp_netdev_flow {
- struct hmap_node node; /* Element in dp_netdev's 'flow_table'. */
+ /* Packet classification. */
+ struct cls_rule cr; /* In owning dp_netdev's 'cls'. */
+
+ /* Hash table index by unmasked flow.*/
+ struct hmap_node node; /* In owning dp_netdev's 'flow_table'. */
struct flow flow; /* The flow that created this entry. */
- struct cls_rule cr; /* The rule created for the 'flow' and inserted
- into dp_netdev's classifier. */
+
/* Statistics. */
long long int used; /* Last used time, in monotonic msecs. */
long long int packet_count; /* Number of packets matched. */
@@ -760,6 +763,20 @@ dp_netdev_lookup_flow(const struct dp_netdev *dp, const
struct flow *flow)
: NULL);
}
+static struct dp_netdev_flow *
+dp_netdev_find_flow(const struct dp_netdev *dp, const struct flow *flow)
+{
+ struct dp_netdev_flow *netdev_flow;
+
+ HMAP_FOR_EACH_WITH_HASH (netdev_flow, node, flow_hash(flow, 0),
+ &dp->flow_table) {
+ if (flow_equal(&netdev_flow->flow, flow)) {
+ return netdev_flow;
+ }
+ }
+ return NULL;
+}
+
static void
get_dpif_flow_stats(struct dp_netdev_flow *netdev_flow,
struct dpif_flow_stats *stats)
@@ -838,8 +855,8 @@ dpif_netdev_flow_get(const struct dpif *dpif,
}
ovs_mutex_lock(&dp_netdev_mutex);
- netdev_flow = dp_netdev_lookup_flow(dp, &key);
- if (netdev_flow && flow_equal(&key, &netdev_flow->flow)) {
+ netdev_flow = dp_netdev_find_flow(dp, &key);
+ if (netdev_flow) {
if (stats) {
get_dpif_flow_stats(netdev_flow, stats);
}
@@ -895,8 +912,7 @@ dp_netdev_flow_add(struct dp_netdev *dp, const struct flow
*flow,
return error;
}
- hmap_insert(&dp->flow_table, &netdev_flow->node,
- cls_rule_hash(&netdev_flow->cr, 0));
+ hmap_insert(&dp->flow_table, &netdev_flow->node, flow_hash(flow, 0));
return 0;
}
@@ -979,8 +995,8 @@ dpif_netdev_flow_del(struct dpif *dpif, const struct
dpif_flow_del *del)
}
ovs_mutex_lock(&dp_netdev_mutex);
- netdev_flow = dp_netdev_lookup_flow(dp, &key);
- if (netdev_flow && flow_equal(&key, &netdev_flow->flow)) {
+ netdev_flow = dp_netdev_find_flow(dp, &key);
+ if (netdev_flow) {
if (del->stats) {
get_dpif_flow_stats(netdev_flow, del->stats);
}
What do you think?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
