On Sep 4, 2012, at 8:40 PM, Jesse Gross wrote:
> On Tue, Sep 4, 2012 at 12:24 PM, Kyle Mestery (kmestery)
> <kmest...@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 <kmest...@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.
>
Got it, will make this change.
>>>> 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.
OK, sounds good. I'll make both these changes and submit a V3 of this patch,
hopefully tomorrow.
Thanks,
Kyle
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
