On Tue, Feb 26, 2013 at 3:55 PM, Jesse Gross <[email protected]> wrote:
> On Tue, Feb 26, 2013 at 1:39 PM, Pravin B Shelar <[email protected]> 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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
