On Wed, Apr 16, 2014 at 5:03 PM, Jesse Gross <[email protected]> wrote: > 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.
Let me think more about your proposal. It seems obvious I am missing some insights you have. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
