[ovs-dev] [PATCH 1/1] Add support for tun_key to OVS datapath

[Kyle Mestery (kmestery)]

On Sep 4, 2012, at 8:40 PM, Jesse Gross wrote:
> On Tue, Sep 4, 2012 at 12:24 PM, Kyle Mestery (kmestery)
> <kmestery at cisco.com> wrote:
>> On Sep 3, 2012, at 3:43 PM, Jesse Gross wrote:
>>> On Wed, Aug 29, 2012 at 7:00 AM, Kyle Mestery <kmestery at cisco.com> wrote:
>>>> diff --git a/datapath/actions.c b/datapath/actions.c
>>>> index 208f260..83382b9 100644
>>>> --- a/datapath/actions.c
>>>> +++ b/datapath/actions.c
>>>> @@ -343,7 +343,11 @@ static int execute_set_action(struct sk_buff *skb,
>>>>               break;
>>>> 
>>>>       case OVS_KEY_ATTR_TUN_ID:
>>>> -               OVS_CB(skb)->tun_id = nla_get_be64(nested_attr);
>>>> +               OVS_CB(skb)->tun_key->tun_id = nla_get_be64(nested_attr);
>>> 
>>> I think this doesn't quite work out so simply as a compatibility layer
>>> because tun_key is either NULL or pointing directly into the actions
>>> of the flow, which are supposed to be immutable.  Long term this won't
>>> be a problem but in the meantime we'll have a figure out a way to fake
>>> up an appropriate tun_key.
>>> 
>> So, I think what we should do here is allocate OVS_CB(skb)->tun_key in
>> this case. We could modify struct ovs_key_ipv4_tunnel to have a flag value
>> indicating if it was allocated or not, to allow for correct handling at the time
>> of termination. Thoughts?
> 
> I think we probably could keep one on the stack when we execute
> actions.  That would avoid the need to track allocation and freeing
> later.
> 
Actually, after thinking about this, I'm unclear as to how to proceed here. Don't we
need to ensure the tun_id is passed with the skbuff when outputting the packet?
If so, how does just keeping it on the stack help here, since all the output code is
now changed to grab it from the OVS_SB(skbuff) pointer?

>>>> diff --git a/datapath/flow.h b/datapath/flow.h
>>>> index 5be481e..bab5363 100644
>>>> --- a/datapath/flow.h
>>>> +++ b/datapath/flow.h
>>>> struct sw_flow_key {
>>>>       struct {
>>>> -               __be64  tun_id;         /* Encapsulating tunnel ID. */
>>>> +               struct ovs_key_ipv4_tunnel tun_key;  /* Encapsulating tunnel key. */
>>> 
>>> One thing that I think we should look into is how we can both shrink
>>> this fields down for the purposes of matching (for example, I believe
>>> the flags fields is always zero here) and move it to the end of the
>>> struct (so that non-tunneled packets don't have to process it at all).
>>> We've seen that hashing and comparison are the most expensive
>>> operations in the OVS datapath so we should try to avoid introducing
>>> extra bytes in places where they aren't needed.  Obviously, this
>>> becomes even more important with IPv6 tunneling.  Normally I wouldn't
>>> worry about optimization of a new feature until the end but since I'd
>>> like to start putting in patches as they are ready it's important that
>>> we don't introduce regressions for existing behavior.
>>> 
>> So, as a start, move it out of the "struct phy" here into something at the end
>> of "struct sw_flow_key"?
> 
> Yeah.  The issue is that the struct is already variable sized, so it's
> difficult to know where the end of the current struct is.  We'll
> probably need some kind of function to access it.