[PATCH v2 0/2] mm/slab: trivial fixup for SLAB_TYPESAFE_BY_RCU example code snippet

[PATCH v2 0/2] mm/slab: trivial fixup for SLAB_TYPESAFE_BY_RCU example code snippet

[SeongJae Park]

Changes from v1
(https://lore.kernel.org/linux-mm/20230415003754.1852-1-sj@kernel.org/)
- Update label (s/again/begin/) correctly (Matthew Wilcox)
- Add missed rcu_read_unlock()

This patchset is for trivial fixup for SLAB_TYPESAFE_BY_RCU example code
snippet, namely adding missed semicolon and breaking RCU read-side
critical section into smaller ones.

SeongJae Park (2):
  mm/slab: add a missing semicolon on SLAB_TYPESAFE_BY_RCU example code
  mm/slab: break up RCU readers on SLAB_TYPESAFE_BY_RCU example code

 include/linux/slab.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] mm/slab: add a missing semicolon on SLAB_TYPESAFE_BY_RCU example code

[SeongJae Park]

An example code snippet for SLAB_TYPESAFE_BY_RCU is missing a semicolon.
Add it.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 include/linux/slab.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index f8b1d63c63a3..b18e56c6f06c 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -53,7 +53,7 @@
  * stays valid, the trick to using this is relying on an independent
  * object validation pass. Something like:
  *
- *  rcu_read_lock()
+ *  rcu_read_lock();
  * again:
  *  obj = lockless_lookup(key);
  *  if (obj) {
-- 
2.25.1

Re: [PATCH v2 1/2] mm/slab: add a missing semicolon on SLAB_TYPESAFE_BY_RCU example code

[Paul E. McKenney]

On Sat, Apr 15, 2023 at 03:31:58AM +0000, SeongJae Park wrote:
> An example code snippet for SLAB_TYPESAFE_BY_RCU is missing a semicolon.
> Add it.
> 
> Signed-off-by: SeongJae Park <sj@kernel.org>

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

Or please let me know if you would like me to take it.  (Probably better
going up through the usual slab route, though.)

							Thanx, Paul

> ---
>  include/linux/slab.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index f8b1d63c63a3..b18e56c6f06c 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -53,7 +53,7 @@
>   * stays valid, the trick to using this is relying on an independent
>   * object validation pass. Something like:
>   *
> - *  rcu_read_lock()
> + *  rcu_read_lock();
>   * again:
>   *  obj = lockless_lookup(key);
>   *  if (obj) {
> -- 
> 2.25.1
> 

Re: [PATCH v2 1/2] mm/slab: add a missing semicolon on SLAB_TYPESAFE_BY_RCU example code

[Vlastimil Babka]

On 4/24/23 19:43, Paul E. McKenney wrote:
> On Sat, Apr 15, 2023 at 03:31:58AM +0000, SeongJae Park wrote:
>> An example code snippet for SLAB_TYPESAFE_BY_RCU is missing a semicolon.
>> Add it.
>>
>> Signed-off-by: SeongJae Park <sj@kernel.org>
> 
> Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> 
> Or please let me know if you would like me to take it.  (Probably better
> going up through the usual slab route, though.)

Yeah will take it via slab after merge window, but was hoping you'd ack
that (mainly 2/2) as indeed the correct example first.

Thanks, Vlastimil

> 							Thanx, Paul
> 
>> ---
>>  include/linux/slab.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/slab.h b/include/linux/slab.h
>> index f8b1d63c63a3..b18e56c6f06c 100644
>> --- a/include/linux/slab.h
>> +++ b/include/linux/slab.h
>> @@ -53,7 +53,7 @@
>>   * stays valid, the trick to using this is relying on an independent
>>   * object validation pass. Something like:
>>   *
>> - *  rcu_read_lock()
>> + *  rcu_read_lock();
>>   * again:
>>   *  obj = lockless_lookup(key);
>>   *  if (obj) {
>> -- 
>> 2.25.1
>>

Re: [PATCH v2 1/2] mm/slab: add a missing semicolon on SLAB_TYPESAFE_BY_RCU example code

[Paul E. McKenney]

On Mon, Apr 24, 2023 at 08:27:45PM +0200, Vlastimil Babka wrote:
> On 4/24/23 19:43, Paul E. McKenney wrote:
> > On Sat, Apr 15, 2023 at 03:31:58AM +0000, SeongJae Park wrote:
> >> An example code snippet for SLAB_TYPESAFE_BY_RCU is missing a semicolon.
> >> Add it.
> >>
> >> Signed-off-by: SeongJae Park <sj@kernel.org>
> > 
> > Reviewed-by: Paul E. McKenney <paulmck@kernel.org>
> > 
> > Or please let me know if you would like me to take it.  (Probably better
> > going up through the usual slab route, though.)
> 
> Yeah will take it via slab after merge window, but was hoping you'd ack
> that (mainly 2/2) as indeed the correct example first.

You got it!  ;-)

							Thanx, Paul

> Thanks, Vlastimil
> 
> > 							Thanx, Paul
> > 
> >> ---
> >>  include/linux/slab.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/linux/slab.h b/include/linux/slab.h
> >> index f8b1d63c63a3..b18e56c6f06c 100644
> >> --- a/include/linux/slab.h
> >> +++ b/include/linux/slab.h
> >> @@ -53,7 +53,7 @@
> >>   * stays valid, the trick to using this is relying on an independent
> >>   * object validation pass. Something like:
> >>   *
> >> - *  rcu_read_lock()
> >> + *  rcu_read_lock();
> >>   * again:
> >>   *  obj = lockless_lookup(key);
> >>   *  if (obj) {
> >> -- 
> >> 2.25.1
> >>

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

[SeongJae Park]

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(-)

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();
-- 
2.25.1

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

[Matthew Wilcox]

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?

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

[SeongJae Park]

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

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

[Vlastimil Babka]

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();

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

[SeongJae Park]

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();
> 

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

[Matthew Wilcox]

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.

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

[SeongJae Park]

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

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

[Vlastimil Babka]

On 4/17/23 21:01, SeongJae Park wrote:
> 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.

It's ok, I can just use that when picking the patches up without a new resend.

> Thanks,
> SJ

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

[SeongJae Park]

On Mon, 17 Apr 2023 21:02:22 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:

> On 4/17/23 21:01, SeongJae Park wrote:
> > 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.
> 
> It's ok, I can just use that when picking the patches up without a new resend.

Sorry, already sent[1]...  Please use or ignore it on your convenience.

[1] https://lore.kernel.org/linux-mm/20230417190450.1682-1-sj@kernel.org/


Thanks,
SJ

> 
> > Thanks,
> > SJ
> 

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

[Matthew Wilcox]

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.