[ovs-dev] [PATCH v2] dpif-netdev: Introduce a classifier in userspace datapath.

[Ben Pfaff]

On Wed, Nov 13, 2013 at 08:32:32AM -0800, Gurucharan Shetty wrote:
> On Tue, Nov 12, 2013 at 3:59 PM, Ben Pfaff <blp at 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 <gshetty at 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.