Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 29 Mar 2019 00:44:05 -0400
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Jann Horn <jannh@google.com>
Cc:     kernel list <linux-kernel@vger.kernel.org>,
        Oleg Nesterov <oleg@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Josh Triplett <josh@joshtriplett.org>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        linux-doc@vger.kernel.org,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        "Paul E. McKenney" <paulmck@linux.ibm.com>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH] doc/rcuref: Document real world examples in kernel
Message-ID: <20190329044405.GA229534@google.com>
References: <20190329024047.206989-1-joel@joelfernandes.org>
 <CAG48ez1DrHKG2PoQjdpc=vT55enaRgQpGfAwo5ujV25TLHJL0g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAG48ez1DrHKG2PoQjdpc=vT55enaRgQpGfAwo5ujV25TLHJL0g@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Mar 29, 2019 at 05:06:21AM +0100, Jann Horn wrote:
> On Fri, Mar 29, 2019 at 3:40 AM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> > Document similar real world examples in the kernel corresponding to the
> > second and third code snippets. Also correct an issue in
> > release_referenced() in the code snippet example.
> >
> > Cc: oleg@redhat.com
> > Cc: jannh@google.com
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >
> > ---
> >  Documentation/RCU/rcuref.txt | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/RCU/rcuref.txt b/Documentation/RCU/rcuref.txt
> > index 613033ff2b9b..e5f4a49f886a 100644
> > --- a/Documentation/RCU/rcuref.txt
> > +++ b/Documentation/RCU/rcuref.txt
> > @@ -28,7 +28,8 @@ add()                         search_and_reference()
> >  release_referenced()                   delete()
> >  {                                      {
> >      ...                                            write_lock(&list_lock);
> > -    atomic_dec(&el->rc, relfunc)           ...
> > +    if(atomic_dec_and_test(&el->rc))       ...
> > +       kfree(el);
> >      ...                                            remove_element
> >  }                                          write_unlock(&list_lock);
> >                                             ...
> > @@ -114,6 +115,11 @@ element can therefore safely be freed.  This in turn guarantees that if
> >  any reader finds the element, that reader may safely acquire a reference
> >  without checking the value of the reference counter.
> >
> > +The other advantage of the last pattern is, if there are several calls to
> > +search_and_reference() in parallel to the delete(), then all of those will
> > +succeed in obtaining a reference to the object if the object could be found in
> > +the list before it was deleted in delete().
> 
> Isn't this the same as what the previous paragraph said? "if
> any reader finds the element, that reader may safely acquire a reference
> without checking the value of the reference counter".

You are right.  But I felt it was less explicit about the fact that several
search_and_reference() calls can succeed will not FAIL like the previous example.

I can reword it as below:

As can be seen, a clear advantage of the last pattern is, if there are
several calls to search_and_reference() in parallel to the delete(), then all
of those will succeed in obtaining a reference to the object if the object
could be found in the list before it was deleted in delete(), unlike the
previous pattern which would fail to acquire references.

Or, can I entirely drop it if Paul and others also feel it is not necessary.

> >  In cases where delete() can sleep, synchronize_rcu() can be called from
> >  delete(), so that el_free() can be subsumed into delete as follows:
> >
> > @@ -130,3 +136,7 @@ delete()
> >         kfree(el);
> >      ...
> >  }
> > +
> > +As additional examples in the kernel, This last pattern is also followed by
> 
> nit: s/, This/, this/

Will fix.

Thanks Jann!

 - Joel