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?)
> > 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.
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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
