* [PATCH] net: optimize cmpxchg in ip_idents_reserve @ 2020-01-15 3:23 Shaokun Zhang 2020-01-16 12:27 ` David Miller 0 siblings, 1 reply; 19+ messages in thread From: Shaokun Zhang @ 2020-01-15 3:23 UTC (permalink / raw) To: netdev, linux-kernel Cc: Yuqi Jin, David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, Eric Dumazet, Yang Guo, Shaokun Zhang From: Yuqi Jin <jinyuqi@huawei.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@davemloft.net> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> Cc: Eric Dumazet <edumazet@google.com> Cc: Yang Guo <guoyang2@huawei.com> Signed-off-by: Yuqi Jin <jinyuqi@huawei.com> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com> --- net/ipv4/route.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/ipv4/route.c b/net/ipv4/route.c index 87e979f2b74a..7e28c7121c20 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -496,10 +496,10 @@ u32 ip_idents_reserve(u32 hash, int segs) delta = prandom_u32_max(now - old); /* Do not use atomic_add_return() as it makes UBSAN unhappy */ + old = (u32)atomic_read(p_id); do { - old = (u32)atomic_read(p_id); new = old + delta + segs; - } while (atomic_cmpxchg(p_id, old, new) != old); + } while (!atomic_try_cmpxchg(p_id, &old, new)); return new - segs; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-15 3:23 [PATCH] net: optimize cmpxchg in ip_idents_reserve Shaokun Zhang @ 2020-01-16 12:27 ` David Miller 2020-01-16 14:05 ` Shaokun Zhang 2020-01-16 15:12 ` Eric Dumazet 0 siblings, 2 replies; 19+ messages in thread From: David Miller @ 2020-01-16 12:27 UTC (permalink / raw) To: zhangshaokun Cc: netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2 From: Shaokun Zhang <zhangshaokun@hisilicon.com> Date: Wed, 15 Jan 2020 11:23:40 +0800 > From: Yuqi Jin <jinyuqi@huawei.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@davemloft.net> > Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> > Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Yang Guo <guoyang2@huawei.com> > Signed-off-by: Yuqi Jin <jinyuqi@huawei.com> > Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-16 12:27 ` David Miller @ 2020-01-16 14:05 ` Shaokun Zhang 2020-01-16 15:12 ` Eric Dumazet 1 sibling, 0 replies; 19+ messages in thread From: Shaokun Zhang @ 2020-01-16 14:05 UTC (permalink / raw) To: David Miller Cc: netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2 Hi David, On 2020/1/16 20:27, David Miller wrote: > From: Shaokun Zhang <zhangshaokun@hisilicon.com> > Date: Wed, 15 Jan 2020 11:23:40 +0800 > >> From: Yuqi Jin <jinyuqi@huawei.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@davemloft.net> >> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> >> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> >> Cc: Eric Dumazet <edumazet@google.com> >> Cc: Yang Guo <guoyang2@huawei.com> >> Signed-off-by: Yuqi Jin <jinyuqi@huawei.com> >> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.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. Ok, I will do it and resubmit. Thanks your reply, Shaokun > > Otherwise, I'm not applying this, sorry. > > . > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-16 12:27 ` David Miller 2020-01-16 14:05 ` Shaokun Zhang @ 2020-01-16 15:12 ` Eric Dumazet 2020-01-16 15:19 ` Eric Dumazet 1 sibling, 1 reply; 19+ messages in thread From: Eric Dumazet @ 2020-01-16 15:12 UTC (permalink / raw) To: David Miller, zhangshaokun Cc: netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2 On 1/16/20 4:27 AM, David Miller wrote: > From: Shaokun Zhang <zhangshaokun@hisilicon.com> > Date: Wed, 15 Jan 2020 11:23:40 +0800 > >> From: Yuqi Jin <jinyuqi@huawei.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@davemloft.net> >> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> >> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> >> Cc: Eric Dumazet <edumazet@google.com> >> Cc: Yang Guo <guoyang2@huawei.com> >> Signed-off-by: Yuqi Jin <jinyuqi@huawei.com> >> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.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) ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-16 15:12 ` Eric Dumazet @ 2020-01-16 15:19 ` Eric Dumazet 2020-01-17 6:54 ` Shaokun Zhang 0 siblings, 1 reply; 19+ messages in thread From: Eric Dumazet @ 2020-01-16 15:19 UTC (permalink / raw) To: David Miller, zhangshaokun Cc: netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2 On 1/16/20 7:12 AM, Eric Dumazet wrote: > > > On 1/16/20 4:27 AM, David Miller wrote: >> From: Shaokun Zhang <zhangshaokun@hisilicon.com> >> Date: Wed, 15 Jan 2020 11:23:40 +0800 >> >>> From: Yuqi Jin <jinyuqi@huawei.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@davemloft.net> >>> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> >>> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> >>> Cc: Eric Dumazet <edumazet@google.com> >>> Cc: Yang Guo <guoyang2@huawei.com> >>> Signed-off-by: Yuqi Jin <jinyuqi@huawei.com> >>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.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; } ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-16 15:19 ` Eric Dumazet @ 2020-01-17 6:54 ` Shaokun Zhang 2020-01-17 12:32 ` Peter Zijlstra 0 siblings, 1 reply; 19+ messages in thread From: Shaokun Zhang @ 2020-01-17 6:54 UTC (permalink / raw) To: Eric Dumazet, David Miller Cc: netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2, Will Deacon +Cc Will Deacon, On 2020/1/16 23:19, Eric Dumazet wrote: > > > On 1/16/20 7:12 AM, Eric Dumazet wrote: >> >> >> On 1/16/20 4:27 AM, David Miller wrote: >>> From: Shaokun Zhang <zhangshaokun@hisilicon.com> >>> Date: Wed, 15 Jan 2020 11:23:40 +0800 >>> >>>> From: Yuqi Jin <jinyuqi@huawei.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@davemloft.net> >>>> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> >>>> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org> >>>> Cc: Eric Dumazet <edumazet@google.com> >>>> Cc: Yang Guo <guoyang2@huawei.com> >>>> Signed-off-by: Yuqi Jin <jinyuqi@huawei.com> >>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.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 I appreciate Eric's idea. > + /* 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); But about this, I still think that we can try to use atomic_try_cmpxchg instead of atomic_cmpxchg, like refcount_add_not_zero in include/linux/refcount.h I abstract the model, as follow: while (get_cycles() <= (time_temp + t_cnt)) { do{ old_temp = wk_in->num.counter; oldt = atomic64_cmpxchg(&wk_in->num, old_temp, old_temp+1); }while(oldt != old_temp); myndelay(delay_o); w_cnt+=1; } val = atomic64_read(&wk_in->num); while (get_cycles() <= (time_temp + t_cnt)) { do{ new_val = val + 1; }while(!atomic64_try_cmpxchg(&wk_in->num, &val, new_val)); myndelay(delay_o); w_cnt += 1; } If we take the access global variable out of loop, the performance become better on both x86 and arm64, as follow: Kunpeng920: 24 cores call it and the successful operations per second atomic64_read in loop atomic64_read out of the loop 6291034 8751962 Intel 6248: 20 cores call it and the successful operations per second atomic64_read in loop atomic64_read out of the loop 8934792 9978998 So how about this? ;-) delta = prandom_u32_max(now - old); +#ifdef CONFIG_UBSAN /* Do not use atomic_add_return() as it makes UBSAN unhappy */ + old = (u32)atomic_read(p_id); do { - old = (u32)atomic_read(p_id); new = old + delta + segs; - } while (atomic_cmpxchg(p_id, old, new) != old); + } while (!atomic_try_cmpxchg(p_id, &old, new)); +#else + new = atomic_add_return(segs + delta, p_id); +#endif Thanks, Shaokun > +#else > + new = atomic_add_return(segs + delta, p_id); > +#endif > > return new - segs; > } > > > . > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-17 6:54 ` Shaokun Zhang @ 2020-01-17 12:32 ` Peter Zijlstra 2020-01-17 16:35 ` Eric Dumazet 2020-01-19 3:46 ` Shaokun Zhang 0 siblings, 2 replies; 19+ messages in thread From: Peter Zijlstra @ 2020-01-17 12:32 UTC (permalink / raw) To: Shaokun Zhang Cc: Eric Dumazet, David Miller, netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2, Will Deacon On Fri, Jan 17, 2020 at 02:54:03PM +0800, Shaokun Zhang wrote: > So how about this? ;-) > > delta = prandom_u32_max(now - old); > > +#ifdef CONFIG_UBSAN > /* Do not use atomic_add_return() as it makes UBSAN unhappy */ > + old = (u32)atomic_read(p_id); > do { > - old = (u32)atomic_read(p_id); > new = old + delta + segs; > - } while (atomic_cmpxchg(p_id, old, new) != old); > + } while (!atomic_try_cmpxchg(p_id, &old, new)); > +#else > + new = atomic_add_return(segs + delta, p_id); > +#endif That's crazy, just accept that UBSAN is taking bonghits and ignore it. Use atomic_add_return() unconditionally. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-17 12:32 ` Peter Zijlstra @ 2020-01-17 16:35 ` Eric Dumazet 2020-01-17 18:03 ` Arvind Sankar 2020-01-19 3:46 ` Shaokun Zhang 1 sibling, 1 reply; 19+ messages in thread From: Eric Dumazet @ 2020-01-17 16:35 UTC (permalink / raw) To: Peter Zijlstra, Shaokun Zhang Cc: Eric Dumazet, David Miller, netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2, Will Deacon On 1/17/20 4:32 AM, Peter Zijlstra wrote: > > That's crazy, just accept that UBSAN is taking bonghits and ignore it. > Use atomic_add_return() unconditionally. > Yes, we might simply add a comment so that people do not bug us if their compiler is too old. /* If UBSAN reports an error there, please make sure your compiler * supports -fno-strict-overflow before reporting it. */ return atomic_add_return(segs + delta, p_id) - segs; ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-17 16:35 ` Eric Dumazet @ 2020-01-17 18:03 ` Arvind Sankar 2020-01-17 18:16 ` Eric Dumazet 0 siblings, 1 reply; 19+ messages in thread From: Arvind Sankar @ 2020-01-17 18:03 UTC (permalink / raw) To: Eric Dumazet Cc: Peter Zijlstra, Shaokun Zhang, David Miller, netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2, Will Deacon On Fri, Jan 17, 2020 at 08:35:07AM -0800, Eric Dumazet wrote: > > > On 1/17/20 4:32 AM, Peter Zijlstra wrote: > > > > > That's crazy, just accept that UBSAN is taking bonghits and ignore it. > > Use atomic_add_return() unconditionally. > > > > Yes, we might simply add a comment so that people do not bug us if > their compiler is too old. > > /* If UBSAN reports an error there, please make sure your compiler > * supports -fno-strict-overflow before reporting it. > */ > return atomic_add_return(segs + delta, p_id) - segs; > Do we need that comment any more? The flag was apparently introduced in gcc-4.2 and we only support 4.6+ anyway? ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-17 18:03 ` Arvind Sankar @ 2020-01-17 18:16 ` Eric Dumazet 2020-01-17 18:38 ` Arvind Sankar 0 siblings, 1 reply; 19+ messages in thread From: Eric Dumazet @ 2020-01-17 18:16 UTC (permalink / raw) To: Arvind Sankar, Eric Dumazet Cc: Peter Zijlstra, Shaokun Zhang, David Miller, netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2, Will Deacon On 1/17/20 10:03 AM, Arvind Sankar wrote: > On Fri, Jan 17, 2020 at 08:35:07AM -0800, Eric Dumazet wrote: >> >> >> On 1/17/20 4:32 AM, Peter Zijlstra wrote: >> >>> >>> That's crazy, just accept that UBSAN is taking bonghits and ignore it. >>> Use atomic_add_return() unconditionally. >>> >> >> Yes, we might simply add a comment so that people do not bug us if >> their compiler is too old. >> >> /* If UBSAN reports an error there, please make sure your compiler >> * supports -fno-strict-overflow before reporting it. >> */ >> return atomic_add_return(segs + delta, p_id) - segs; >> > > Do we need that comment any more? The flag was apparently introduced in > gcc-4.2 and we only support 4.6+ anyway? Wasńt it the case back in 2016 already for linux-4.8 ? What will prevent someone to send another report to netdev/lkml ? -fno-strict-overflow support is not a prereq for CONFIG_UBSAN. Fact that we kept in lib/ubsan.c and lib/test_ubsan.c code for test_ubsan_add_overflow() and test_ubsan_sub_overflow() is disturbing. commit adb03115f4590baa280ddc440a8eff08a6be0cb7 Author: Eric Dumazet <edumazet@google.com> Date: Tue Sep 20 18:06:17 2016 -0700 net: get rid of an signed integer overflow in ip_idents_reserve() Jiri Pirko reported an UBSAN warning happening in ip_idents_reserve() [] UBSAN: Undefined behaviour in ./arch/x86/include/asm/atomic.h:156:11 [] signed integer overflow: [] -2117905507 + -695755206 cannot be represented in type 'int' Since we do not have uatomic_add_return() yet, use atomic_cmpxchg() so that the arithmetics can be done using unsigned int. Fixes: 04ca6973f7c1 ("ip: make IP identifiers less predictable") Signed-off-by: Eric Dumazet <edumazet@google.com> Reported-by: Jiri Pirko <jiri@resnulli.us> Signed-off-by: David S. Miller <davem@davemloft.net> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-17 18:16 ` Eric Dumazet @ 2020-01-17 18:38 ` Arvind Sankar 2020-01-17 18:48 ` Eric Dumazet 0 siblings, 1 reply; 19+ messages in thread From: Arvind Sankar @ 2020-01-17 18:38 UTC (permalink / raw) To: Eric Dumazet Cc: Arvind Sankar, Peter Zijlstra, Shaokun Zhang, David Miller, netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2, Will Deacon On Fri, Jan 17, 2020 at 10:16:45AM -0800, Eric Dumazet wrote: > Wasńt it the case back in 2016 already for linux-4.8 ? > > What will prevent someone to send another report to netdev/lkml ? > > -fno-strict-overflow support is not a prereq for CONFIG_UBSAN. > > Fact that we kept in lib/ubsan.c and lib/test_ubsan.c code for > test_ubsan_add_overflow() and test_ubsan_sub_overflow() is disturbing. > No, it was bumped in 2018 in commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6"). That raised it from 3.2 -> 4.6. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-17 18:38 ` Arvind Sankar @ 2020-01-17 18:48 ` Eric Dumazet 2020-01-20 8:18 ` Peter Zijlstra 0 siblings, 1 reply; 19+ messages in thread From: Eric Dumazet @ 2020-01-17 18:48 UTC (permalink / raw) To: Arvind Sankar, Eric Dumazet Cc: Peter Zijlstra, Shaokun Zhang, David Miller, netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2, Will Deacon On 1/17/20 10:38 AM, Arvind Sankar wrote: > On Fri, Jan 17, 2020 at 10:16:45AM -0800, Eric Dumazet wrote: >> Wasńt it the case back in 2016 already for linux-4.8 ? >> >> What will prevent someone to send another report to netdev/lkml ? >> >> -fno-strict-overflow support is not a prereq for CONFIG_UBSAN. >> >> Fact that we kept in lib/ubsan.c and lib/test_ubsan.c code for >> test_ubsan_add_overflow() and test_ubsan_sub_overflow() is disturbing. >> > > No, it was bumped in 2018 in commit cafa0010cd51 ("Raise the minimum > required gcc version to 4.6"). That raised it from 3.2 -> 4.6. > This seems good to me, for gcc at least. Maybe it is time to enfore -fno-strict-overflow in KBUILD_CFLAGS instead of making it conditional. Thanks. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-17 18:48 ` Eric Dumazet @ 2020-01-20 8:18 ` Peter Zijlstra 2020-05-07 9:12 ` Shaokun Zhang 0 siblings, 1 reply; 19+ messages in thread From: Peter Zijlstra @ 2020-01-20 8:18 UTC (permalink / raw) To: Eric Dumazet Cc: Arvind Sankar, Shaokun Zhang, David Miller, netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2, Will Deacon On Fri, Jan 17, 2020 at 10:48:19AM -0800, Eric Dumazet wrote: > > > On 1/17/20 10:38 AM, Arvind Sankar wrote: > > On Fri, Jan 17, 2020 at 10:16:45AM -0800, Eric Dumazet wrote: > >> Wasńt it the case back in 2016 already for linux-4.8 ? > >> > >> What will prevent someone to send another report to netdev/lkml ? > >> > >> -fno-strict-overflow support is not a prereq for CONFIG_UBSAN. > >> > >> Fact that we kept in lib/ubsan.c and lib/test_ubsan.c code for > >> test_ubsan_add_overflow() and test_ubsan_sub_overflow() is disturbing. > >> > > > > No, it was bumped in 2018 in commit cafa0010cd51 ("Raise the minimum > > required gcc version to 4.6"). That raised it from 3.2 -> 4.6. > > > > This seems good to me, for gcc at least. > > Maybe it is time to enfore -fno-strict-overflow in KBUILD_CFLAGS > instead of making it conditional. IIRC there was a bug in UBSAN vs -fwrapv/-fno-strict-overflow that was only fixed in gcc-8 or 9 or so. So while the -fwrapv/-fno-strict-overflow flag has been correctly supported since like forever, UBSAN was buggy until quite recent when used in conjustion with that flag. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-20 8:18 ` Peter Zijlstra @ 2020-05-07 9:12 ` Shaokun Zhang 2020-05-07 13:49 ` Eric Dumazet 0 siblings, 1 reply; 19+ messages in thread From: Shaokun Zhang @ 2020-05-07 9:12 UTC (permalink / raw) To: Peter Zijlstra, Eric Dumazet Cc: Arvind Sankar, David Miller, netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2, Will Deacon Hi Peter/Eric, Shall we use atomic_add_return() unconditionally and add some comments? Or I missed something. Thanks, Shaokun On 2020/1/20 16:18, Peter Zijlstra wrote: > On Fri, Jan 17, 2020 at 10:48:19AM -0800, Eric Dumazet wrote: >> >> >> On 1/17/20 10:38 AM, Arvind Sankar wrote: >>> On Fri, Jan 17, 2020 at 10:16:45AM -0800, Eric Dumazet wrote: >>>> Wasńt it the case back in 2016 already for linux-4.8 ? >>>> >>>> What will prevent someone to send another report to netdev/lkml ? >>>> >>>> -fno-strict-overflow support is not a prereq for CONFIG_UBSAN. >>>> >>>> Fact that we kept in lib/ubsan.c and lib/test_ubsan.c code for >>>> test_ubsan_add_overflow() and test_ubsan_sub_overflow() is disturbing. >>>> >>> >>> No, it was bumped in 2018 in commit cafa0010cd51 ("Raise the minimum >>> required gcc version to 4.6"). That raised it from 3.2 -> 4.6. >>> >> >> This seems good to me, for gcc at least. >> >> Maybe it is time to enfore -fno-strict-overflow in KBUILD_CFLAGS >> instead of making it conditional. > > IIRC there was a bug in UBSAN vs -fwrapv/-fno-strict-overflow that was > only fixed in gcc-8 or 9 or so. > > So while the -fwrapv/-fno-strict-overflow flag has been correctly > supported since like forever, UBSAN was buggy until quite recent when > used in conjustion with that flag. > > . > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-05-07 9:12 ` Shaokun Zhang @ 2020-05-07 13:49 ` Eric Dumazet 0 siblings, 0 replies; 19+ messages in thread From: Eric Dumazet @ 2020-05-07 13:49 UTC (permalink / raw) To: Shaokun Zhang Cc: Peter Zijlstra, Eric Dumazet, Arvind Sankar, David Miller, netdev, LKML, jinyuqi, Alexey Kuznetsov, Hideaki YOSHIFUJI, guoyang2, Will Deacon On Thu, May 7, 2020 at 2:12 AM Shaokun Zhang <zhangshaokun@hisilicon.com> wrote: > > Hi Peter/Eric, > > Shall we use atomic_add_return() unconditionally and add some comments? Or I missed > something. > Yes. A big fat comment, because I do not want yet another bogus complaint from someone playing with a buggy UBSAN. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-17 12:32 ` Peter Zijlstra 2020-01-17 16:35 ` Eric Dumazet @ 2020-01-19 3:46 ` Shaokun Zhang 2020-01-19 4:12 ` Eric Dumazet 1 sibling, 1 reply; 19+ messages in thread From: Shaokun Zhang @ 2020-01-19 3:46 UTC (permalink / raw) To: Peter Zijlstra Cc: Eric Dumazet, David Miller, netdev, linux-kernel, jinyuqi, kuznet, yoshfuji, edumazet, guoyang2, Will Deacon Hi Peter, On 2020/1/17 20:32, Peter Zijlstra wrote: > On Fri, Jan 17, 2020 at 02:54:03PM +0800, Shaokun Zhang wrote: >> So how about this? ;-) >> >> delta = prandom_u32_max(now - old); >> >> +#ifdef CONFIG_UBSAN >> /* Do not use atomic_add_return() as it makes UBSAN unhappy */ >> + old = (u32)atomic_read(p_id); >> do { >> - old = (u32)atomic_read(p_id); >> new = old + delta + segs; >> - } while (atomic_cmpxchg(p_id, old, new) != old); >> + } while (!atomic_try_cmpxchg(p_id, &old, new)); >> +#else >> + new = atomic_add_return(segs + delta, p_id); >> +#endif > > That's crazy, just accept that UBSAN is taking bonghits and ignore it. > Use atomic_add_return() unconditionally. We have used the atomic_add_return[1], but it makes the UBSAN unhappy followed by the comment. It seems that Eric also agreed to do it if some comments are added. I will do it later. Thanks, Shaokun [1] https://lkml.org/lkml/2019/7/26/217 > > . > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-19 3:46 ` Shaokun Zhang @ 2020-01-19 4:12 ` Eric Dumazet 2020-01-21 2:40 ` Shaokun Zhang 2020-01-22 8:49 ` Peter Zijlstra 0 siblings, 2 replies; 19+ messages in thread From: Eric Dumazet @ 2020-01-19 4:12 UTC (permalink / raw) To: Shaokun Zhang Cc: Peter Zijlstra, Eric Dumazet, David Miller, netdev, LKML, jinyuqi, Alexey Kuznetsov, Hideaki YOSHIFUJI, guoyang2, Will Deacon On Sat, Jan 18, 2020 at 7:47 PM Shaokun Zhang <zhangshaokun@hisilicon.com> wrote: > > We have used the atomic_add_return[1], but it makes the UBSAN unhappy followed > by the comment. > It seems that Eric also agreed to do it if some comments are added. I will do > it later. > > Thanks, > Shaokun > > [1] https://lkml.org/lkml/2019/7/26/217 > In case you have missed it, we needed a proper analysis. My feedback was quite simple : <quote> Have you first checked that current UBSAN versions will not complain anymore ? </quote> You never did this work, never replied to my question, and months later you come back with a convoluted patch while we simply can proceed with a revert now we are sure that linux kernels are compiled with the proper option. As mentioned yesterday, no need for a comment. Instead the changelog should be explaining why the revert is now safe. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-19 4:12 ` Eric Dumazet @ 2020-01-21 2:40 ` Shaokun Zhang 2020-01-22 8:49 ` Peter Zijlstra 1 sibling, 0 replies; 19+ messages in thread From: Shaokun Zhang @ 2020-01-21 2:40 UTC (permalink / raw) To: Eric Dumazet Cc: Peter Zijlstra, Eric Dumazet, David Miller, netdev, LKML, jinyuqi, Alexey Kuznetsov, Hideaki YOSHIFUJI, guoyang2, Will Deacon Hi Eric, On 2020/1/19 12:12, Eric Dumazet wrote: > On Sat, Jan 18, 2020 at 7:47 PM Shaokun Zhang > <zhangshaokun@hisilicon.com> wrote: >> > >> We have used the atomic_add_return[1], but it makes the UBSAN unhappy followed >> by the comment. >> It seems that Eric also agreed to do it if some comments are added. I will do >> it later. >> >> Thanks, >> Shaokun >> >> [1] https://lkml.org/lkml/2019/7/26/217 >> > > In case you have missed it, we needed a proper analysis. > My feedback was quite simple : > > <quote> > Have you first checked that current UBSAN versions will not complain anymore ? > </quote> > > You never did this work, never replied to my question, and months Yeah, I'm not sure how to deal with the UBSAN issue and you said that some of you would work this. > later you come back > with a convoluted patch while we simply can proceed with a revert now After several months, we thought that we can do it like refcount_add_not_zero, so we submit this patch. > we are sure that linux kernels are compiled with the proper option. > > As mentioned yesterday, no need for a comment. > Instead the changelog should be explaining why the revert is now safe. > Ok, it is really needed to consider this. Thanks, Shaokun > . > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] net: optimize cmpxchg in ip_idents_reserve 2020-01-19 4:12 ` Eric Dumazet 2020-01-21 2:40 ` Shaokun Zhang @ 2020-01-22 8:49 ` Peter Zijlstra 1 sibling, 0 replies; 19+ messages in thread From: Peter Zijlstra @ 2020-01-22 8:49 UTC (permalink / raw) To: Eric Dumazet Cc: Shaokun Zhang, Eric Dumazet, David Miller, netdev, LKML, jinyuqi, Alexey Kuznetsov, Hideaki YOSHIFUJI, guoyang2, Will Deacon On Sat, Jan 18, 2020 at 08:12:48PM -0800, Eric Dumazet wrote: > Instead the changelog should be explaining why the revert is now safe. FWIW, it was always safe, UBSAN was just buggy. ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-05-07 13:50 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-15 3:23 [PATCH] net: optimize cmpxchg in ip_idents_reserve Shaokun Zhang 2020-01-16 12:27 ` David Miller 2020-01-16 14:05 ` Shaokun Zhang 2020-01-16 15:12 ` Eric Dumazet 2020-01-16 15:19 ` Eric Dumazet 2020-01-17 6:54 ` Shaokun Zhang 2020-01-17 12:32 ` Peter Zijlstra 2020-01-17 16:35 ` Eric Dumazet 2020-01-17 18:03 ` Arvind Sankar 2020-01-17 18:16 ` Eric Dumazet 2020-01-17 18:38 ` Arvind Sankar 2020-01-17 18:48 ` Eric Dumazet 2020-01-20 8:18 ` Peter Zijlstra 2020-05-07 9:12 ` Shaokun Zhang 2020-05-07 13:49 ` Eric Dumazet 2020-01-19 3:46 ` Shaokun Zhang 2020-01-19 4:12 ` Eric Dumazet 2020-01-21 2:40 ` Shaokun Zhang 2020-01-22 8:49 ` Peter Zijlstra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).