Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1602533 > unrolled thread

[PATCH 00/17] net subsystem refcount conversions

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/17] net subsystem refcount conversions Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:30 +0100
    [PATCH 02/17] net: convert neigh_params.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:30 +0100
    [PATCH 10/17] net: convert in_device.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
    [PATCH 12/17] net: convert unix_address.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
    [PATCH 14/17] net: convert inet_frag_queue.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
    [PATCH 01/17] net: convert neighbour.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
    [PATCH 11/17] net: convert netpoll_info.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
    [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
      Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Eric Dumazet <eric.dumazet@gmail.com> - 2017-03-16 18:00 +0100
        Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t Kees Cook <keescook@chromium.org> - 2017-03-16 18:40 +0100
          Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t David Miller <davem@davemloft.net> - 2017-03-16 20:20 +0100
            RE: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-03-17 08:50 +0100
              Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Eric Dumazet <eric.dumazet@gmail.com> - 2017-03-17 17:20 +0100
                Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Herbert Xu <herbert@gondor.apana.org.au> - 2017-03-18 18:00 +0100
                  Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t David Miller <davem@davemloft.net> - 2017-03-19 02:40 +0100
                    Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-03-20 11:50 +0100
                      Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-03-20 14:30 +0100
                        Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-03-20 14:50 +0100
                          Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Eric Dumazet <eric.dumazet@gmail.com> - 2017-03-20 16:00 +0100
                            Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Eric Dumazet <eric.dumazet@gmail.com> - 2017-03-20 17:20 +0100
                              Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Eric Dumazet <eric.dumazet@gmail.com> - 2017-03-20 17:40 +0100
                          Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Eric Dumazet <eric.dumazet@gmail.com> - 2017-03-20 16:30 +0100
                            Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-03-20 16:50 +0100
                          Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t Kees Cook <keescook@chromium.org> - 2017-03-21 21:50 +0100
                            Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Eric Dumazet <eric.dumazet@gmail.com> - 2017-03-21 22:30 +0100
                              Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t David Miller <davem@davemloft.net> - 2017-03-21 23:40 +0100
                              Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t Kees Cook <keescook@chromium.org> - 2017-03-22 01:00 +0100
                                Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Eric Dumazet <eric.dumazet@gmail.com> - 2017-03-22 03:10 +0100
                                  Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-03-22 13:30 +0100
                                    Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Eric Dumazet <eric.dumazet@gmail.com> - 2017-03-22 14:30 +0100
                                      Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-03-22 15:40 +0100
                                        Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Eric Dumazet <eric.dumazet@gmail.com> - 2017-03-22 16:00 +0100
                                          Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-03-22 16:10 +0100
                                            Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Eric Dumazet <eric.dumazet@gmail.com> - 2017-03-22 16:30 +0100
                                          Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-03-22 18:00 +0100
                                  Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to refcount_t Kees Cook <keescook@chromium.org> - 2017-03-22 20:10 +0100
                                Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-03-22 14:50 +0100
                        Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Herbert Xu <herbert@gondor.apana.org.au> - 2017-03-20 15:50 +0100
                      Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Herbert Xu <herbert@gondor.apana.org.au> - 2017-03-20 14:50 +0100
                        RE: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t David Laight <David.Laight@ACULAB.COM> - 2017-03-20 15:20 +0100
                          Re: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-03-20 15:30 +0100
                            RE: [PATCH 07/17] net: convert sock.sk_refcnt from atomic_t to  refcount_t David Laight <David.Laight@ACULAB.COM> - 2017-03-20 16:10 +0100
    [PATCH 05/17] net: convert sk_buff_fclones.fclone_ref from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
    [PATCH 15/17] net: convert net.passive from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
    [PATCH 16/17] net: convert netlbl_lsm_cache.refcount from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
    [PATCH 09/17] net: convert ip_mc_list.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
    [PATCH 04/17] net: convert sk_buff.users from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
    [PATCH 08/17] net: convert sk_filter.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
      Re: [PATCH 08/17] net: convert sk_filter.refcnt from atomic_t to  refcount_t Daniel Borkmann <daniel@iogearbox.net> - 2017-03-16 17:10 +0100
        RE: [PATCH 08/17] net: convert sk_filter.refcnt from atomic_t to  refcount_t "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-03-17 09:20 +0100
    [PATCH 17/17] net: convert packet_fanout.sk_ref from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
    [PATCH 06/17] net: convert sock.sk_wmem_alloc from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100
    [PATCH 13/17] net: convert fib_rule.refcnt from atomic_t to refcount_t Elena Reshetova <elena.reshetova@intel.com> - 2017-03-16 16:40 +0100

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1604751 — 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 queues on TX or RX between my reboots).

