[RESEND PATCH] percpu_ref: Fix comment regarding percpu_ref_init flags

[RESEND PATCH] percpu_ref: Fix comment regarding percpu_ref_init flags

[ira.weiny]

From: Ira Weiny <ira.weiny@intel.com>

The comment for percpu_ref_init() implies that using
PERCPU_REF_ALLOW_REINIT will cause the refcount to start at 0.  But
this is not true.  PERCPU_REF_ALLOW_REINIT starts the count at 1 as
if the flags were zero.  Add this fact to the kernel doc comment.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
RESEND:
	Add more people on the CC list to see if I'm wrong here.
	https://lore.kernel.org/lkml/20200206042810.GA29917@iweiny-DESK2.sc.intel.com/
---

 lib/percpu-refcount.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/percpu-refcount.c b/lib/percpu-refcount.c
index 4f6c6ebbbbde..48d7fcff70b6 100644
--- a/lib/percpu-refcount.c
+++ b/lib/percpu-refcount.c
@@ -50,9 +50,9 @@ static unsigned long __percpu *percpu_count_ptr(struct percpu_ref *ref)
  * @flags: PERCPU_REF_INIT_* flags
  * @gfp: allocation mask to use
  *
- * Initializes @ref.  If @flags is zero, @ref starts in percpu mode with a
- * refcount of 1; analagous to atomic_long_set(ref, 1).  See the
- * definitions of PERCPU_REF_INIT_* flags for flag behaviors.
+ * Initializes @ref.  If @flags is zero or PERCPU_REF_ALLOW_REINIT, @ref starts
+ * in percpu mode with a refcount of 1; analagous to atomic_long_set(ref, 1).
+ * See the definitions of PERCPU_REF_INIT_* flags for flag behaviors.
  *
  * Note that @release must not sleep - it may potentially be called from RCU
  * callback context by percpu_ref_kill().
-- 
2.21.0

Re: [RESEND PATCH] percpu_ref: Fix comment regarding percpu_ref_init flags

[Dennis Zhou]

Hi Ira,

On Fri, Feb 21, 2020 at 03:16:07PM -0800, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> The comment for percpu_ref_init() implies that using
> PERCPU_REF_ALLOW_REINIT will cause the refcount to start at 0.  But
> this is not true.  PERCPU_REF_ALLOW_REINIT starts the count at 1 as
> if the flags were zero.  Add this fact to the kernel doc comment.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
> RESEND:
> 	Add more people on the CC list to see if I'm wrong here.
> 	https://lore.kernel.org/lkml/20200206042810.GA29917@iweiny-DESK2.sc.intel.com/
> ---
> 
>  lib/percpu-refcount.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/percpu-refcount.c b/lib/percpu-refcount.c
> index 4f6c6ebbbbde..48d7fcff70b6 100644
> --- a/lib/percpu-refcount.c
> +++ b/lib/percpu-refcount.c
> @@ -50,9 +50,9 @@ static unsigned long __percpu *percpu_count_ptr(struct percpu_ref *ref)
>   * @flags: PERCPU_REF_INIT_* flags
>   * @gfp: allocation mask to use
>   *
> - * Initializes @ref.  If @flags is zero, @ref starts in percpu mode with a
> - * refcount of 1; analagous to atomic_long_set(ref, 1).  See the
> - * definitions of PERCPU_REF_INIT_* flags for flag behaviors.
> + * Initializes @ref.  If @flags is zero or PERCPU_REF_ALLOW_REINIT, @ref starts
> + * in percpu mode with a refcount of 1; analagous to atomic_long_set(ref, 1).
> + * See the definitions of PERCPU_REF_INIT_* flags for flag behaviors.

Yeah. Prior we had both PERCPU_REF_INIT_ATOMIC and PERCPU_REF_INIT_DEAD
with the latter implying the former. So 0 meant percpu and the others
meant atomic. With PERCPU_REF_ALLOW_REINIT, it's probably easier to
understand by saying if neither PERCPU_REF_INIT_ATOMIC or
PERCPU_REF_INIT_DEAD is set, it starts out in percpu mode which is
mentioned in the comments where the flags are defined.  It's not great
having implied flags, but it's worked so far.

Also, it's not quite analagous to atomic_long_set(ref, 1) as there is a
bias to prevent prematurely hitting 0.

