diff mbox series

[v2,2/2] mm/slab: break up RCU readers on SLAB_TYPESAFE_BY_RCU example code

Message ID 20230415033159.4249-3-sj@kernel.org
State New
Headers
Series mm/slab: trivial fixup for SLAB_TYPESAFE_BY_RCU example code snippet |

Commit Message

SeongJae Park April 15, 2023, 3:31 a.m. UTC 
  The SLAB_TYPESAFE_BY_RCU example code snippet is having not tiny RCU
read-side critical section.  'Documentation/RCU/rculist_nulls.rst' has
similar example code snippet, and commit da82af04352b ("doc: Update and
wordsmith rculist_nulls.rst") has broken it.  Apply the change to
SLAB_TYPESAFE_BY_RCU example code snippet, too.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 include/linux/slab.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Matthew Wilcox April 15, 2023, 3:49 a.m. UTC | #1 
On Sat, Apr 15, 2023 at 03:31:59AM +0000, SeongJae Park wrote:
> The SLAB_TYPESAFE_BY_RCU example code snippet is having not tiny RCU
> read-side critical section.  'Documentation/RCU/rculist_nulls.rst' has
> similar example code snippet, and commit da82af04352b ("doc: Update and
> wordsmith rculist_nulls.rst") has broken it.  Apply the change to
> SLAB_TYPESAFE_BY_RCU example code snippet, too.

so the page cache (eg find_get_entry()) does not follow this "split
the RCU critical section" pattern.  Should it?  What's the benefit?
SeongJae Park April 15, 2023, 4:27 p.m. UTC | #2 
On Sat, 15 Apr 2023 04:49:44 +0100 Matthew Wilcox <willy@infradead.org> wrote:

> On Sat, Apr 15, 2023 at 03:31:59AM +0000, SeongJae Park wrote:
> > The SLAB_TYPESAFE_BY_RCU example code snippet is having not tiny RCU
> > read-side critical section.  'Documentation/RCU/rculist_nulls.rst' has
> > similar example code snippet, and commit da82af04352b ("doc: Update and
> > wordsmith rculist_nulls.rst") has broken it.  Apply the change to
> > SLAB_TYPESAFE_BY_RCU example code snippet, too.
> 
> so the page cache (eg find_get_entry()) does not follow this "split
> the RCU critical section" pattern.  Should it?  What's the benefit?

The benefit would be shorter RCU grace period that allows lower memory
footprint, iiuc.

Whether it should split the section or not would depend on the lookup speed and
number of retries, I think.  If the total lookup takes a time that long enough
to make the grace period too long and therefore the amount of RCU-protected
objects that cannot freed due to the grace priod is huge, I think it would
better to follow the pattern.


Thanks,
SJ
Vlastimil Babka April 17, 2023, 11:05 a.m. UTC | #3 
On 4/15/23 05:31, SeongJae Park wrote:
> The SLAB_TYPESAFE_BY_RCU example code snippet is having not tiny RCU

Since "tiny RCU" means something quite specific in the RCU world, it can be
confusing to read it in this sense. We could say e.g. "... snippet uses a
single RCU read-side critical section for retries"?

> read-side critical section.  'Documentation/RCU/rculist_nulls.rst' has
> similar example code snippet, and commit da82af04352b ("doc: Update and
> wordsmith rculist_nulls.rst") has broken it.

"has broken it" has quite different meaning than "has broken it up" :) I
guess we could just add the "up", unless someone has an even better wording.

> Apply the change to
> SLAB_TYPESAFE_BY_RCU example code snippet, too.
> 
> Signed-off-by: SeongJae Park <sj@kernel.org>
> ---
>  include/linux/slab.h | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index b18e56c6f06c..6acf1b7c6551 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -53,16 +53,18 @@
>   * stays valid, the trick to using this is relying on an independent
>   * object validation pass. Something like:
>   *
> + * begin:
>   *  rcu_read_lock();
> - * again:
>   *  obj = lockless_lookup(key);
>   *  if (obj) {
>   *    if (!try_get_ref(obj)) // might fail for free objects
> - *      goto again;
> + *      rcu_read_unlock();
> + *      goto begin;
>   *
>   *    if (obj->key != key) { // not the object we expected
>   *      put_ref(obj);
> - *      goto again;
> + *      rcu_read_unlock();
> + *      goto begin;
>   *    }
>   *  }
>   *  rcu_read_unlock();
SeongJae Park April 17, 2023, 5:26 p.m. UTC | #4 
Hi Vlastimil,

On Mon, 17 Apr 2023 13:05:40 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:

> On 4/15/23 05:31, SeongJae Park wrote:
> > The SLAB_TYPESAFE_BY_RCU example code snippet is having not tiny RCU
> 
> Since "tiny RCU" means something quite specific in the RCU world, it can be
> confusing to read it in this sense. We could say e.g. "... snippet uses a
> single RCU read-side critical section for retries"?

Looks much better, thank you for this suggestion!

> 
> > read-side critical section.  'Documentation/RCU/rculist_nulls.rst' has
> > similar example code snippet, and commit da82af04352b ("doc: Update and
> > wordsmith rculist_nulls.rst") has broken it.
> 
> "has broken it" has quite different meaning than "has broken it up" :) I
> guess we could just add the "up", unless someone has an even better wording.

Good point, thank you for your suggestion!

I will apply above suggestion on the next spin.


Thanks,
SJ

> 
> > Apply the change to
> > SLAB_TYPESAFE_BY_RCU example code snippet, too.
> > 
> > Signed-off-by: SeongJae Park <sj@kernel.org>
> > ---
> >  include/linux/slab.h | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > index b18e56c6f06c..6acf1b7c6551 100644
> > --- a/include/linux/slab.h
> > +++ b/include/linux/slab.h
> > @@ -53,16 +53,18 @@
> >   * stays valid, the trick to using this is relying on an independent
> >   * object validation pass. Something like:
> >   *
> > + * begin:
> >   *  rcu_read_lock();
> > - * again:
> >   *  obj = lockless_lookup(key);
> >   *  if (obj) {
> >   *    if (!try_get_ref(obj)) // might fail for free objects
> > - *      goto again;
> > + *      rcu_read_unlock();
> > + *      goto begin;
> >   *
> >   *    if (obj->key != key) { // not the object we expected
> >   *      put_ref(obj);
> > - *      goto again;
> > + *      rcu_read_unlock();
> > + *      goto begin;
> >   *    }
> >   *  }
> >   *  rcu_read_unlock();
>
Matthew Wilcox April 17, 2023, 5:53 p.m. UTC | #5 
On Mon, Apr 17, 2023 at 05:26:57PM +0000, SeongJae Park wrote:
> Hi Vlastimil,
> 
> On Mon, 17 Apr 2023 13:05:40 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:
> 
> > On 4/15/23 05:31, SeongJae Park wrote:
> > > The SLAB_TYPESAFE_BY_RCU example code snippet is having not tiny RCU
> > 
> > Since "tiny RCU" means something quite specific in the RCU world, it can be
> > confusing to read it in this sense. We could say e.g. "... snippet uses a
> > single RCU read-side critical section for retries"?
> 
> Looks much better, thank you for this suggestion!
> 
> > 
> > > read-side critical section.  'Documentation/RCU/rculist_nulls.rst' has
> > > similar example code snippet, and commit da82af04352b ("doc: Update and
> > > wordsmith rculist_nulls.rst") has broken it.
> > 
> > "has broken it" has quite different meaning than "has broken it up" :) I
> > guess we could just add the "up", unless someone has an even better wording.
> 
> Good point, thank you for your suggestion!
> 
> I will apply above suggestion on the next spin.

For the last one, perhaps changing the tense would have more clarity:

similar example code snippet, and commit da82af04352b ("doc: Update and
wordsmith rculist_nulls.rst") broke it up.
SeongJae Park April 17, 2023, 7:01 p.m. UTC | #6 
On Mon, 17 Apr 2023 18:53:24 +0100 Matthew Wilcox <willy@infradead.org> wrote:

> On Mon, Apr 17, 2023 at 05:26:57PM +0000, SeongJae Park wrote:
> > Hi Vlastimil,
> > 
> > On Mon, 17 Apr 2023 13:05:40 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:
> > 
> > > On 4/15/23 05:31, SeongJae Park wrote:
> > > > The SLAB_TYPESAFE_BY_RCU example code snippet is having not tiny RCU
> > > 
> > > Since "tiny RCU" means something quite specific in the RCU world, it can be
> > > confusing to read it in this sense. We could say e.g. "... snippet uses a
> > > single RCU read-side critical section for retries"?
> > 
> > Looks much better, thank you for this suggestion!
> > 
> > > 
> > > > read-side critical section.  'Documentation/RCU/rculist_nulls.rst' has
> > > > similar example code snippet, and commit da82af04352b ("doc: Update and
> > > > wordsmith rculist_nulls.rst") has broken it.
> > > 
> > > "has broken it" has quite different meaning than "has broken it up" :) I
> > > guess we could just add the "up", unless someone has an even better wording.
> > 
> > Good point, thank you for your suggestion!
> > 
> > I will apply above suggestion on the next spin.
> 
> For the last one, perhaps changing the tense would have more clarity:
> 
> similar example code snippet, and commit da82af04352b ("doc: Update and
> wordsmith rculist_nulls.rst") broke it up.

Thank you for this suggestion, Matthew!  Will send a new version.


Thanks,
SJ
Matthew Wilcox April 17, 2023, 7:31 p.m. UTC | #7 
On Mon, Apr 17, 2023 at 07:01:29PM +0000, SeongJae Park wrote:
> Thank you for this suggestion, Matthew!  Will send a new version.

Please wait 24 hours between sending new versions.  Give discussion
some time to happen.
diff mbox series

Patch

diff --git a/include/linux/slab.h b/include/linux/slab.h
index b18e56c6f06c..6acf1b7c6551 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -53,16 +53,18 @@ 
  * stays valid, the trick to using this is relying on an independent
  * object validation pass. Something like:
  *
+ * begin:
  *  rcu_read_lock();
- * again:
  *  obj = lockless_lookup(key);
  *  if (obj) {
  *    if (!try_get_ref(obj)) // might fail for free objects
- *      goto again;
+ *      rcu_read_unlock();
+ *      goto begin;
  *
  *    if (obj->key != key) { // not the object we expected
  *      put_ref(obj);
- *      goto again;
+ *      rcu_read_unlock();
+ *      goto begin;
  *    }
  *  }
  *  rcu_read_unlock();