Linux Ethernet Bridging - Mar 2017 - [Bridge] [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.

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.