I added a sysctl hack to be able to dynamically change on a given
workload, and we hit other bottlenecks (mainly qdisc locks and driver tx
locks) anyway.

[toc] | [prev] | [next] | [standalone]

#1604691 — 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 op takes
> > vs. how many cycles this new code takes.
> 
> On what uarch?
> 
> I think I tested hand coded asm version and it ended up about double the
> cycles for a cmpxchg loop vs the direct instruction on an IVB-EX (until
> the memory bus saturated, at which point they took the same). Newer
> parts will of course have different numbers,
> 
> Can't we run some iperf on a 40gbe fiber loop or something? It would be
> very useful to have an actual workload we can run.

If atomic ops are converted one by one, it is likely that results will
be noise.

We can not start a global conversion without having a way to have
selective debugging ?

Then, adopting this fine infra would really not be a problem.

Some arches have efficient atomic_inc() ( no full barriers ) while load
+ test + atomic_cmpxchg() + test + loop" is more expensive.

PowerPC has no efficient atomic_inc() and this definitely shows on
network intensive workloads involving concurrent cores/threads.

atomic_cmpxchg() on PowerPC is horribly more expensive because of the
added two SYNC instructions.

networking performance is quite poor on PowerPC as of today.

[toc] | [prev] | [next] | [standalone]

#1604705 — 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 expensive because of the
> added two SYNC instructions.

Note that refcount_t uses atomic_cmpxchg_release() and
atomic_cmpxchg_relaxed() which avoid most of the painful barriers.

[toc] | [prev] | [next] | [standalone]

#1605983 — 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 <peterz@infradead.org> 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 op takes
>> vs. how many cycles this new code takes.
>
> On what uarch?
>
> I think I tested hand coded asm version and it ended up about double the
> cycles for a cmpxchg loop vs the direct instruction on an IVB-EX (until
> the memory bus saturated, at which point they took the same). Newer
> parts will of course have different numbers,
>
> Can't we run some iperf on a 40gbe fiber loop or something? It would be
> very useful to have an actual workload we can run.

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 concurrent 'netperf -t UDP_STREAM'" example (from
elsewhere in this email thread) real-world meaningful enough?

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1606007 — 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 concurrent 'netperf -t UDP_STREAM'" example (from
> elsewhere in this email thread) real-world meaningful enough?

Not at all ;)

This was targeting the specific change I had in mind for
ip_idents_reserve(), which is not used by TCP flows.

Unfortunately there is no good test simulating real-world workloads,
which are mostly using TCP flows.

Most synthetic tools you can find are not using epoll(), and very often
hit bottlenecks in other layers.


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 kernel
qualification (roughly one month, thousands of hosts..., days of SWE).

[toc] | [prev] | [next] | [standalone]

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

From: Eric Dumazet <eric.dumazet@gmail.com>
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 kernel
> qualification (roughly one month, thousands of hosts..., days of SWE).

+1

[toc] | [prev] | [next] | [standalone]

#1606105 — 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 <eric.dumazet@gmail.com> 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 performance changes, which is what we need to
>> measure.
>>
>> Is Eric's "20 concurrent 'netperf -t UDP_STREAM'" example (from
>> elsewhere in this email thread) real-world meaningful enough?
>
> Not at all ;)
>
> This was targeting the specific change I had in mind for
> ip_idents_reserve(), which is not used by TCP flows.

Okay, I just wanted to check. I didn't think so, but it was the only
example in the thread.

> Unfortunately there is no good test simulating real-world workloads,
> which are mostly using TCP flows.

