[ovs-dev] [PATCH] Tunnel: Cleanup old tunnel infrastructure.
Kyle Mestery (kmestery)
kmestery at cisco.com
Wed Feb 20 19:56:08 UTC 2013
On Feb 20, 2013, at 12:44 PM, Jesse Gross <jesse at nicira.com> wrote:
> Here are a couple of small comments that I'd already written. I
> haven't gone through the main part of the patch yet but I figured that
> I might as well send them if you are going to respin the patch.
>
> On Tue, Feb 19, 2013 at 5:35 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
>> diff --git a/datapath/actions.c b/datapath/actions.c
>> index f638ffc..cf44de3 100644
>> --- a/datapath/actions.c
>> +++ b/datapath/actions.c
>> @@ -439,22 +439,6 @@ static int execute_set_action(struct sk_buff *skb,
>> skb_set_mark(skb, nla_get_u32(nested_attr));
>> break;
>>
>> - case OVS_KEY_ATTR_TUN_ID:
>> - /* If we're only using the TUN_ID action, store the value in a
>> - * temporary instance of struct ovs_key_ipv4_tunnel on the stack.
>> - * If both IPV4_TUNNEL and TUN_ID are being used together we
>> - * can't write into the IPV4_TUNNEL action, so make a copy and
>> - * write into that version.
>> - */
>> - if (!OVS_CB(skb)->tun_key)
>> - memset(tun_key, 0, sizeof(*tun_key));
>> - else if (OVS_CB(skb)->tun_key != tun_key)
>> - memcpy(tun_key, OVS_CB(skb)->tun_key, sizeof(*tun_key));
>> - OVS_CB(skb)->tun_key = tun_key;
>> -
>> - OVS_CB(skb)->tun_key->tun_id = nla_get_be64(nested_attr);
>> - break;
>
> I think we can further simplify this by removing the temporary tun_key
> that we are keeping on the stack in ovs_execute_actions() and passing
> around.
>
>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>> index 7ee31a2..12e85b2 100644
>> --- a/include/linux/openvswitch.h
>> +++ b/include/linux/openvswitch.h
>> @@ -242,6 +242,18 @@ enum {
>>
>> #define OVS_PATCH_ATTR_MAX (__OVS_PATCH_ATTR_MAX - 1)
>>
>> +/* OVS_VPORT_ATTR_OPTIONS attributes for tunnels.
>> + *
>> + * OVS_TUNNEL_ATTR_DST_PORT is useful for vxlan.
>
> I'm not sure that we need this comment about VXLAN here since it's
> probably easier to just describe each element next to it.
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Another thing I just noticed:
I don't see where tnl_vport->dst_port is set when creating a VXLAN
port. I see it set when setting tunnel options, but not during create.
I think this explains why when I tested this patch with VXLAN I see
the destination port being zero during transmit.
More information about the dev
mailing list