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

Groups > linux.kernel > #1587859 > unrolled thread

[RFC][PATCH 09/10] locking/refcount: Provide refcount_dec_unless()

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

  [RFC][PATCH 09/10] locking/refcount: Provide refcount_dec_unless() Peter Zijlstra <peterz@infradead.org> - 2017-02-24 19:50 +0100
    RE: [RFC][PATCH 09/10] locking/refcount: Provide  refcount_dec_unless() "Reshetova, Elena" <elena.reshetova@intel.com> - 2017-02-27 10:40 +0100

#1587859 — [RFC][PATCH 09/10] locking/refcount: Provide refcount_dec_unless()

By allowing a different unless value than 1, we can do dec_and_lock
like things on higher values, like 2, which is useful if the data
structure we lock also owns a reference (because in that case we'd
never hit 1).

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 include/linux/refcount.h |    8 +++++++-
 lib/refcount.c           |   14 ++++++++------
 2 files changed, 15 insertions(+), 7 deletions(-)

--- a/include/linux/refcount.h
+++ b/include/linux/refcount.h
@@ -35,7 +35,13 @@ extern __must_check bool refcount_dec_an
 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_unless(refcount_t *r, unsigned int unless);
+
+static inline __must_check bool refcount_dec_not_one(refcount_t *r)
+{
+	return refcount_dec_unless(r, 1);
+}
+
 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);
 
--- a/lib/refcount.c
+++ b/lib/refcount.c
@@ -174,12 +174,14 @@ bool refcount_dec_if_one(refcount_t *r)
 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.
+ * No atomic_t counterpart, it decrements unless the value is as specified, in
+ * which case it will return false.
  *
- * Was often done like: atomic_add_unless(&var, -1, 1)
+ * Was often done like: atomic_add_unless(&var, -1, unless), where the most
+ * common variant has unless==1 is provided as a convenience wrapper, see
+ * refcount_dec_not_one().
  */
-bool refcount_dec_not_one(refcount_t *r)
+bool refcount_dec_unless(refcount_t *r, unsigned int unless)
 {
 	unsigned int new, val = atomic_read(&r->refs);
 
@@ -187,7 +189,7 @@ bool refcount_dec_not_one(refcount_t *r)
 		if (unlikely(val == UINT_MAX))
 			return true;
 
-		if (val == 1)
+		if (val == unless)
 			return false;
 
 		new = val - 1;
@@ -200,7 +202,7 @@ bool refcount_dec_not_one(refcount_t *r)
 
 	return true;
 }
-EXPORT_SYMBOL_GPL(refcount_dec_not_one);
+EXPORT_SYMBOL_GPL(refcount_dec_unless);
 
 /*
  * Similar to atomic_dec_and_mutex_lock(), it will WARN on underflow and fail

[toc] | [next] | [standalone]

#1588562 — RE: [RFC][PATCH 09/10] locking/refcount: Provide refcount_dec_unless()

> By allowing a different unless value than 1, we can do dec_and_lock
> like things on higher values, like 2, which is useful if the data
> structure we lock also owns a reference (because in that case we'd
> never hit 1).
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  include/linux/refcount.h |    8 +++++++-
>  lib/refcount.c           |   14 ++++++++------
>  2 files changed, 15 insertions(+), 7 deletions(-)

Even if we decide not to touch inode->i_count because people believe it doesn't make that much sense, 
this API extension would be highly useful in other places. The consequences if an usage counter overflows can be also very much undesirable and I don't think the code that uses them is prepared to handle such cases. 
So we also need a way to address it if we want to get rid of problem in general.  
In some cases you can do a general +1 on counting scheme, benefit from an existing lock and convert them to refcount_t, but sometimes it is really not appropriate to take a new generic lock.

So, there are basically two ways for converting these cases:

- create a new type, usagecounter_t (put whatever name you like) (as we discussed before a number of times) and provide below functions there
- extend refcount_t type to have the below functions 

First approach might be overall cleaner, but extend even further the set of atomic-like primitives we already have. Some people get confused when given too many choices and default to atomic_t as "old, default option". We don't want this to happen.
Second approach is more compact, but its usage needs to be documented more precisely. Personally it is hard for me to think of a case when adding refcount_dec_unless() to API
would result in people misusing refcount_t API more than they would do without it. But maybe I am not creative enough? 

Best Regards,
Elena. 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web