[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