On Mon, Nov 4, 2013 at 10:03 AM, Ben Pfaff <b...@nicira.com> wrote:
> On Sun, Nov 03, 2013 at 06:56:05PM -0800, Gurucharan Shetty wrote:
>> On Fri, Nov 1, 2013 at 1:21 PM, Ben Pfaff <b...@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 <gshe...@nicira.com>
>> >
>> > 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.
>
> That's what I meant. Thanks.
>
>> > 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.
>
> OK.
>
> I hope that these errors match what the kernel datapath uses. (Did you
> check?)
I spoke to Andy and checked the kernel datapath. Kernel datapath does
it slightly different for dpif_netdev_flow_put()
It does the following.
* If the flow look up is a success and the put flow was with the
CREATE flag, return a EXIST (irrespective of whether it is a exact
flow or overlap flow)
* If the flow lookup was a success and the put flow was with modify
flag, only modify if the flows are equal. Else, return a EINVAL.
I will change the userspace datapath do the same too.
>
>> > 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.
>
> OK. Another option is to force the in_port not be wildcarded. (I think
> that the kernel does that.) That might be a better option because I am
> not sure how one would implement a partially masked in_port.
Okay. I will do that. I guess, now I will have to check the in_port's
validity irrespective of whether it was originally masked or not.
>
> I think that the conditions below could be simplified a little, from:
>> 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;
>> }
>
> to:
>
> if ((!mask_key || odp_to_u32(mask->in_port.odp_port) == UINT32_MAX)
> && !is_valid_port_number(in_port) && in_port != ODPP_NONE)
>
>> > 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?
>
> No cast should be needed. I mean that the caller can declare a "struct
> flow_wildcards" and fill in the struct flow member, then pass the
> address of the flow_wildcards container.
Thanks. I will send a V2.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
