On Wed, Nov 13, 2013 at 08:32:32AM -0800, Gurucharan Shetty wrote:
> On Tue, Nov 12, 2013 at 3:59 PM, Ben Pfaff <b...@nicira.com> 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 <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?
> 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?
Done.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
