[PATCH] doc/rcuref: Document real world examples in kernel

[PATCH] doc/rcuref: Document real world examples in kernel

[Joel Fernandes (Google)]

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().
+
 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
+reference counting of 'struct pid' while the prior pattern where
+atomic_inc_not_zero() is used is used by struct posix_acl.
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH] doc/rcuref: Document real world examples in kernel

[Jann Horn]

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".

>  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/

Re: [PATCH] doc/rcuref: Document real world examples in kernel

[Joel Fernandes]

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

Re: [PATCH] doc/rcuref: Document real world examples in kernel

[Joel Fernandes]

On Fri, Mar 29, 2019 at 12:44:05AM -0400, Joel Fernandes wrote:
> 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.

Here I meant "I can entirely drop this part of the patch if Paul and others
also feel it is not necessary."

thanks,

 - Joel