[ovs-dev] [recirc datapath V2 3/3] datapath: add recirc action

Andy Zhou azhou at nicira.com
Wed Apr 16 23:46:38 UTC 2014


On Wed, Apr 16, 2014 at 4:15 PM, Jesse Gross <jesse at nicira.com> wrote:
> On Wed, Apr 16, 2014 at 3:22 PM, Andy Zhou <azhou at nicira.com> wrote:
>> On Wed, Apr 16, 2014 at 3:07 PM, Jesse Gross <jesse at nicira.com> wrote:
>>> On Wed, Apr 16, 2014 at 2:32 PM, Andy Zhou <azhou at nicira.com> wrote:
>>>> On Wed, Apr 16, 2014 at 2:25 PM, Jesse Gross <jesse at nicira.com> wrote:
>>>>> On Wed, Apr 16, 2014 at 11:13 AM, Andy Zhou <azhou at nicira.com> wrote:
>>>>>> On Wed, Apr 16, 2014 at 11:03 AM, Pravin Shelar <pshelar at nicira.com> wrote:
>>>>>>> On Wed, Apr 16, 2014 at 6:51 AM, Andy Zhou <azhou at nicira.com> 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.

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.



More information about the dev mailing list