I can take this and massage the wording a bit.

>   *
>   * Note that @release must not sleep - it may potentially be called from RCU
>   * callback context by percpu_ref_kill().
> -- 
> 2.21.0
> 

Thanks,
Dennis

Re: [RESEND PATCH] percpu_ref: Fix comment regarding percpu_ref_init flags

[Roman Gushchin]

On Fri, Feb 21, 2020 at 06:56:27PM -0500, Dennis Zhou wrote:
> Hi Ira,
> 
> On Fri, Feb 21, 2020 at 03:16:07PM -0800, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The comment for percpu_ref_init() implies that using
> > PERCPU_REF_ALLOW_REINIT will cause the refcount to start at 0.  But
> > this is not true.  PERCPU_REF_ALLOW_REINIT starts the count at 1 as
> > if the flags were zero.  Add this fact to the kernel doc comment.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> > RESEND:
> > 	Add more people on the CC list to see if I'm wrong here.
> > 	https://lore.kernel.org/lkml/20200206042810.GA29917@iweiny-DESK2.sc.intel.com/
> > ---
> > 
> >  lib/percpu-refcount.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/percpu-refcount.c b/lib/percpu-refcount.c
> > index 4f6c6ebbbbde..48d7fcff70b6 100644
> > --- a/lib/percpu-refcount.c
> > +++ b/lib/percpu-refcount.c
> > @@ -50,9 +50,9 @@ static unsigned long __percpu *percpu_count_ptr(struct percpu_ref *ref)
> >   * @flags: PERCPU_REF_INIT_* flags
> >   * @gfp: allocation mask to use
> >   *
> > - * Initializes @ref.  If @flags is zero, @ref starts in percpu mode with a
> > - * refcount of 1; analagous to atomic_long_set(ref, 1).  See the
> > - * definitions of PERCPU_REF_INIT_* flags for flag behaviors.
> > + * Initializes @ref.  If @flags is zero or PERCPU_REF_ALLOW_REINIT, @ref starts
> > + * in percpu mode with a refcount of 1; analagous to atomic_long_set(ref, 1).
> > + * See the definitions of PERCPU_REF_INIT_* flags for flag behaviors.
> 
> Yeah. Prior we had both PERCPU_REF_INIT_ATOMIC and PERCPU_REF_INIT_DEAD
> with the latter implying the former. So 0 meant percpu and the others
> meant atomic. With PERCPU_REF_ALLOW_REINIT, it's probably easier to
> understand by saying if neither PERCPU_REF_INIT_ATOMIC or
> PERCPU_REF_INIT_DEAD is set, it starts out in percpu mode which is
> mentioned in the comments where the flags are defined.  It's not great
> having implied flags, but it's worked so far.
> 
> Also, it's not quite analagous to atomic_long_set(ref, 1) as there is a
> bias to prevent prematurely hitting 0.
> 
> I can take this and massage the wording a bit.

Hello Ira! Hello Dennis!

Yeah, I'd simple say that it starts in the percpu mode, except the case when
PERCPU_REF_INIT_ATOMIC is set, then (atomic mode, 1) and
PERCPU_REF_INIT_DEAD is set, then (atomic mode, 0).

PERCPU_REF_ALLOW_REINIT actually doesn't affect the initial state.

Thanks!

Re: [RESEND PATCH] percpu_ref: Fix comment regarding percpu_ref_init flags

[Ira Weiny]

[snip]

