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

Groups > linux.kernel > #1579819 > unrolled thread

Re: [tip:locking/core] refcount_t: Introduce a special purpose refcount type

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [tip:locking/core] refcount_t: Introduce a special purpose  refcount type Peter Zijlstra <peterz@infradead.org> - 2017-02-13 15:40 +0100
    Re: [tip:locking/core] refcount_t: Introduce a special purpose  refcount type Kees Cook <keescook@chromium.org> - 2017-02-13 18:50 +0100
      Re: [tip:locking/core] refcount_t: Introduce a special purpose  refcount type Peter Zijlstra <peterz@infradead.org> - 2017-02-13 19:10 +0100
        Re: [tip:locking/core] refcount_t: Introduce a special purpose  refcount type Ingo Molnar <mingo@kernel.org> - 2017-02-13 20:40 +0100
        Re: [tip:locking/core] refcount_t: Introduce a special purpose  refcount type Linus Torvalds <torvalds@linux-foundation.org> - 2017-02-13 21:20 +0100
    RE: [tip:locking/core] refcount_t: Introduce a special purpose  refcount type "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-14 08:30 +0100
      Re: [tip:locking/core] refcount_t: Introduce a special purpose  refcount type Ingo Molnar <mingo@kernel.org> - 2017-02-15 10:10 +0100
        RE: [tip:locking/core] refcount_t: Introduce a special purpose  refcount type "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-15 12:20 +0100

#1579819 — Re: [tip:locking/core] refcount_t: Introduce a special purpose refcount type

On Fri, Feb 10, 2017 at 12:31:15AM -0800, tip-bot for Peter Zijlstra wrote:
> Commit-ID:  f405df5de3170c00e5c54f8b7cf4766044a032ba
> Gitweb:     http://git.kernel.org/tip/f405df5de3170c00e5c54f8b7cf4766044a032ba
> Author:     Peter Zijlstra <peterz@infradead.org>
> AuthorDate: Mon, 14 Nov 2016 18:06:19 +0100
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Fri, 10 Feb 2017 09:04:19 +0100
> 
> refcount_t: Introduce a special purpose refcount type
> 
> Provide refcount_t, an atomic_t like primitive built just for
> refcounting.
> 
> It provides saturation semantics such that overflow becomes impossible
> and thereby 'spurious' use-after-free is avoided.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>

---
Subject: refcount: Out-of-line everything
From: Peter Zijlstra <peterz@infradead.org>
Date: Fri Feb 10 16:27:52 CET 2017

Linus asked to please make this real C code.

And since size then isn't an issue what so ever anymore, remove the
debug knob and make all WARN()s unconditional.

Cc: gregkh@linuxfoundation.org
Cc: keescook@chromium.org
Cc: dwindsor@gmail.com
Cc: elena.reshetova@intel.com
Cc: ishkamiel@gmail.com
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/refcount.h |  279 ++---------------------------------------------
 lib/Kconfig.debug        |   13 --
 lib/Makefile             |    2 
 lib/refcount.c           |  267 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 281 insertions(+), 280 deletions(-)

--- a/include/linux/refcount.h
+++ b/include/linux/refcount.h
@@ -1,55 +1,10 @@
 #ifndef _LINUX_REFCOUNT_H
 #define _LINUX_REFCOUNT_H
 
-/*
- * Variant of atomic_t specialized for reference counts.
- *
- * The interface matches the atomic_t interface (to aid in porting) but only
- * provides the few functions one should use for reference counting.
- *
- * It differs in that the counter saturates at UINT_MAX and will not move once
- * there. This avoids wrapping the counter and causing 'spurious'
- * use-after-free issues.
- *
- * Memory ordering rules are slightly relaxed wrt regular atomic_t functions
- * and provide only what is strictly required for refcounts.
- *
- * The increments are fully relaxed; these will not provide ordering. The
- * rationale is that whatever is used to obtain the object we're increasing the
- * reference count on will provide the ordering. For locked data structures,
- * its the lock acquire, for RCU/lockless data structures its the dependent
- * load.
- *
- * Do note that inc_not_zero() provides a control dependency which will order
- * future stores against the inc, this ensures we'll never modify the object
- * if we did not in fact acquire a reference.
- *
- * The decrements will provide release order, such that all the prior loads and
- * stores will be issued before, it also provides a control dependency, which
- * will order us against the subsequent free().
- *
- * The control dependency is against the load of the cmpxchg (ll/sc) that
- * succeeded. This means the stores aren't fully ordered, but this is fine
- * because the 1->0 transition indicates no concurrency.
- *
- * Note that the allocator is responsible for ordering things between free()
- * and alloc().
- *
- */
-
 #include <linux/atomic.h>
