[ovs-dev] [PATCH v1 2/6] dpif-netdev: Introduce a classifier in userspace datapath.

[Ben Pfaff]

On Fri, Nov 01, 2013 at 02:15:13AM -0700, 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>

Clang says:

    ../lib/dpif-netdev.c:638:5: error: calling function 'classifier_remove' requires exclusive lock on 'dp->cls.rwlock' [-Werror,-Wthread-safety-analysis]
        classifier_remove(&dp->cls, &netdev_flow->cr);
        ^
    ../lib/dpif-netdev.c:752:10: error: calling function 'classifier_lookup' requires shared lock on 'dp->cls.rwlock' [-Werror,-Wthread-safety-analysis]
        cr = classifier_lookup(&dp->cls, flow, NULL);
             ^
    ../lib/dpif-netdev.c:882:5: error: calling function 'classifier_insert' requires exclusive lock on 'dp->cls.rwlock' [-Werror,-Wthread-safety-analysis]
        classifier_insert(&dp->cls, &netdev_flow->cr);
        ^
    ../lib/dpif-netdev.c:886:9: error: calling function 'classifier_remove' requires exclusive lock on 'dp->cls.rwlock' [-Werror,-Wthread-safety-analysis]
            classifier_remove(&dp->cls, &netdev_flow->cr);
            ^

All the flows in the classifier will have the same priority, right?  I
think so.  The "default" here, to my eyes, implies that the default
can be overridden:
> +/* By default, choose a priority in the middle. */
> +#define NETDEV_DEFAULT_PRIORITY 0x8000

I know why struct dp_netdev_flow needs 'flow' in addition to a
cls_rule: because a cls_rule always zeros out the bits in the flow,
and dpif-netdev needs to be able to report the original values of
those bits.  But why does struct dp_netdev_flow need another copy of
the mask?

I don't really understand dp_netdev_lookup_flow().  It is being called
in two different and conflicting contexts.  dp_netdev_port_input()
uses it to determine how to handle an incoming packet, which correctly
would just call classifier_lookup().  The other functions that call
dp_netdev_lookup_flow() should just call
classifier_find_rule_exactly().  (Or am I missing something
important?)

Also in dpif_netdev_flow_mask_from_nlattrs(), should we now only check
that the in_port is valid, if the in_port is not wildcarded?

The wrapper in the conditional here looks odd to me, because the
indentation does not reflect the logical structure of the test:
    if (odp_flow_key_to_flow(key, key_len, flow) || (mask_key &&
        odp_flow_key_to_mask(mask_key, mask_key_len, mask, flow))) {
I would write it as, say:
    if (odp_flow_key_to_flow(key, key_len, flow)
        || (mask_key
            && odp_flow_key_to_mask(mask_key, mask_key_len, mask, flow))) {
although || and && at the ends of lines instead of at the beginning is
fine too if you prefer.

In dp_netdev_flow_add(), copying the mask into a flow_wildcards
structure seems wasteful.  I think that the caller can easily provide
the mask in that form, without copying.

Thanks,

Ben.