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

[Jesse Gross]

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.

>>> 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?

You also have a typo in the error message ("Reicrc_id").