linux-kernel - Re: [RESEND PATCH] percpu_ref: Fix comment regarding percpu_ref

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200221235627.GA59628@dennisz-mbp.dhcp.thefacebook.com>
Date:   Fri, 21 Feb 2020 18:56:27 -0500
From:   Dennis Zhou <dennis@...nel.org>
To:     ira.weiny@...el.com
Cc:     linux-kernel@...r.kernel.org, Petr Mladek <pmladek@...e.com>,
        Tejun Heo <tj@...nel.org>, Dennis Zhou <dennis@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Roman Gushchin <guro@...com>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>
Subject: Re: [RESEND PATCH] percpu_ref: Fix comment regarding percpu_ref_init
 flags

Hi Ira,

On Fri, Feb 21, 2020 at 03:16:07PM -0800, ira.weiny@...el.com wrote:
> From: Ira Weiny <ira.weiny@...el.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@...el.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