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

[Andy Zhou]

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/actions.c b/datapath/actions.c
>>>> index cb239c8..4182b3d 100644
>>>> --- a/datapath/actions.c
>>>> +++ b/datapath/actions.c
>>>> +static int execute_recirc(struct datapath *dp, struct sk_buff *skb,
>>>> +                                const struct nlattr *a)
>>>> +{
>>>> +       struct sw_flow_key recirc_key;
>>>> +       uint16_t in_port = OVS_CB(skb)->pkt_key->phy.in_port;
>>>> +       uint32_t dp_hash = OVS_CB(skb)->pkt_key->dp_hash;
>>>> +       struct vport *p;
>>>> +       int err;
>>>> +
>>>> +       err = ovs_flow_extract(skb, in_port, &recirc_key);
>>>> +       if (err)
>>>> +               return err;
>>>> +
>>>> +       p = ovs_vport_rcu(dp, in_port);
>>>> +       if (unlikely(!p)) {
>>>> +               kfree_skb(skb);
>>>> +               return -ENODEV;
>>>> +       }
>>>> +
>>> We can store in-port in ovs-cb to avoid hash lookup.
>> Do you mean vport * ? Sure if ovs-cb can afford to lose 8 bytes.
>
> Space isn't really an issue at this point, so it should be fine.
Will do.
>
>>>> 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?

> You also have a typo in the error message ("Reicrc_id").
Thanks for the catching it. Will fix.