-#include <linux/bug.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 
-#ifdef CONFIG_DEBUG_REFCOUNT
-#define REFCOUNT_WARN(cond, str) WARN_ON(cond)
-#define __refcount_check	__must_check
-#else
-#define REFCOUNT_WARN(cond, str) (void)(cond)
-#define __refcount_check
-#endif
-
 typedef struct refcount_struct {
 	atomic_t refs;
 } refcount_t;
@@ -66,229 +21,21 @@ static inline unsigned int refcount_read
 	return atomic_read(&r->refs);
 }
 
-static inline __refcount_check
-bool refcount_add_not_zero(unsigned int i, refcount_t *r)
-{
-	unsigned int old, new, val = atomic_read(&r->refs);
-
-	for (;;) {
-		if (!val)
-			return false;
-
-		if (unlikely(val == UINT_MAX))
-			return true;
-
-		new = val + i;
-		if (new < val)
-			new = UINT_MAX;
-		old = atomic_cmpxchg_relaxed(&r->refs, val, new);
-		if (old == val)
-			break;
-
-		val = old;
-	}
-
-	REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
-
-	return true;
-}
-
-static inline void refcount_add(unsigned int i, refcount_t *r)
-{
-	REFCOUNT_WARN(!refcount_add_not_zero(i, r), "refcount_t: addition on 0; use-after-free.\n");
-}
-
-/*
- * Similar to atomic_inc_not_zero(), will saturate at UINT_MAX and WARN.
- *
- * Provides no memory ordering, it is assumed the caller has guaranteed the
- * object memory to be stable (RCU, etc.). It does provide a control dependency
- * and thereby orders future stores. See the comment on top.
- */
-static inline __refcount_check
-bool refcount_inc_not_zero(refcount_t *r)
-{
-	unsigned int old, new, val = atomic_read(&r->refs);
+extern __must_check bool refcount_add_not_zero(unsigned int i, refcount_t *r);
+extern void refcount_add(unsigned int i, refcount_t *r);
 
-	for (;;) {
-		new = val + 1;
+extern __must_check bool refcount_inc_not_zero(refcount_t *r);
+extern void refcount_inc(refcount_t *r);
 
-		if (!val)
-			return false;
+extern __must_check bool refcount_sub_and_test(unsigned int i, refcount_t *r);
+extern void refcount_sub(unsigned int i, refcount_t *r);
 
-		if (unlikely(!new))
-			return true;
-
-		old = atomic_cmpxchg_relaxed(&r->refs, val, new);
-		if (old == val)
-			break;
-
-		val = old;
-	}
-
-	REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
-
-	return true;
-}
-
-/*
- * Similar to atomic_inc(), will saturate at UINT_MAX and WARN.
- *
- * Provides no memory ordering, it is assumed the caller already has a
- * reference on the object, will WARN when this is not so.
- */
-static inline void refcount_inc(refcount_t *r)
-{
-	REFCOUNT_WARN(!refcount_inc_not_zero(r), "refcount_t: increment on 0; use-after-free.\n");
-}
-
-/*
- * Similar to atomic_dec_and_test(), it will WARN on underflow and fail to
- * decrement when saturated at UINT_MAX.
- *
- * Provides release memory ordering, such that prior loads and stores are done
- * before, and provides a control dependency such that free() must come after.
- * See the comment on top.
- */
-static inline __refcount_check
-bool refcount_sub_and_test(unsigned int i, refcount_t *r)
-{
-	unsigned int old, new, val = atomic_read(&r->refs);
-
-	for (;;) {
-		if (unlikely(val == UINT_MAX))
-			return false;
-
-		new = val - i;
-		if (new > val) {
-			REFCOUNT_WARN(new > val, "refcount_t: underflow; use-after-free.\n");
-			return false;
-		}
-
-		old = atomic_cmpxchg_release(&r->refs, val, new);
-		if (old == val)
-			break;
-
-		val = old;
-	}
-
-	return !new;
-}
-
-static inline __refcount_check
-bool refcount_dec_and_test(refcount_t *r)
-{
-	return refcount_sub_and_test(1, r);
-}
-
-/*
- * Similar to atomic_dec(), it will WARN on underflow and fail to decrement
- * when saturated at UINT_MAX.
- *
- * Provides release memory ordering, such that prior loads and stores are done
- * before.
- */
-static inline
-void refcount_dec(refcount_t *r)
-{
-	REFCOUNT_WARN(refcount_dec_and_test(r), "refcount_t: decrement hit 0; leaking memory.\n");
-}
-
-/*
- * No atomic_t counterpart, it attempts a 1 -> 0 transition and returns the
- * success thereof.
- *
- * Like all decrement operations, it provides release memory order and provides
- * a control dependency.
- *
- * It can be used like a try-delete operator; this explicit case is provided
- * and not cmpxchg in generic, because that would allow implementing unsafe
- * operations.
- */
-static inline __refcount_check
-bool refcount_dec_if_one(refcount_t *r)
-{
-	return atomic_cmpxchg_release(&r->refs, 1, 0) == 1;
-}
-
-/*
- * No atomic_t counterpart, it decrements unless the value is 1, in which case
- * it will return false.
- *
- * Was often done like: atomic_add_unless(&var, -1, 1)
- */
-static inline __refcount_check
-bool refcount_dec_not_one(refcount_t *r)
-{
-	unsigned int old, new, val = atomic_read(&r->refs);
-
-	for (;;) {
-		if (unlikely(val == UINT_MAX))
-			return true;
-
-		if (val == 1)
-			return false;
-
-		new = val - 1;
-		if (new > val) {
-			REFCOUNT_WARN(new > val, "refcount_t: underflow; use-after-free.\n");
-			return true;
-		}
-
-		old = atomic_cmpxchg_release(&r->refs, val, new);
-		if (old == val)
-			break;
-
-		val = old;
-	}
-
-	return true;
-}
-
-/*
- * Similar to atomic_dec_and_mutex_lock(), it will WARN on underflow and fail
- * to decrement when saturated at UINT_MAX.
- *
- * Provides release memory ordering, such that prior loads and stores are done
- * before, and provides a control dependency such that free() must come after.
- * See the comment on top.
- */
-static inline __refcount_check
-bool refcount_dec_and_mutex_lock(refcount_t *r, struct mutex *lock)
-{
-	if (refcount_dec_not_one(r))
-		return false;
-
-	mutex_lock(lock);
-	if (!refcount_dec_and_test(r)) {
-		mutex_unlock(lock);
-		return false;
-	}
-
-	return true;
-}
-
-/*
- * Similar to atomic_dec_and_lock(), it will WARN on underflow and fail to
- * decrement when saturated at UINT_MAX.
- *
- * Provides release memory ordering, such that prior loads and stores are done
- * before, and provides a control dependency such that free() must come after.
- * See the comment on top.
- */
-static inline __refcount_check
-bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock)
-{
-	if (refcount_dec_not_one(r))
-		return false;
-
-	spin_lock(lock);
-	if (!refcount_dec_and_test(r)) {
-		spin_unlock(lock);
-		return false;
-	}
-
-	return true;
-}
+extern __must_check bool refcount_dec_and_test(refcount_t *r);
+extern void refcount_dec(refcount_t *r);
+
+extern __must_check bool refcount_dec_if_one(refcount_t *r);
+extern __must_check bool refcount_dec_not_one(refcount_t *r);
+extern __must_check bool refcount_dec_and_mutex_lock(refcount_t *r, struct mutex *lock);
+extern __must_check bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock);
 
 #endif /* _LINUX_REFCOUNT_H */
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -716,19 +716,6 @@ source "lib/Kconfig.kmemcheck"
 
 source "lib/Kconfig.kasan"
 
