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@xxxxxxxxx wrote:
> From: Ira Weiny <ira.weiny@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> ---
> RESEND:
> 	Add more people on the CC list to see if I'm wrong here.
> 	https://lore.kernel.org/lkml/20200206042810.GA29917@xxxxxxxxxxxxxxxxxxxxxxxxx/
> ---
> 
>  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