On Tue, Nov 12, 2013 at 3:59 PM, Ben Pfaff <[email protected]> wrote:
> 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 <[email protected]>
>
> 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?
Yes. That will work.
>
> 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?
This looks fine to me as well (tests pass). If you find everything
else alright, can you please fold this in and apply?
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