Sure, but there has to be _something_ that can be used to test to
measure the effects. Without a meaningful test, it's weird to reject a
change for performance reasons.

> Most synthetic tools you can find are not using epoll(), and very often
> hit bottlenecks in other layers.
>
>
> It looks like our suggestion to get kernel builds with atomic_inc()
> being exactly an atomic_inc() is not even discussed or implemented.

So, FWIW, I originally tried to make this a CONFIG in the first couple
passes at getting a refcount defense. I would be fine with this, but I
was not able to convince Peter. :) However, things have evolved a lot
since then, so perhaps there are things do be done here.

> Coding this would require less time than running a typical Google kernel
> qualification (roughly one month, thousands of hosts..., days of SWE).

It wasn't the issue of coding time; just that it had been specifically
not wanted. :)

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 UNPROTECTED_REFCOUNT
#include <refcount.h>

or similar?

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1606148 — 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 UNPROTECTED_REFCOUNT
> #include <refcount.h>
> 
> or similar?

At first, it could be something simple like that yes.

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.

Then later, maybe provide a dynamic infrastructure so that we can
dynamically force the full checks even for refcount_inc_relaxed() on say
1% of the hosts, to get better debug coverage ?

[toc] | [prev] | [next] | [standalone]

#1606414 — 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 doesn't
care about refcount performance much, can still have all the tests
enabled.

But the code that cares about it (and preferably can prove it with
numbers) can use the other.

I'm also somewhat hesitant to use _relaxed for this distinction, as it
has a clear meaning in atomics, maybe _nocheck?

Also; what operations do you want _nocheck variants of, only
refcount_inc() ?

That said; I'm really loath to provide these without actual measurements
that prove they make a difference.

> Then later, maybe provide a dynamic infrastructure so that we can
> dynamically force the full checks even for refcount_inc_relaxed() on say
> 1% of the hosts, to get better debug coverage ?

Shouldn't be too hard to do in arch specific code using alternative
stuff. Generic code could use jump labels I suppose, but that would
result in bigger code.

[toc] | [prev] | [next] | [standalone]

#1606478 — 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.
> 
> So you'd want a duplicate interface, such that most code, which doesn't
> care about refcount performance much, can still have all the tests
> enabled.
> 
> But the code that cares about it (and preferably can prove it with
> numbers) can use the other.
> 
> I'm also somewhat hesitant to use _relaxed for this distinction, as it
> has a clear meaning in atomics, maybe _nocheck?
> 
> Also; what operations do you want _nocheck variants of, only
> refcount_inc() ?

