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

[Gurucharan Shetty]

On Fri, Nov 1, 2013 at 1:21 PM, Ben Pfaff <blp at nicira.com> wrote:
> 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);
>             ^
I fixed this.

>
> 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 changed the name to NETDEV_RULE_PRIORITY.

>
> 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 suppose you mean that we can just get back the mask from the 'cls_rule'
through minimask_expand() function? I did not know about this function.
I am using this in V2.

>
> 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?)

I spoke with Ben about this offline.
The other functions that call dp_netdev_lookup_flow() are:
1) dpif_netdev_flow_get().
2) dpif_netdev_flow_put().
3) dpif_netdev_flow_del().

For V2, I am changing the logic as follows.
For 1) and 3), I will do a classifier_lookup() for the supplied 'flow'
and then verify it against the flow stored in the hmap for an exact
match. Only return success if there is an exact match, else return
ENOENT. The assumption is that userspace will not send a overlapping
flow to delete/get a mega-flow in the datapath but rather send the
same flow that was used to create the mega-flow in the first place.

For 2),
Do a classifier_lookup() for the flow. If there is a match and the
'flow' used to create the original entry is the same as the one being
supplied again, return a EEXIST. If they are not the same, then there
is an overlapping flow. We should return an error to indicate that. I
am using EINVAL for a lack of better alternative.

>
> 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?
Okay. I think I made a general assumption that in_port is not
wildcarded. But I don't see
any reason why that assumption should be true. So I will add the
following incremental.

@@ -804,7 +810,12 @@ dpif_netdev_flow_mask_from_nlattrs(const struct
nlattr *key, uint32_t key_len,
     }

     in_port = flow->in_port.odp_port;
-    if (!is_valid_port_number(in_port) && in_port != ODPP_NONE) {
+    if (mask_key) {
+        if (odp_to_u32(mask->in_port.odp_port) == UINT32_MAX
+            && !is_valid_port_number(in_port) && in_port != ODPP_NONE) {
+            return EINVAL;
+        }
+    } else if (!is_valid_port_number(in_port) && in_port != ODPP_NONE) {
         return EINVAL;
     }



>
> 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.
Okay.
>
> 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.
You mean do a pointer cast like this?
match_init(&match, flow, (struct flow_wildcards *)mask);

I suppose that would be a problem if eventually 'struct
flow_wildcards' has one more member? Or did you have something else in
mind?

>
> Thanks,
>
> Ben.