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

[Andy Zhou]

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.