> > > diff --git a/lib/percpu-refcount.c b/lib/percpu-refcount.c
> > > index 4f6c6ebbbbde..48d7fcff70b6 100644
> > > --- a/lib/percpu-refcount.c
> > > +++ b/lib/percpu-refcount.c
> > > @@ -50,9 +50,9 @@ static unsigned long __percpu *percpu_count_ptr(struct percpu_ref *ref)
> > >   * @flags: PERCPU_REF_INIT_* flags
> > >   * @gfp: allocation mask to use
> > >   *
> > > - * Initializes @ref.  If @flags is zero, @ref starts in percpu mode with a
> > > - * refcount of 1; analagous to atomic_long_set(ref, 1).  See the
> > > - * definitions of PERCPU_REF_INIT_* flags for flag behaviors.
> > > + * Initializes @ref.  If @flags is zero or PERCPU_REF_ALLOW_REINIT, @ref starts
> > > + * in percpu mode with a refcount of 1; analagous to atomic_long_set(ref, 1).
> > > + * See the definitions of PERCPU_REF_INIT_* flags for flag behaviors.
> > 
> > Yeah. Prior we had both PERCPU_REF_INIT_ATOMIC and PERCPU_REF_INIT_DEAD
> > with the latter implying the former. So 0 meant percpu and the others
> > meant atomic. With PERCPU_REF_ALLOW_REINIT, it's probably easier to
> > understand by saying if neither PERCPU_REF_INIT_ATOMIC or
> > PERCPU_REF_INIT_DEAD is set, it starts out in percpu mode which is
> > mentioned in the comments where the flags are defined.  It's not great
> > having implied flags, but it's worked so far.
> > 
> > Also, it's not quite analagous to atomic_long_set(ref, 1) as there is a
> > bias to prevent prematurely hitting 0.
> > 
> > I can take this and massage the wording a bit.
> 
> Hello Ira! Hello Dennis!
> 
> Yeah, I'd simple say that it starts in the percpu mode, except the case when
> PERCPU_REF_INIT_ATOMIC is set, then (atomic mode, 1) and
> PERCPU_REF_INIT_DEAD is set, then (atomic mode, 0).
> 
> PERCPU_REF_ALLOW_REINIT actually doesn't affect the initial state.
> 

Thanks for the clarification.  Dennis let me know if you want me to resubmit
the patch.

Thanks!
Ira

Re: [RESEND PATCH] percpu_ref: Fix comment regarding percpu_ref_init flags

[Dennis Zhou]

On Sun, Feb 23, 2020 at 06:32:23AM -0800, Ira Weiny wrote:
> 
> [snip]
> 
> > > > diff --git a/lib/percpu-refcount.c b/lib/percpu-refcount.c
> > > > index 4f6c6ebbbbde..48d7fcff70b6 100644
> > > > --- a/lib/percpu-refcount.c
> > > > +++ b/lib/percpu-refcount.c
> > > > @@ -50,9 +50,9 @@ static unsigned long __percpu *percpu_count_ptr(struct percpu_ref *ref)
> > > >   * @flags: PERCPU_REF_INIT_* flags
> > > >   * @gfp: allocation mask to use
> > > >   *
> > > > - * Initializes @ref.  If @flags is zero, @ref starts in percpu mode with a
> > > > - * refcount of 1; analagous to atomic_long_set(ref, 1).  See the
> > > > - * definitions of PERCPU_REF_INIT_* flags for flag behaviors.
> > > > + * Initializes @ref.  If @flags is zero or PERCPU_REF_ALLOW_REINIT, @ref starts
> > > > + * in percpu mode with a refcount of 1; analagous to atomic_long_set(ref, 1).
> > > > + * See the definitions of PERCPU_REF_INIT_* flags for flag behaviors.
> > > 
> > > Yeah. Prior we had both PERCPU_REF_INIT_ATOMIC and PERCPU_REF_INIT_DEAD
> > > with the latter implying the former. So 0 meant percpu and the others
> > > meant atomic. With PERCPU_REF_ALLOW_REINIT, it's probably easier to
> > > understand by saying if neither PERCPU_REF_INIT_ATOMIC or
> > > PERCPU_REF_INIT_DEAD is set, it starts out in percpu mode which is
> > > mentioned in the comments where the flags are defined.  It's not great
> > > having implied flags, but it's worked so far.
> > > 
> > > Also, it's not quite analagous to atomic_long_set(ref, 1) as there is a
> > > bias to prevent prematurely hitting 0.
> > > 
> > > I can take this and massage the wording a bit.
> > 
> > Hello Ira! Hello Dennis!
> > 
> > Yeah, I'd simple say that it starts in the percpu mode, except the case when
> > PERCPU_REF_INIT_ATOMIC is set, then (atomic mode, 1) and
> > PERCPU_REF_INIT_DEAD is set, then (atomic mode, 0).
> > 
> > PERCPU_REF_ALLOW_REINIT actually doesn't affect the initial state.
> > 
> 
> Thanks for the clarification.  Dennis let me know if you want me to resubmit
> the patch.
> 
> Thanks!
> Ira
> 

Sorry for the delay. I've applied it to for-5.7.

Thanks,
Dennis