Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Tue, Mar 21, 2017 at 7:03 PM, Eric Dumazet wrote: > On Tue, 2017-03-21 at 16:51 -0700, Kees Cook wrote: > >> Am I understanding you correctly that you'd want something like: >> >> refcount.h: >> #ifdef UNPROTECTED_REFCOUNT >> #define refcount_inc(x) atomic_inc(x) >>

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Wed, Mar 22, 2017 at 07:54:04AM -0700, Eric Dumazet wrote: > > I guess someone could code a lib/test_refcount.c launching X threads > using either atomic_inc or refcount_inc() in a loop. > > That would give a rough estimate of the refcount_t overhead among > various platforms. Cycles spend

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Wed, 2017-03-22 at 16:08 +0100, Peter Zijlstra wrote: > On Wed, Mar 22, 2017 at 07:54:04AM -0700, Eric Dumazet wrote: > > On Wed, 2017-03-22 at 15:33 +0100, Peter Zijlstra wrote: > > > > > > > > But I would feel a whole lot better about the entire thing if we could > > > measure their impact.

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Wed, Mar 22, 2017 at 07:54:04AM -0700, Eric Dumazet wrote: > On Wed, 2017-03-22 at 15:33 +0100, Peter Zijlstra wrote: > > > > > But I would feel a whole lot better about the entire thing if we could > > measure their impact. It would also give us good precedent to whack > > other potential

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Wed, 2017-03-22 at 15:33 +0100, Peter Zijlstra wrote: > > But I would feel a whole lot better about the entire thing if we could > measure their impact. It would also give us good precedent to whack > other potential users of _nocheck over the head with -- show numbers. I wont be able to

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Wed, Mar 22, 2017 at 06:22:16AM -0700, Eric Dumazet wrote: > But admittedly we can replace all these by standard refcount_inc() and > simply provide a CONFIG option to turn off the checks, and let brave > people enable this option. Still brings us back to lacking a real reason to provide that

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Tue, Mar 21, 2017 at 04:51:13PM -0700, Kees Cook wrote: > On Tue, Mar 21, 2017 at 2:23 PM, Eric Dumazet wrote: > > Unfortunately there is no good test simulating real-world workloads, > > which are mostly using TCP flows. > > Sure, but there has to be _something_ that

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Wed, 2017-03-22 at 13:25 +0100, Peter Zijlstra wrote: > On Tue, Mar 21, 2017 at 07:03:19PM -0700, Eric Dumazet wrote: > > > Note that we might define two refcount_inc() : One that does whole > > tests, and refcount_inc_relaxed() that might translate to atomic_inc() > > on non debug kernels. >

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Tue, Mar 21, 2017 at 07:03:19PM -0700, Eric Dumazet wrote: > Note that we might define two refcount_inc() : One that does whole > tests, and refcount_inc_relaxed() that might translate to atomic_inc() > on non debug kernels. So you'd want a duplicate interface, such that most code, which

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Tue, 2017-03-21 at 16:51 -0700, Kees Cook wrote: > Am I understanding you correctly that you'd want something like: > > refcount.h: > #ifdef UNPROTECTED_REFCOUNT > #define refcount_inc(x) atomic_inc(x) > ... > #else > void refcount_inc(... > ... > #endif > > some/net.c: > #define

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Tue, Mar 21, 2017 at 2:23 PM, Eric Dumazet wrote: > On Tue, 2017-03-21 at 13:49 -0700, Kees Cook wrote: > >> Yeah, this is exactly what I'd like to find as well. Just comparing >> cycles between refcount implementations, while interesting, doesn't >> show us real-world

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

