| Message ID | 20230415033159.4249-3-sj@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/slab: trivial fixup for SLAB_TYPESAFE_BY_RCU example code snippet | expand |
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?
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
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();
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(); >
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.
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
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
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 >
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 --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();
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(-)