[PATCH percpu/for-3.16 1/2] percpu-refcount: rename percpu_ref_tryget() to percpu_ref_tryget_live()

[PATCH percpu/for-3.16 1/2] percpu-refcount: rename percpu_ref_tryget() to percpu_ref_tryget_live()

[Tejun Heo]

percpu_ref_tryget() is different from the usual tryget semantics in
that it fails if the refcnt is in its dying stage even if the refcnt
hasn't reached zero yet.  We're about to introduce the more
conventional tryget and the current one has only one user.  Let's
rename it to percpu_ref_tryget_live() so that it explicitly signifies
the peculiarities of its semantics.

This is pure rename.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Kent Overstreet <koverstreet@google.com>
---
 include/linux/cgroup.h          |    2 +-
 include/linux/percpu-refcount.h |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -108,7 +108,7 @@ static inline bool css_tryget(struct cgr
 {
 	if (css->flags & CSS_ROOT)
 		return true;
-	return percpu_ref_tryget(&css->refcnt);
+	return percpu_ref_tryget_live(&css->refcnt);
 }
 
 /**
--- a/include/linux/percpu-refcount.h
+++ b/include/linux/percpu-refcount.h
@@ -118,7 +118,7 @@ static inline void percpu_ref_get(struct
 }
 
 /**
- * percpu_ref_tryget - try to increment a percpu refcount
+ * percpu_ref_tryget_live - try to increment a live percpu refcount
  * @ref: percpu_ref to try-get
  *
  * Increment a percpu refcount unless it has already been killed.  Returns
@@ -129,7 +129,7 @@ static inline void percpu_ref_get(struct
  * used.  After the confirm_kill callback is invoked, it's guaranteed that
  * no new reference will be given out by percpu_ref_tryget().
  */
-static inline bool percpu_ref_tryget(struct percpu_ref *ref)
+static inline bool percpu_ref_tryget_live(struct percpu_ref *ref)
 {
 	unsigned __percpu *pcpu_count;
 	int ret = false;

[PATCH REPOST percpu/for-3.16 1/2] percpu-refcount: rename percpu_ref_tryget() to percpu_ref_tryget_live()

[Tejun Heo]

percpu_ref_tryget() is different from the usual tryget semantics in
that it fails if the refcnt is in its dying stage even if the refcnt
hasn't reached zero yet.  We're about to introduce the more
conventional tryget and the current one has only one user.  Let's
rename it to percpu_ref_tryget_live() so that it explicitly signifies
the peculiarities of its semantics.

This is pure rename.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Kent Overstreet <kmo@daterainc.com>
---
Reposting with Kent's new email address.

Thanks.

 include/linux/cgroup.h          |    2 +-
 include/linux/percpu-refcount.h |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -108,7 +108,7 @@ static inline bool css_tryget(struct cgr
 {
 	if (css->flags & CSS_ROOT)
 		return true;
-	return percpu_ref_tryget(&css->refcnt);
+	return percpu_ref_tryget_live(&css->refcnt);
 }
 
 /**
--- a/include/linux/percpu-refcount.h
+++ b/include/linux/percpu-refcount.h
@@ -118,7 +118,7 @@ static inline void percpu_ref_get(struct
 }
 
 /**
- * percpu_ref_tryget - try to increment a percpu refcount
+ * percpu_ref_tryget_live - try to increment a live percpu refcount
  * @ref: percpu_ref to try-get
  *
  * Increment a percpu refcount unless it has already been killed.  Returns
@@ -129,7 +129,7 @@ static inline void percpu_ref_get(struct
  * used.  After the confirm_kill callback is invoked, it's guaranteed that
  * no new reference will be given out by percpu_ref_tryget().
  */
-static inline bool percpu_ref_tryget(struct percpu_ref *ref)
+static inline bool percpu_ref_tryget_live(struct percpu_ref *ref)
 {
 	unsigned __percpu *pcpu_count;
 	int ret = false;

[PATCH percpu/for-3.16 2/2] percpu-refcount: implement percpu_ref_tryget()

[Tejun Heo]

Implement percpu_ref_tryget() which fails if the refcnt already
reached zero.  Note that this is different from the recently renamed
percpu_ref_tryget_live() which fails if the refcnt has been killed and
is draining the remaining references.  percpu_ref_tryget() succeeds on
a killed refcnt as long as its current refcnt is above zero.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Kent Overstreet <kmo@daterainc.com>
---
 include/linux/percpu-refcount.h |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

--- a/include/linux/percpu-refcount.h
+++ b/include/linux/percpu-refcount.h
@@ -118,6 +118,36 @@ static inline void percpu_ref_get(struct
 }
 
 /**
+ * percpu_ref_tryget - try to increment a percpu refcount
+ * @ref: percpu_ref to try-get
+ *
+ * Increment a percpu refcount unless its count already reached zero.
+ * Returns %true on success; %false on failure.
+ *
+ * The caller is responsible for ensuring that @ref stays accessible.
+ */
+static inline bool percpu_ref_tryget(struct percpu_ref *ref)
+{
+	unsigned __percpu *pcpu_count;
+	int ret = false;
+
+	rcu_read_lock_sched();
+
+	pcpu_count = ACCESS_ONCE(ref->pcpu_count);
+
+	if (likely(REF_STATUS(pcpu_count) == PCPU_REF_PTR)) {
+		__this_cpu_inc(*pcpu_count);
+		ret = true;
+	} else {
+		ret = atomic_inc_not_zero(&ref->count);
+	}
+
+	rcu_read_unlock_sched();
+
+	return ret;
+}
+
+/**
  * percpu_ref_tryget_live - try to increment a live percpu refcount
  * @ref: percpu_ref to try-get
  *
@@ -128,6 +158,8 @@ static inline void percpu_ref_get(struct
  * will fail.  For such guarantee, percpu_ref_kill_and_confirm() should be
  * used.  After the confirm_kill callback is invoked, it's guaranteed that
  * no new reference will be given out by percpu_ref_tryget().
+ *
+ * The caller is responsible for ensuring that @ref stays accessible.
  */
 static inline bool percpu_ref_tryget_live(struct percpu_ref *ref)
 {

Re: [PATCH percpu/for-3.16 2/2] percpu-refcount: implement percpu_ref_tryget()

[Tejun Heo]

On Wed, May 07, 2014 at 11:58:10AM -0400, Tejun Heo wrote:
> Implement percpu_ref_tryget() which fails if the refcnt already
> reached zero.  Note that this is different from the recently renamed
> percpu_ref_tryget_live() which fails if the refcnt has been killed and
> is draining the remaining references.  percpu_ref_tryget() succeeds on
> a killed refcnt as long as its current refcnt is above zero.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Kent Overstreet <kmo@daterainc.com>

Applying 1-2 to percpu/for-3.16.

Thanks.

-- 
tejun

Re: [PATCH percpu/for-3.16 2/2] percpu-refcount: implement percpu_ref_tryget()

[Kent Overstreet]

On Wed, May 07, 2014 at 11:58:10AM -0400, Tejun Heo wrote:
> Implement percpu_ref_tryget() which fails if the refcnt already
> reached zero.  Note that this is different from the recently renamed
> percpu_ref_tryget_live() which fails if the refcnt has been killed and
> is draining the remaining references.  percpu_ref_tryget() succeeds on
> a killed refcnt as long as its current refcnt is above zero.

I'd still kind of prefer tryget() to be labelled "deprecated, don't use outside
the cgroup code" or somesuch, but it's not a huge deal :)

Acked-by: Kent Overstreet <kmo@daterainc.com>

> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Kent Overstreet <kmo@daterainc.com>
> ---
>  include/linux/percpu-refcount.h |   32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> --- a/include/linux/percpu-refcount.h
> +++ b/include/linux/percpu-refcount.h
> @@ -118,6 +118,36 @@ static inline void percpu_ref_get(struct
>  }
>  
>  /**
> + * percpu_ref_tryget - try to increment a percpu refcount
> + * @ref: percpu_ref to try-get
> + *
> + * Increment a percpu refcount unless its count already reached zero.
> + * Returns %true on success; %false on failure.
> + *
> + * The caller is responsible for ensuring that @ref stays accessible.
> + */
> +static inline bool percpu_ref_tryget(struct percpu_ref *ref)
> +{
> +	unsigned __percpu *pcpu_count;
> +	int ret = false;
> +
> +	rcu_read_lock_sched();
> +
> +	pcpu_count = ACCESS_ONCE(ref->pcpu_count);
> +
> +	if (likely(REF_STATUS(pcpu_count) == PCPU_REF_PTR)) {
> +		__this_cpu_inc(*pcpu_count);
> +		ret = true;
> +	} else {
> +		ret = atomic_inc_not_zero(&ref->count);
> +	}
> +
> +	rcu_read_unlock_sched();
> +
> +	return ret;
> +}
> +
> +/**
>   * percpu_ref_tryget_live - try to increment a live percpu refcount
>   * @ref: percpu_ref to try-get
>   *
> @@ -128,6 +158,8 @@ static inline void percpu_ref_get(struct
>   * will fail.  For such guarantee, percpu_ref_kill_and_confirm() should be
>   * used.  After the confirm_kill callback is invoked, it's guaranteed that
>   * no new reference will be given out by percpu_ref_tryget().
> + *
> + * The caller is responsible for ensuring that @ref stays accessible.
>   */
>  static inline bool percpu_ref_tryget_live(struct percpu_ref *ref)
>  {

Re: [PATCH percpu/for-3.16 2/2] percpu-refcount: implement percpu_ref_tryget()

[Tejun Heo]

On Fri, May 09, 2014 at 12:40:21PM -0700, Kent Overstreet wrote:
> On Wed, May 07, 2014 at 11:58:10AM -0400, Tejun Heo wrote:
> > Implement percpu_ref_tryget() which fails if the refcnt already
> > reached zero.  Note that this is different from the recently renamed
> > percpu_ref_tryget_live() which fails if the refcnt has been killed and
> > is draining the remaining references.  percpu_ref_tryget() succeeds on
> > a killed refcnt as long as its current refcnt is above zero.
> 
> I'd still kind of prefer tryget() to be labelled "deprecated, don't use outside
> the cgroup code" or somesuch, but it's not a huge deal :)

Hmmm... why would it be deprecated?  These are just two different
operations.

Thanks.

-- 
tejun

Re: [PATCH percpu/for-3.16 2/2] percpu-refcount: implement percpu_ref_tryget()

[Kent Overstreet]

On Fri, May 09, 2014 at 03:41:40PM -0400, Tejun Heo wrote:
> On Fri, May 09, 2014 at 12:40:21PM -0700, Kent Overstreet wrote:
> > On Wed, May 07, 2014 at 11:58:10AM -0400, Tejun Heo wrote:
> > > Implement percpu_ref_tryget() which fails if the refcnt already
> > > reached zero.  Note that this is different from the recently renamed
> > > percpu_ref_tryget_live() which fails if the refcnt has been killed and
> > > is draining the remaining references.  percpu_ref_tryget() succeeds on
> > > a killed refcnt as long as its current refcnt is above zero.
> > 
> > I'd still kind of prefer tryget() to be labelled "deprecated, don't use outside
> > the cgroup code" or somesuch, but it's not a huge deal :)
> 
> Hmmm... why would it be deprecated?  These are just two different
> operations.

Well not so much deprecated as "bad, avoid" - IMO using tryget() almost always
(I haven't seen a convincing counterexample) means you screwed up your
refcounting somewhere, if you need to take a ref on something whatever made that
object visible to you should have its own ref.

(I think we had this debate, but that was awhile ago...)

Re: [PATCH percpu/for-3.16 2/2] percpu-refcount: implement percpu_ref_tryget()

[Tejun Heo]

Hello,

On Fri, May 09, 2014 at 12:51:09PM -0700, Kent Overstreet wrote:
> Well not so much deprecated as "bad, avoid" - IMO using tryget() almost always
> (I haven't seen a convincing counterexample) means you screwed up your
> refcounting somewhere, if you need to take a ref on something whatever made that
> object visible to you should have its own ref.
> 
> (I think we had this debate, but that was awhile ago...)

Oh sure, tryget can definitely be misunderstood but RCU protected
iteration is one valid use case.

	rcu_read_lock();
	locate the object of interest;
	tryget[_live]() depending on the use case;
	rcu_read_unlock();

	access the object.

It's not different from why we use atomic_inc_not_zero() in some
places.  The only difference is that percpu_ref distinguishes live
vs. dying states.  It's true that this can be used in pretty stupid
ways but I think the comments are pretty clear on that.  Do you think
we need more warning there?

Thanks.

-- 
tejun

Re: [PATCH percpu/for-3.16 2/2] percpu-refcount: implement percpu_ref_tryget()

[Kent Overstreet]

On Fri, May 09, 2014 at 03:55:22PM -0400, Tejun Heo wrote:
> Hello,
> 
> On Fri, May 09, 2014 at 12:51:09PM -0700, Kent Overstreet wrote:
> > Well not so much deprecated as "bad, avoid" - IMO using tryget() almost always
> > (I haven't seen a convincing counterexample) means you screwed up your
> > refcounting somewhere, if you need to take a ref on something whatever made that
> > object visible to you should have its own ref.
> > 
> > (I think we had this debate, but that was awhile ago...)
> 
> Oh sure, tryget can definitely be misunderstood but RCU protected
> iteration is one valid use case.
> 
> 	rcu_read_lock();
> 	locate the object of interest;
> 	tryget[_live]() depending on the use case;
> 	rcu_read_unlock();
> 
> 	access the object.

No, it's not needed with RCU... look at the aio code for an example (or don't,
save your eyes instead).

Conceptually the RCU data structure should own a refcount on the things that are
accessible via it; that ref shouldn't be dropped until after it's removed and an
RCU barrier has happened.

Re: [PATCH percpu/for-3.16 2/2] percpu-refcount: implement percpu_ref_tryget()

[Tejun Heo]

Hey,

On Sun, May 11, 2014 at 05:06:19PM -0700, Kent Overstreet wrote:
> On Fri, May 09, 2014 at 03:55:22PM -0400, Tejun Heo wrote:
> > Hello,
> > 
> > On Fri, May 09, 2014 at 12:51:09PM -0700, Kent Overstreet wrote:
> > > Well not so much deprecated as "bad, avoid" - IMO using tryget() almost always
> > > (I haven't seen a convincing counterexample) means you screwed up your
> > > refcounting somewhere, if you need to take a ref on something whatever made that
> > > object visible to you should have its own ref.
> > > 
> > > (I think we had this debate, but that was awhile ago...)
> > 
> > Oh sure, tryget can definitely be misunderstood but RCU protected
> > iteration is one valid use case.
> > 
> > 	rcu_read_lock();
> > 	locate the object of interest;
> > 	tryget[_live]() depending on the use case;
> > 	rcu_read_unlock();
> > 
> > 	access the object.
> 
> No, it's not needed with RCU... look at the aio code for an example (or don't,
> save your eyes instead).
> 
> Conceptually the RCU data structure should own a refcount on the things that are
> accessible via it; that ref shouldn't be dropped until after it's removed and an
> RCU barrier has happened.

You can do that if the object can disappear from iteration when its
ref gets killed.  IOW, the list visiblity is not tied to the reference
count.  cgroup_subsys_states can't do that.  They have to remain
visible to iteration until the object's last ref is dropped because
controllers need to be able to walk them while the object is being
drained.

Thanks.

-- 
tejun