-config DEBUG_REFCOUNT
-	bool "Verbose refcount checks"
-	help
-	  Say Y here if you want reference counters (refcount_t and kref) to
-	  generate WARNs on dubious usage. Without this refcount_t will still
-	  be a saturating counter and avoid Use-After-Free by turning it into
-	  a resource leak Denial-Of-Service.
-
-	  Use of this option will increase kernel text size but will alert the
-	  admin of potential abuse.
-
-	  If in doubt, say "N".
-
 endmenu # "Memory Debugging"
 
 config ARCH_HAS_KCOV
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -36,7 +36,7 @@ obj-y += bcd.o div64.o sort.o parser.o h
 	 gcd.o lcm.o list_sort.o uuid.o flex_array.o iov_iter.o clz_ctz.o \
 	 bsearch.o find_bit.o llist.o memweight.o kfifo.o \
 	 percpu-refcount.o percpu_ida.o rhashtable.o reciprocal_div.o \
-	 once.o
+	 once.o refcount.o
 obj-y += string_helpers.o
 obj-$(CONFIG_TEST_STRING_HELPERS) += test-string_helpers.o
 obj-y += hexdump.o
--- /dev/null
+++ b/lib/refcount.c
@@ -0,0 +1,267 @@
+/*
+ * Variant of atomic_t specialized for reference counts.
+ *
+ * The interface matches the atomic_t interface (to aid in porting) but only
+ * provides the few functions one should use for reference counting.
+ *
+ * It differs in that the counter saturates at UINT_MAX and will not move once
+ * there. This avoids wrapping the counter and causing 'spurious'
+ * use-after-free issues.
+ *
+ * Memory ordering rules are slightly relaxed wrt regular atomic_t functions
+ * and provide only what is strictly required for refcounts.
+ *
+ * The increments are fully relaxed; these will not provide ordering. The
+ * rationale is that whatever is used to obtain the object we're increasing the
+ * reference count on will provide the ordering. For locked data structures,
+ * its the lock acquire, for RCU/lockless data structures its the dependent
+ * load.
+ *
+ * Do note that inc_not_zero() provides a control dependency which will order
+ * future stores against the inc, this ensures we'll never modify the object
+ * if we did not in fact acquire a reference.
+ *
+ * The decrements will provide release order, such that all the prior loads and
+ * stores will be issued before, it also provides a control dependency, which
+ * will order us against the subsequent free().
+ *
+ * The control dependency is against the load of the cmpxchg (ll/sc) that
+ * succeeded. This means the stores aren't fully ordered, but this is fine
+ * because the 1->0 transition indicates no concurrency.
+ *
+ * Note that the allocator is responsible for ordering things between free()
+ * and alloc().
+ *
+ */
+
+#include <linux/refcount.h>
+#include <linux/bug.h>
+
+bool refcount_add_not_zero(unsigned int i, refcount_t *r)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+
+	for (;;) {
+		if (!val)
+			return false;
+
+		if (unlikely(val == UINT_MAX))
+			return true;
+
+		new = val + i;
+		if (new < val)
+			new = UINT_MAX;
+		old = atomic_cmpxchg_relaxed(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(refcount_add_not_zero);
+
+void refcount_add(unsigned int i, refcount_t *r)
+{
+	WARN(!refcount_add_not_zero(i, r), "refcount_t: addition on 0; use-after-free.\n");
+}
+EXPORT_SYMBOL_GPL(refcount_add);
+
+/*
+ * Similar to atomic_inc_not_zero(), will saturate at UINT_MAX and WARN.
+ *
+ * Provides no memory ordering, it is assumed the caller has guaranteed the
+ * object memory to be stable (RCU, etc.). It does provide a control dependency
+ * and thereby orders future stores. See the comment on top.
+ */
+bool refcount_inc_not_zero(refcount_t *r)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+
+	for (;;) {
+		new = val + 1;
+
+		if (!val)
+			return false;
+
+		if (unlikely(!new))
+			return true;
+
+		old = atomic_cmpxchg_relaxed(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(refcount_inc_not_zero);
+
+/*
+ * Similar to atomic_inc(), will saturate at UINT_MAX and WARN.
+ *
+ * Provides no memory ordering, it is assumed the caller already has a
+ * reference on the object, will WARN when this is not so.
+ */
+void refcount_inc(refcount_t *r)
+{
+	WARN(!refcount_inc_not_zero(r), "refcount_t: increment on 0; use-after-free.\n");
+}
+EXPORT_SYMBOL_GPL(refcount_inc);
+
+bool refcount_sub_and_test(unsigned int i, refcount_t *r)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+
+	for (;;) {
+		if (unlikely(val == UINT_MAX))
+			return false;
+
+		new = val - i;
+		if (new > val) {
+			WARN(new > val, "refcount_t: underflow; use-after-free.\n");
+			return false;
+		}
+
+		old = atomic_cmpxchg_release(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	return !new;
+}
+EXPORT_SYMBOL_GPL(refcount_sub_and_test);
+
+/*
+ * Similar to atomic_dec_and_test(), it will WARN on underflow and fail to
+ * decrement when saturated at UINT_MAX.
+ *
+ * Provides release memory ordering, such that prior loads and stores are done
+ * before, and provides a control dependency such that free() must come after.
+ * See the comment on top.
+ */
+bool refcount_dec_and_test(refcount_t *r)
+{
+	return refcount_sub_and_test(1, r);
+}
+EXPORT_SYMBOL_GPL(refcount_dec_and_test);
+
+/*
+ * Similar to atomic_dec(), it will WARN on underflow and fail to decrement
+ * when saturated at UINT_MAX.
+ *
+ * Provides release memory ordering, such that prior loads and stores are done
+ * before.
+ */
+
+void refcount_dec(refcount_t *r)
+{
+	WARN(refcount_dec_and_test(r), "refcount_t: decrement hit 0; leaking memory.\n");
+}
+EXPORT_SYMBOL_GPL(refcount_dec);
+
+/*
+ * No atomic_t counterpart, it attempts a 1 -> 0 transition and returns the
+ * success thereof.
+ *
+ * Like all decrement operations, it provides release memory order and provides
+ * a control dependency.
+ *
+ * It can be used like a try-delete operator; this explicit case is provided
+ * and not cmpxchg in generic, because that would allow implementing unsafe
+ * operations.
+ */
+bool refcount_dec_if_one(refcount_t *r)
+{
+	return atomic_cmpxchg_release(&r->refs, 1, 0) == 1;
+}
+EXPORT_SYMBOL_GPL(refcount_dec_if_one);
+
+/*
+ * No atomic_t counterpart, it decrements unless the value is 1, in which case
+ * it will return false.
+ *
+ * Was often done like: atomic_add_unless(&var, -1, 1)
+ */
+bool refcount_dec_not_one(refcount_t *r)
+{
+	unsigned int old, new, val = atomic_read(&r->refs);
+
+	for (;;) {
+		if (unlikely(val == UINT_MAX))
+			return true;
+
+		if (val == 1)
+			return false;
+
+		new = val - 1;
+		if (new > val) {
+			WARN(new > val, "refcount_t: underflow; use-after-free.\n");
+			return true;
+		}
+
+		old = atomic_cmpxchg_release(&r->refs, val, new);
+		if (old == val)
+			break;
+
+		val = old;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(refcount_dec_not_one);
+
+/*
+ * Similar to atomic_dec_and_mutex_lock(), it will WARN on underflow and fail
+ * to decrement when saturated at UINT_MAX.
+ *
+ * Provides release memory ordering, such that prior loads and stores are done
+ * before, and provides a control dependency such that free() must come after.
+ * See the comment on top.
+ */
+bool refcount_dec_and_mutex_lock(refcount_t *r, struct mutex *lock)
+{
+	if (refcount_dec_not_one(r))
+		return false;
+
+	mutex_lock(lock);
+	if (!refcount_dec_and_test(r)) {
+		mutex_unlock(lock);
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(refcount_dec_and_mutex_lock);
+
+/*
+ * Similar to atomic_dec_and_lock(), it will WARN on underflow and fail to
+ * decrement when saturated at UINT_MAX.
+ *
+ * Provides release memory ordering, such that prior loads and stores are done
+ * before, and provides a control dependency such that free() must come after.
+ * See the comment on top.
+ */
+bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock)
+{
+	if (refcount_dec_not_one(r))
+		return false;
+
+	spin_lock(lock);
+	if (!refcount_dec_and_test(r)) {
+		spin_unlock(lock);
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(refcount_dec_and_lock);
+

[toc] | [next] | [standalone]

#1579967

On Mon, Feb 13, 2017 at 6:34 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Feb 10, 2017 at 12:31:15AM -0800, tip-bot for Peter Zijlstra wrote:
>> Commit-ID:  f405df5de3170c00e5c54f8b7cf4766044a032ba
>> Gitweb:     http://git.kernel.org/tip/f405df5de3170c00e5c54f8b7cf4766044a032ba
>> Author:     Peter Zijlstra <peterz@infradead.org>
>> AuthorDate: Mon, 14 Nov 2016 18:06:19 +0100
>> Committer:  Ingo Molnar <mingo@kernel.org>
>> CommitDate: Fri, 10 Feb 2017 09:04:19 +0100
>>
>> refcount_t: Introduce a special purpose refcount type
>>
>> Provide refcount_t, an atomic_t like primitive built just for
>> refcounting.
>>
>> It provides saturation semantics such that overflow becomes impossible
>> and thereby 'spurious' use-after-free is avoided.
>>
>> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
> ---
> Subject: refcount: Out-of-line everything
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Fri Feb 10 16:27:52 CET 2017
>
> Linus asked to please make this real C code.

No objection from me, but I'm curious to see the conversation. Where
did this discussion happen? (I'm curious to see the reasoning behind
the decisions about the various trade-offs.)

> And since size then isn't an issue what so ever anymore, remove the
> debug knob and make all WARN()s unconditional.

Are you still going to land the x86 WARN_ON improvements?

-Kees

-- 
Kees Cook
Pixel Security

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

#1579977

On Mon, Feb 13, 2017 at 09:48:42AM -0800, Kees Cook wrote:
> On Mon, Feb 13, 2017 at 6:34 AM, Peter Zijlstra <peterz@infradead.org> wrote:

> > Linus asked to please make this real C code.
> 
> No objection from me, but I'm curious to see the conversation. Where
> did this discussion happen? (I'm curious to see the reasoning behind
> the decisions about the various trade-offs.)

I think Linus' email ended up being private; not much discussion other
than him saying he would like to see this.

Given the current state of code generation I wasn't in a state to argue
much. We can always revisit later.

> > And since size then isn't an issue what so ever anymore, remove the
> > debug knob and make all WARN()s unconditional.
> 
> Are you still going to land the x86 WARN_ON improvements?

Yes, once I manage to eke some response out of the relevant arch
maintainers on the subject ;-)

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

#1580040

* Peter Zijlstra <peterz@infradead.org> wrote:

> > > And since size then isn't an issue what so ever anymore, remove the debug 
> > > knob and make all WARN()s unconditional.
> > 
> > Are you still going to land the x86 WARN_ON improvements?
> 
> Yes, once I manage to eke some response out of the relevant arch maintainers on 
> the subject ;-)

so I'm enthusiastically in support of it all! Does that count as a response? ;-)

Thanks,

	Ingo

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

#1580051

On Mon, Feb 13, 2017 at 10:00 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> I think Linus' email ended up being private; not much discussion other
> than him saying he would like to see this.

I ended up reacting to the tipbot email, so I just replied to whoever
were on the cc list there, I guess.

              Linus

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

#1580365

> Subject: refcount: Out-of-line everything
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Fri Feb 10 16:27:52 CET 2017
> 
> Linus asked to please make this real C code.

Perhaps a completely stupid question, but I am going to ask anyway since only this way I can learn.
What a real difference it makes? Or are we talking more about styling and etc.? Is it because of size concerns?
This way it is certainly now done differently than rest of atomic and kref... 

Best Regards,
Elena.

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

#1581143

* Reshetova, Elena <elena.reshetova@intel.com> wrote:

> > Subject: refcount: Out-of-line everything
> > From: Peter Zijlstra <peterz@infradead.org>
> > Date: Fri Feb 10 16:27:52 CET 2017
> > 
> > Linus asked to please make this real C code.
> 
> Perhaps a completely stupid question, but I am going to ask anyway since only 
> this way I can learn. What a real difference it makes? Or are we talking more 
> about styling and etc.? Is it because of size concerns? This way it is certainly 
> now done differently than rest of atomic and kref...

It's about generated code size mostly.

This inline function is way too large to be inlined:

static inline __refcount_check
bool refcount_add_not_zero(unsigned int i, refcount_t *r)
{
	unsigned int old, new, val = atomic_read(&r->refs);

	for (;;) {
		if (!val)
			return false;

		if (unlikely(val == UINT_MAX))
			return true;

		new = val + i;
		if (new < val)
			new = UINT_MAX;
		old = atomic_cmpxchg_relaxed(&r->refs, val, new);
		if (old == val)
			break;

		val = old;
	}

	REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");

	return true;
}

When used then this function generates this much code on x86-64 defconfig:

00000000000084d0 <test>:
    84d0:	8b 0f                	mov    (%rdi),%ecx
    84d2:	55                   	push   %rbp
    84d3:	48 89 e5             	mov    %rsp,%rbp

    84d6:	85 c9                	test   %ecx,%ecx                |
    84d8:	74 30                	je     850a <test+0x3a>         |
    84da:	83 f9 ff             	cmp    $0xffffffff,%ecx         |
    84dd:	be ff ff ff ff       	mov    $0xffffffff,%esi         |
    84e2:	75 04                	jne    84e8 <test+0x18>         |
    84e4:	eb 1d                	jmp    8503 <test+0x33>         |
    84e6:	89 c1                	mov    %eax,%ecx                |
    84e8:	8d 51 01             	lea    0x1(%rcx),%edx           |
    84eb:	89 c8                	mov    %ecx,%eax                |
    84ed:	39 ca                	cmp    %ecx,%edx                |
    84ef:	0f 42 d6             	cmovb  %esi,%edx                |
    84f2:	f0 0f b1 17          	lock cmpxchg %edx,(%rdi)        |
    84f6:	39 c8                	cmp    %ecx,%eax                |
    84f8:	74 09                	je     8503 <test+0x33>         |
    84fa:	85 c0                	test   %eax,%eax                |
    84fc:	74 0c                	je     850a <test+0x3a>         |
    84fe:	83 f8 ff             	cmp    $0xffffffff,%eax         |
    8501:	75 e3                	jne    84e6 <test+0x16>         |
    8503:	b8 01 00 00 00       	mov    $0x1,%eax                |

    8508:	5d                   	pop    %rbp
    8509:	c3                   	retq   
    850a:	31 c0                	xor    %eax,%eax
    850c:	5d                   	pop    %rbp
    850d:	c3                   	retq   


(I've annotated the fastpath impact with '|'. Out of line code generally does not 
count.)

It's about ~50 bytes of code per usage site. It might be better in some cases, but 
not by much.

This is way above any sane inlining threshold. The 'unconditionally good' inlining 
threshold is at 1-2 instructions and below ~10 bytes of code.

So for example refcount_set() and refcount_read() can stay inlined:

static inline void refcount_set(refcount_t *r, unsigned int n)
{
	atomic_set(&r->refs, n);
}

static inline unsigned int refcount_read(const refcount_t *r)
{
	return atomic_read(&r->refs);
}


... beacuse they compile into a single instruction with 2-5 bytes I$ overhead.

Thanks,

	Ingo

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

#1581224

> * Reshetova, Elena <elena.reshetova@intel.com> wrote:
> 
> > > Subject: refcount: Out-of-line everything
> > > From: Peter Zijlstra <peterz@infradead.org>
> > > Date: Fri Feb 10 16:27:52 CET 2017
> > >
> > > Linus asked to please make this real C code.
> >
> > Perhaps a completely stupid question, but I am going to ask anyway since only
> > this way I can learn. What a real difference it makes? Or are we talking more
> > about styling and etc.? Is it because of size concerns? This way it is certainly
> > now done differently than rest of atomic and kref...
> 
> It's about generated code size mostly.
> 
> This inline function is way too large to be inlined:
> 
> static inline __refcount_check
> bool refcount_add_not_zero(unsigned int i, refcount_t *r)
> {
> 	unsigned int old, new, val = atomic_read(&r->refs);
> 
> 	for (;;) {
> 		if (!val)
> 			return false;
> 
> 		if (unlikely(val == UINT_MAX))
> 			return true;
> 
> 		new = val + i;
> 		if (new < val)
> 			new = UINT_MAX;
> 		old = atomic_cmpxchg_relaxed(&r->refs, val, new);
> 		if (old == val)
> 			break;
> 
> 		val = old;
> 	}
> 
> 	REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated;
> leaking memory.\n");
> 
> 	return true;
> }
> 
> When used then this function generates this much code on x86-64 defconfig:
> 
> 00000000000084d0 <test>:
>     84d0:	8b 0f                	mov    (%rdi),%ecx
>     84d2:	55                   	push   %rbp
>     84d3:	48 89 e5             	mov    %rsp,%rbp
> 
>     84d6:	85 c9                	test   %ecx,%ecx                |
>     84d8:	74 30                	je     850a <test+0x3a>         |
>     84da:	83 f9 ff             	cmp    $0xffffffff,%ecx         |
>     84dd:	be ff ff ff ff       	mov    $0xffffffff,%esi         |
>     84e2:	75 04                	jne    84e8 <test+0x18>         |
>     84e4:	eb 1d                	jmp    8503 <test+0x33>         |
>     84e6:	89 c1                	mov    %eax,%ecx                |
>     84e8:	8d 51 01             	lea    0x1(%rcx),%edx           |
>     84eb:	89 c8                	mov    %ecx,%eax                |
>     84ed:	39 ca                	cmp    %ecx,%edx                |
>     84ef:	0f 42 d6             	cmovb  %esi,%edx                |
>     84f2:	f0 0f b1 17          	lock cmpxchg %edx,(%rdi)        |
>     84f6:	39 c8                	cmp    %ecx,%eax                |
>     84f8:	74 09                	je     8503 <test+0x33>         |
>     84fa:	85 c0                	test   %eax,%eax                |
>     84fc:	74 0c                	je     850a <test+0x3a>         |
>     84fe:	83 f8 ff             	cmp    $0xffffffff,%eax         |
>     8501:	75 e3                	jne    84e6 <test+0x16>         |
>     8503:	b8 01 00 00 00       	mov    $0x1,%eax                |
> 
>     8508:	5d                   	pop    %rbp
>     8509:	c3                   	retq
>     850a:	31 c0                	xor    %eax,%eax
>     850c:	5d                   	pop    %rbp
>     850d:	c3                   	retq
> 
> 
> (I've annotated the fastpath impact with '|'. Out of line code generally does not
> count.)
> 
> It's about ~50 bytes of code per usage site. It might be better in some cases, but
> not by much.
> 
> This is way above any sane inlining threshold. The 'unconditionally good' inlining
> threshold is at 1-2 instructions and below ~10 bytes of code.
> 
> So for example refcount_set() and refcount_read() can stay inlined:
> 
> static inline void refcount_set(refcount_t *r, unsigned int n)
> {
> 	atomic_set(&r->refs, n);
> }
> 
> static inline unsigned int refcount_read(const refcount_t *r)
> {
> 	return atomic_read(&r->refs);
> }
> 
> 
> ... beacuse they compile into a single instruction with 2-5 bytes I$ overhead.
> 
> Thanks,
> 
> 	Ingo

Thank you very much Ingo for such detailed and nice explanation! 

Best Regards,
Elena

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web