netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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 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-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-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

* 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

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).