linux-kernel - Re: [PATCH] net: optimize cmpxchg in ip_idents

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1b3aaddf-22f5-1846-90f1-42e68583c1e4@gmail.com>
Date:   Thu, 16 Jan 2020 07:19:05 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     David Miller <davem@...emloft.net>, zhangshaokun@...ilicon.com
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        jinyuqi@...wei.com, kuznet@....inr.ac.ru, yoshfuji@...ux-ipv6.org,
        edumazet@...gle.com, guoyang2@...wei.com
Subject: Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve



On 1/16/20 7:12 AM, Eric Dumazet wrote:
> 
> 
> On 1/16/20 4:27 AM, David Miller wrote:
>> From: Shaokun Zhang <zhangshaokun@...ilicon.com>
>> Date: Wed, 15 Jan 2020 11:23:40 +0800
>>
>>> From: Yuqi Jin <jinyuqi@...wei.com>
>>>
>>> atomic_try_cmpxchg is called instead of atomic_cmpxchg that can reduce
>>> the access number of the global variable @p_id in the loop. Let's
>>> optimize it for performance.
>>>
>>> Cc: "David S. Miller" <davem@...emloft.net>
>>> Cc: Alexey Kuznetsov <kuznet@....inr.ac.ru>
>>> Cc: Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>
>>> Cc: Eric Dumazet <edumazet@...gle.com>
>>> Cc: Yang Guo <guoyang2@...wei.com>
>>> Signed-off-by: Yuqi Jin <jinyuqi@...wei.com>
>>> Signed-off-by: Shaokun Zhang <zhangshaokun@...ilicon.com>
>>
>> I doubt this makes any measurable improvement in performance.
>>
>> If you can document a specific measurable improvement under
>> a useful set of circumstances for real usage, then put those
>> details into the commit message and resubmit.
>>
>> Otherwise, I'm not applying this, sorry.
>>
> 
> 
> Real difference that could be made here is to 
> only use this cmpxchg() dance for CONFIG_UBSAN
> 
> When CONFIG_UBSAN is not set, atomic_add_return() is just fine.
> 
> (Supposedly UBSAN should not warn about that either, but this depends on compiler version)

I will test something like :

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 2010888e68ca96ae880481973a6d808d6c5612c5..e2fa972f5c78f2aefc801db6a45b2a81141c3028 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -495,11 +495,15 @@ u32 ip_idents_reserve(u32 hash, int segs)
        if (old != now && cmpxchg(p_tstamp, old, now) == old)
                delta = prandom_u32_max(now - old);
 
-       /* Do not use atomic_add_return() as it makes UBSAN unhappy */
+#ifdef CONFIG_UBSAN
+       /* Do not use atomic_add_return() as it makes old UBSAN versions unhappy */
        do {
                old = (u32)atomic_read(p_id);
                new = old + delta + segs;
        } while (atomic_cmpxchg(p_id, old, new) != old);
+#else
+       new = atomic_add_return(segs + delta, p_id);
+#endif
 
        return new - segs;
 }