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.
>>> 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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