I was mostly thinking of points where we were already checking the value
either before or after the atomic_inc(), using some lazy check (a la
WARN_ON(atomic_read(p) == 0) or something like that.

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.


 

[toc] | [prev] | [next] | [standalone]

#1606593 — 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 CONFIG
option. Not to mention that this CONFIG knob will kill the warnings for
everything, even the code that might not be as heavily audited as
network and which doesn't really care much about the performance of
refcount operations.


So I'm actually in favour of _nocheck variants, if we can show the need
for them. And I like your idea of being able to dynamically switch them
back to full debug as well.


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.

[toc] | [prev] | [next] | [standalone]

#1606616 — 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 measure the impact on real workloads, our productions
kernels are based on 4.3 at this moment.

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.

[toc] | [prev] | [next] | [standalone]

#1606630 — 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 users of _nocheck over the head with -- show numbers.
> 
> I wont be able to measure the impact on real workloads, our productions
> kernels are based on 4.3 at this moment.

Is there really no micro bench that exercises the relevant network
paths? Do you really fully rely on Google production workloads?

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

Its also a fairly meaningless number. It doesn't include any of the
other work the network path does.

[toc] | [prev] | [next] | [standalone]

#1606664 — 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. 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 measure the impact on real workloads, our productions
> > kernels are based on 4.3 at this moment.
> 
> Is there really no micro bench that exercises the relevant network
> paths? Do you really fully rely on Google production workloads?

You could run a synflood test, with ~10 Mpps.

sock_hold() is definitely used in SYN handling.

Last upstream kernels do not work on my lab hosts, for whatever reason.

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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 on uncontended ops:

					SKL	SNB	IVB-EP

atomic:		lock incl		~15	~13	~10
atomic-ref:	call refcount_inc	~31	~37	~31
atomic-ref2:	$inlined		~23	~22	~21


Contended numbers (E3-1245 v5):


root@skl:~/spinlocks# LOCK=./atomic ./test1.sh
1: 14.797240
2: 87.451230
4: 100.747790
8: 118.234010

root@skl:~/spinlocks# LOCK=./atomic-ref ./test1.sh
1: 30.627320
2: 91.866730
4: 111.029560
8: 141.922420

root@skl:~/spinlocks# LOCK=./atomic-ref2 ./test1.sh
1: 23.243930
2: 98.620250
4: 119.604240
8: 124.864380



The code includes the patches found here:

  https://lkml.kernel.org/r/20170317211918.393791494@infradead.org

and effectively does:

#define REFCOUNT_WARN(cond, str) WARN_ON_ONCE(cond)

 s/WARN_ONCE/REFCOUNT_WARN/

on lib/refcount.c

Find the tarball of the userspace code used attached (its a bit of a
mess; its grown over time and needs a cleanup).

I used: gcc (Debian 6.3.0-6) 6.3.0 20170205


So while its about ~20 cycles worse, reducing contention is far more
effective than removing straight line instruction count (which too is
entirely possible, because GCC generates absolute shite in places).

[toc] | [prev] | [next] | [standalone]

#1606919 — 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 <eric.dumazet@gmail.com> 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)
>> ...
>> #else
>> void refcount_inc(...
>> ...
>> #endif
>>
>> some/net.c:
>> #define UNPROTECTED_REFCOUNT
>> #include <refcount.h>
>>
>> or similar?
>
> At first, it could be something simple like that yes.
>
> 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.
>
> Then later, maybe provide a dynamic infrastructure so that we can
> dynamically force the full checks even for refcount_inc_relaxed() on say
> 1% of the hosts, to get better debug coverage ?

Well, this isn't about finding bugs in normal workflows. This is about
catching bugs that attackers have found and start exploiting to gain a
use-after-free primitive. The intention is for it to be always
enabled.

-Kees

-- 
Kees Cook
Pixel Security

[toc] | [prev] | [next] | [standalone]

#1606515 — 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 <eric.dumazet@gmail.com> 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 can be used to test to
> measure the effects. Without a meaningful test, it's weird to reject a
> change for performance reasons.

This. How can you optimize if there's no way to actually measure
something?

> > Most synthetic tools you can find are not using epoll(), and very often
> > hit bottlenecks in other layers.
> >
> >
> > It looks like our suggestion to get kernel builds with atomic_inc()
> > being exactly an atomic_inc() is not even discussed or implemented.
> 
> So, FWIW, I originally tried to make this a CONFIG in the first couple
> passes at getting a refcount defense. I would be fine with this, but I
> was not able to convince Peter. :) However, things have evolved a lot
> since then, so perhaps there are things do be done here.

Well, the argument was that unless there's a benchmark that shows it
cares, its all premature optimization.

Similarly, you wanted this enabled at all times because hardening.

[toc] | [prev] | [next] | [standalone]

#1604652 — 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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[toc] | [prev] | [next] | [standalone]

#1604586 — 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 should
demonstrate that it does not cause a performance regression.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[toc] | [prev] | [next] | [standalone]

#1604615 — 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
> higher? Anyway, since you're proposing this change you should
> demonstrate that it does not cause a performance regression.

What checks does refcnt_t actually do?

An extra decrement is hard to detect since the item gets freed early.
I guess making the main 'allocate/free' code hold (say) 64k references
would give some leeway for extra decrements.

An extra increment will be detected when the count eventually wraps.
Unless the error is in a very common path that won't happen for a long time.

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.

Otherwise having a common, but not every packet, code path verify that the
reference count is 'sane' would give reasonable coverage.

	David

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web