From: Eric Dumazet Date: Tue, 21 Mar 2017 14:23:09 -0700 > It looks like our suggestion to get kernel builds with atomic_inc() > being exactly an atomic_inc() is not even discussed or implemented. > > Coding this would require less time than running a typical Google

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Tue, 2017-03-21 at 13:49 -0700, Kees Cook wrote: > Yeah, this is exactly what I'd like to find as well. Just comparing > cycles between refcount implementations, while interesting, doesn't > show us real-world performance changes, which is what we need to > measure. > > Is Eric's "20

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Mon, Mar 20, 2017 at 6:40 AM, Peter Zijlstra wrote: > On Mon, Mar 20, 2017 at 09:27:13PM +0800, Herbert Xu wrote: >> On Mon, Mar 20, 2017 at 02:23:57PM +0100, Peter Zijlstra wrote: >> > >> > So what bench/setup do you want ran? >> >> You can start by counting how many

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Mon, 2017-03-20 at 09:18 -0700, Eric Dumazet wrote: > Interesting. > > UDP ipv4 xmit path gets a ~25 % improvement on PPC with this patch. > > ( 20 concurrent netperf -t UDP_STREAM : 2.45 Mpps -> 3.07 Mpps ) Well, there _is_ a difference, but not 25 % (this was probably caused by different

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Mon, 2017-03-20 at 07:59 -0700, Eric Dumazet wrote: > On Mon, 2017-03-20 at 07:51 -0700, Eric Dumazet wrote: > > > atomic_cmpxchg() on PowerPC is horribly more expensive because of the > > added two SYNC instructions. > > Although I just saw that refcount was using atomic_cmpxchg_relaxed() >

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Mon, Mar 20, 2017 at 07:51:01AM -0700, Eric Dumazet wrote: > PowerPC has no efficient atomic_inc() and this definitely shows on > network intensive workloads involving concurrent cores/threads. Correct, PPC LL/SC are dreadfully expensive. > atomic_cmpxchg() on PowerPC is horribly more

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Mon, 2017-03-20 at 14:40 +0100, Peter Zijlstra wrote: > On Mon, Mar 20, 2017 at 09:27:13PM +0800, Herbert Xu wrote: > > On Mon, Mar 20, 2017 at 02:23:57PM +0100, Peter Zijlstra wrote: > > > > > > So what bench/setup do you want ran? > > > > You can start by counting how many cycles an atomic

RE: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

From: Peter Zijlstra > Sent: 20 March 2017 14:28 > On Mon, Mar 20, 2017 at 02:10:24PM +, David Laight wrote: > > On x86 the cpu flags from the 'lock inc/dec' could be used to reasonably > > cheaply detect errors - provided you actually generate a forwards branch. > > Note that currently there

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Mon, 2017-03-20 at 07:51 -0700, Eric Dumazet wrote: > atomic_cmpxchg() on PowerPC is horribly more expensive because of the > added two SYNC instructions. Although I just saw that refcount was using atomic_cmpxchg_relaxed() Time to find some documentation (probably missing) or get some specs

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Mon, Mar 20, 2017 at 02:23:57PM +0100, Peter Zijlstra wrote: > > So what bench/setup do you want ran? You can start by counting how many cycles an atomic op takes vs. how many cycles this new code takes. Cheers, -- Email: Herbert Xu Home Page:

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Mon, Mar 20, 2017 at 02:10:24PM +, David Laight wrote: > On x86 the cpu flags from the 'lock inc/dec' could be used to reasonably > cheaply detect errors - provided you actually generate a forwards branch. Note that currently there is no arch specific implementation. We could of course

RE: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

From: Herbert Xu > Sent: 20 March 2017 13:16 > On Mon, Mar 20, 2017 at 11:39:37AM +0100, Peter Zijlstra wrote: > > > > Can we at least give a benchmark and have someone run numbers? We should > > be able to quantify these things. > > Do you realise how many times this thing gets hit at 10Gb/s or

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Mon, Mar 20, 2017 at 09:27:13PM +0800, Herbert Xu wrote: > On Mon, Mar 20, 2017 at 02:23:57PM +0100, Peter Zijlstra wrote: > > > > So what bench/setup do you want ran? > > You can start by counting how many cycles an atomic op takes > vs. how many cycles this new code takes. On what uarch? I

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Mon, Mar 20, 2017 at 11:39:37AM +0100, Peter Zijlstra wrote: > > Can we at least give a benchmark and have someone run numbers? We should > be able to quantify these things. Do you realise how many times this thing gets hit at 10Gb/s or higher? Anyway, since you're proposing this change you

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Mon, Mar 20, 2017 at 09:16:29PM +0800, Herbert Xu wrote: > On Mon, Mar 20, 2017 at 11:39:37AM +0100, Peter Zijlstra wrote: > > > > Can we at least give a benchmark and have someone run numbers? We should > > be able to quantify these things. > > Do you realise how many times this thing gets

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Sat, Mar 18, 2017 at 06:21:21PM -0700, David Miller wrote: > From: Herbert Xu > Date: Sun, 19 Mar 2017 00:47:59 +0800 > > > Eric Dumazet wrote: > >> On Fri, 2017-03-17 at 07:42 +, Reshetova, Elena wrote: > >> > >>> Should we then

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

From: Herbert Xu Date: Sun, 19 Mar 2017 00:47:59 +0800 > Eric Dumazet wrote: >> On Fri, 2017-03-17 at 07:42 +, Reshetova, Elena wrote: >> >>> Should we then first measure the actual numbers to understand what we >>> are talking here

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

Eric Dumazet wrote: > On Fri, 2017-03-17 at 07:42 +, Reshetova, Elena wrote: > >> Should we then first measure the actual numbers to understand what we >> are talking here about? >> I would be glad to do it if you suggest what is the correct way to do >> measurements

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Fri, 2017-03-17 at 07:42 +, Reshetova, Elena wrote: > Should we then first measure the actual numbers to understand what we > are talking here about? > I would be glad to do it if you suggest what is the correct way to do > measurements here to actually reflect the real life use cases.

RE: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

> From: Kees Cook > Date: Thu, 16 Mar 2017 11:38:25 -0600 > > > I am, of course, biased, but I think the evidence of actual > > refcounting attacks outweighs the theoretical performance cost of > > these changes. > > This is not theoretical at all. > > We count the

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

From: Kees Cook Date: Thu, 16 Mar 2017 11:38:25 -0600 > I am, of course, biased, but I think the evidence of actual > refcounting attacks outweighs the theoretical performance cost of > these changes. This is not theoretical at all. We count the nanoseconds that every

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Thu, Mar 16, 2017 at 10:58 AM, Eric Dumazet wrote: > On Thu, 2017-03-16 at 17:28 +0200, Elena Reshetova wrote: >> refcount_t type and corresponding API should be >> used instead of atomic_t when the variable is used as >> a reference counter. This allows to avoid

Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t

On Thu, 2017-03-16 at 17:28 +0200, Elena Reshetova wrote: > refcount_t type and corresponding API should be > used instead of atomic_t when the variable is used as > a reference counter. This allows to avoid accidental > refcounter overflows that might lead to use-after-free > situations. ... >