[ovs-dev] [PATCH v4 3/3] datapath: add layer 3 flow/port support

[Jesse Gross]

On Thu, Oct 9, 2014 at 3:34 AM, Lori Jakab <lojakab at cisco.com> wrote:
> On 9/12/14 12:26 AM, Jesse Gross wrote:
>> On Mon, Sep 8, 2014 at 5:43 AM, Lori Jakab <lojakab at cisco.com> wrote:
>>> --- a/datapath/flow_netlink.c
>>> +++ b/datapath/flow_netlink.c
>>> @@ -1831,6 +1831,8 @@ static int __ovs_nla_copy_actions(const struct
>>> nlattr
>>> *attr,
>>>                  case OVS_ACTION_ATTR_POP_ETH:
>>>                          if (noeth)
>>>                                  return -EINVAL;
>>> +                       if (vlan_tci != htons(0))
>>> +                               return -EINVAL;
>>
>> I wonder if this check is necessary/correct? It seems like there could
>> be other equivalent L2 tags that we don't know about, like QinQ.
>
>
> Yes, but those tags should be removed before a pop_eth() action, not as the
> part of the pop_eth() action.  At least that was my understanding to the
> above discussion.  So I added that conditional to "enforce" that we have a
> "clean" Ethernet header.

Yes, I agree that everything should already have been removed by this
point. The question was just whether it is good to have a partial
check that is not necessarily comprehensive but I guess there is no
harm in checking a common case (maybe add a comment)?

One small thing: It's maybe a little cleaner to check against
VLAN_TAG_PRESENT rather than 0 like we do in other places. I know it's
equivalent but it's perhaps a little more self-documenting.

>>> I think is_layer3 in the control block should not be used on output
>>> anymore,
>>> since we cannot guarantee it is correct.  So another change I suggest is
>>> removing the check for a packet being layer 2 or layer 3 in the various
>>> ${vport}-send() functions, and keep its use limited to receiving.
>>
>> That generally makes sense to me. For things like LISP would you then
>> check to make sure that this is an IPv4/v6 packet before
>> encapsulation?
>
>
> Well, the lisp_send() function in vport-lisp.c only receives the vport and
> sk_buff as parameters, and OVS_CB doesn't have the flow key anymore.  A
> patch I proposed initially just removed the the 'flow' member in
> ad50cb605b24495956b6a7664d379abd3912ed50, but I see the 'pkt_key' member was
> removed later by Pravin:
>
>    commit e74d48171e590b50cdcb2798a3e7c5c32313887b
>    Author: Pravin B Shelar <pshelar at nicira.com>
>    Date:   Wed Sep 17 18:58:44 2014 -0700
>
>         datapath: Remove pkt_key from OVS_CB.
>
>         OVS keeps pointer to packet key in skb->cb, but the packet key is
>         store on stack. This could make code bit tricky. So it is better to
>         get rid of the pointer.
>
>         Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
>         Acked-by: Andy Zhou <azhou at nicira.com>
>
>
> So I don't have the necessary information there to do the check.

I think skb->protocol should still be accurate, so probably the
existing check should actually be OK already (maybe with just an
additional check to look at skb->vlan_tci).

>> If so, could we apply the same mechanism on the receive
>> side to completely avoid the need for the is_layer3 flag (since I
>> think it would then only be read in a single place)?
>
>
> I need the is_layer3 flag to be able to parse the packet correctly in
> key_extract() in flow.c.  Not sure if there is another way to do it,
> especially without a flow key in SKB_CB.

I was just thinking about passing it in a parameter but I'm not sure
if that's actually cleaner or not. It's probably a little messier in
that code path but on the other hand it's less of a global object that
way. In any case, we're no longer so pressured for space in
OVS_SKB_CB.