[ovs-dev] [PATCH v3] Tunnel: Cleanup old tunnel infrastructure.

Pravin Shelar pshelar at nicira.com
Wed Feb 27 00:30:45 UTC 2013


On Tue, Feb 26, 2013 at 3:55 PM, Jesse Gross <jesse at nicira.com> wrote:
> On Tue, Feb 26, 2013 at 1:39 PM, Pravin B Shelar <pshelar at nicira.com> wrote:
>> diff --git a/datapath/tunnel.c b/datapath/tunnel.c
>> index a05cf54..47cecb3 100644
>> --- a/datapath/tunnel.c
>> +++ b/datapath/tunnel.c
>> @@ -543,6 +283,9 @@ int ovs_tnl_send(struct vport *vport, struct sk_buff *skb)
>>         u8 ttl;
>>         u8 tos;
>>
>> +       if (!OVS_CB(skb)->tun_key)
>
> Can you add an unlikely() here?
>
>> +               goto error_free;
>> +
>>         /* Validate the protocol headers before we try to use them. */
>>         if (skb->protocol == htons(ETH_P_8021Q) &&
>>             !vlan_tx_tag_present(skb)) {
>
> I think we can remove this whole block of code where we validate the
> inner protocol header since all the logic that used to use them is now
> in userspace.
>
> On the receive side, I think we can also drop the ecn_decapsulate()
> function since that should also be handled in userspace.
>
ok.

>> diff --git a/datapath/vport-vxlan.c b/datapath/vport-vxlan.c
>> index 388d9fb..3bfb567 100644
>> --- a/datapath/vport-vxlan.c
>> +++ b/datapath/vport-vxlan.c
>>  static void vxlan_tunnel_release(struct vxlan_port *vxlan_port)
>>  {
>> -       vxlan_port->count--;
>> +       if (!vxlan_port)
>> +               return;
>>
>> -       if (vxlan_port->count == 0) {
>> -               /* Release old socket */
>> -               sk_release_kernel(vxlan_port->vxlan_rcv_socket->sk);
>> -               list_del(&vxlan_port->list);
>> -               kfree(vxlan_port);
>> -       }
>> +       /* Release old socket */
>> +       sk_release_kernel(vxlan_port->vxlan_rcv_socket->sk);
>> +       list_del_rcu(&vxlan_port->list);
>> +       kfree(vxlan_port);
>
> Before you were protecting vxlan_port with a deferred free by RCU.
> Don't we still need to do this?  I think it's possible to be using the
> data structure after both the socket and list entry have been removed
> (or does sk_release_kernel include a call to synchronize_rcu())?
>
right, somehow I dropped that change in v3.
I have moved sk_release_kernel() to rcu callback.

>> diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
>> index 63d1cac..2cf356b 100644
>> --- a/include/linux/openvswitch.h
>> +++ b/include/linux/openvswitch.h
>> @@ -243,6 +243,16 @@ enum {
>>
>>  #define OVS_PATCH_ATTR_MAX (__OVS_PATCH_ATTR_MAX - 1)
>>
>> +/* OVS_VPORT_ATTR_OPTIONS attributes for tunnels.
>> + */
>> +enum {
>> +       OVS_TUNNEL_ATTR_UNSPEC,
>> +       OVS_TUNNEL_ATTR_DST_PORT, /* 16-bit UDP port, used by VXLAN. */
>
> This is now used by both LISP and VXLAN, maybe we should just say that
> it is for L4 tunnels.

ok.



More information about the dev mailing list