On Wed, Apr 16, 2014 at 4:46 PM, Andy Zhou <[email protected]> wrote: > On Wed, Apr 16, 2014 at 4:15 PM, Jesse Gross <[email protected]> wrote: >> On Wed, Apr 16, 2014 at 3:22 PM, Andy Zhou <[email protected]> wrote: >>> On Wed, Apr 16, 2014 at 3:07 PM, Jesse Gross <[email protected]> wrote: >>>> On Wed, Apr 16, 2014 at 2:32 PM, Andy Zhou <[email protected]> wrote: >>>>> On Wed, Apr 16, 2014 at 2:25 PM, Jesse Gross <[email protected]> wrote: >>>>>> On Wed, Apr 16, 2014 at 11:13 AM, Andy Zhou <[email protected]> wrote: >>>>>>> On Wed, Apr 16, 2014 at 11:03 AM, Pravin Shelar <[email protected]> >>>>>>> wrote: >>>>>>>> On Wed, Apr 16, 2014 at 6:51 AM, Andy Zhou <[email protected]> wrote: >>>>>>>>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c >>>>>>>>> index 0a1effa..883d9bf 100644 >>>>>>>>> --- a/datapath/flow_netlink.c >>>>>>>>> +++ b/datapath/flow_netlink.c >>>>>>>>> @@ -474,6 +476,23 @@ static int metadata_from_nlattrs(struct >>>>>>>>> sw_flow_match *match, u64 *attrs, >>>>>>>>> *attrs &= ~(1ULL << OVS_KEY_ATTR_DP_HASH); >>>>>>>>> } >>>>>>>>> >>>>>>>>> + if (*attrs & (1ULL << OVS_KEY_ATTR_RECIRC_ID)) { >>>>>>>>> + u32 recirc_id = >>>>>>>>> nla_get_u32(a[OVS_KEY_ATTR_RECIRC_ID]); >>>>>>>>> + >>>>>>>>> + if (is_mask && (recirc_id > 0 && recirc_id < >>>>>>>>> UINT_MAX)) { >>>>>>>>> + OVS_NLERR("Reicrc_id mask is neither >>>>>>>>> wildcard, nor exact match \n"); >>>>>>>>> + return -EINVAL; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + SW_FLOW_KEY_PUT(match, recirc_id, recirc_id, is_mask); >>>>>>>>> + *attrs &= ~(1ULL << OVS_KEY_ATTR_RECIRC_ID); >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + if (is_mask) { >>>>>>>>> + /* Always exact match recirc_id. */ >>>>>>>>> + SW_FLOW_KEY_PUT(match, recirc_id, UINT_MAX, is_mask); >>>>>>>>> + } >>>>>>>>> + >>>>>>>> >>>>>>>> If the recirc_id is zero it should be safe to ignore it. Can you >>>>>>>> explain need for exact match for recirc_id ? >>>>>>> >>>>>>> Consider the following two flows: >>>>>>> >>>>>>> 1) in_port = 1 ; actions= A >>>>>>> 2) recirc_id = 2, in_port =1 : Actions=B >>>>>>> >>>>>>> Without forcing exact match of recirc_id, a packet matches 2) could >>>>>>> also match 1) >>>>>> >>>>>> Presumably anybody that is using recirculation would already make this >>>>>> exact match already, right? Is there any possible use case for a >>>>>> partial mask? >>>>>> >>>>> Partial mask is not allowed for recirc_id. Either exact match or wildcard. >>>>> I am interpreting a missing attribute to be exact match 0. I think Pravin >>>>> is arguing for interpreting it as a wildcard. What do you think? >>>> >>>> If we don't think there is ever a case for partial masks then it's not >>>> clear why it's OK to have something fully masked out. Presumably >>>> somebody that is wildcarding the recirculation ID isn't doing >>>> recirculation, in which case matching on ID 0 is fine. However, if >>>> that's the case then I think that we can enforce any masks explicitly >>>> supplied for recirc_id are exact match (rather than only disallowing >>>> partials). >>>> >>>> The thing that bothers me about this is that it's introducing a >>>> somewhat special case for something that the kernel doesn't really >>>> care about: there's no inherent reason why we can do a partial match >>>> on the recirc_id, it's just that we can't think of a good reason now. >>>> As we saw with the introduction of megaflows, special cases can come >>>> back to bite us later on. >>> >>> Good points. How about we do the following: >>> >>> 1. If recirc_id is mssing, we treat it as exact match 0, Older user space >>> will >>> not send recirc_id, and currently we can't be 100% sure it is an older >>> kernel. >>> This is also nice to the user space since they don't always have to be >>> mindful >>> of kernel version when generating regular flows. >>> >>> 2. When ever a recirc_id mask is transmitted, we will obey it. >>> Recirc_id is now just >>> like any other field. >>> >>> 3. If recirc_id mask is missing, we can also treat them as a wildcard >>> just like any other >>> field. User space has to make sure whenever recirc_id is send, a >>> proper mask should also >>> be used. >>> >>> With this, the only special case is the interpretation of missing >>> attribute of recirci_id. >> >> To summarize, the only thing special is that a missing recirc_id is >> exact match instead of wildcard, right? The case of missing recirc_id >> should only come up when recirculation isn't in use because either >> userspace doesn't support it or no features that require it are in >> use. In those cases, the recirc_id will always be zero so I don't know >> that it makes a difference whether it is exact or wildcarded. >> >> I think we're also OK with mismatched kernel/userspace version: >> * Old kernel/new userspace: When using a recirculate feature, test if >> the kernel supports it. In this case, use recirc_id on all flows. If >> not in use, recirc_id is never used (wildcarded). >> * New kernel/old userspace: Userspace will never use the recirculate >> action since it doesn't know about it. In this case, it will never >> appear in the flow definition from either side and will always be >> wildcarded. >> >> Potentially the only corner case that I see is if a recirculate >> feature is initially not used and is later turned on. If we weren't >> initially setting recirc_id with a mask, then we would need to >> invalidate all flows but if they were defaulting to 0/exact we >> wouldn't need to do that. I'm not sure this is worth it though. > > Yes, this is the reason. Currently kernel does not need to know about > user space. It would be nice if we can keep it this way. This is a solvable > problem by moving responsibility to the user space to always use the proper > flow > format. This is a theoretical problem, probably not likely to happen > in real life, at > least not for OVS user space.
I don't understand this. None of these scenarios involve the kernel doing anything different depending on userspace version - it's purely based on whether userspace uses this new action or not. It sounds like you are assuming that the kernel would always send the recirc_id, even if it is zero. I don't think this is necessary. However, even if we were to do this the compatibility layer should handle it on older userspaces by echoing it back. > A practical issue I can think of is for testing, The flow dump results > will be different > depends on if datapath support recirc or not, even when the test has nothing > to do with recirc. Are you saying that these keys would be passed down in all cases, to avoid the revalidation that I was describing above? In any case, I don't think this is really all that significant. The human readable dump of the flows has never been a stable API. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
