[ovs-dev] [PATCH 3/3] datapath: vxlan: Optimize vxlan rcv

Jesse Gross jesse at nicira.com
Fri Sep 6 19:38:54 UTC 2013


On Fri, Sep 6, 2013 at 12:30 PM, Pravin Shelar <pshelar at nicira.com> wrote:
> On Fri, Sep 6, 2013 at 12:15 PM, Jesse Gross <jesse at nicira.com> wrote:
>> On Fri, Sep 6, 2013 at 11:39 AM, Pravin B Shelar <pshelar at nicira.com> wrote:
>>> diff --git a/datapath/linux/compat/vxlan.c b/datapath/linux/compat/vxlan.c
>>> index db14f2f..3d4fec3 100644
>>> --- a/datapath/linux/compat/vxlan.c
>>> +++ b/datapath/linux/compat/vxlan.c
>>> @@ -124,7 +124,8 @@ static int vxlan_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
>>>         if (iptunnel_pull_header(skb, VXLAN_HLEN, htons(ETH_P_TEB)))
>>>                 goto drop;
>>>
>>> -       vs = vxlan_find_sock(sock_net(sk), inet_sport(sk));
>>> +       smp_read_barrier_depends();
>>> +       vs = (struct vxlan_sock *)sk->sk_user_data;
>>>         if (!vs)
>>>                 goto drop;
>>
>> I think this is a correct backport of the upstream commit but this
>> part worries me a little bit because it is missing the ACCESS_ONCE
>> part of rcu_dereference. It's unlikely due to the code structure but
>> in theory the compiler could decide to omit 'vs' completely causing
>> refetches across the NULL check.
>
> How compiler can omit 'vs' when NULL check has direct dependency on it?

I mean rather than allocating some register or additional local
storage on the stack, the compiler could treat 'vs' as an alias for
sk->sk_user_data. Then any time that somebody accesses 'vs' it would
fetch sk->sk_user_data instead, which could change.



More information about the dev mailing list