| Message ID | 97594073-e296-4876-9d6a-1e4a4f33d857@paulmck-laptop (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | srcu: Guarantee non-negative return value from srcu_read_lock() | expand |
On Mon, Oct 21, 2024 at 3:13 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > For almost 20 years, the int return value from srcu_read_lock() has > been always either zero or one. This commit therefore documents the > fact that it will be non-negative. > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Andrii Nakryiko <andrii@kernel.org > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > index bab1dae3f69e6..512a8c54ba5ba 100644 > --- a/include/linux/srcu.h > +++ b/include/linux/srcu.h > @@ -238,13 +238,14 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor); > * a mutex that is held elsewhere while calling synchronize_srcu() or > * synchronize_srcu_expedited(). > * > - * The return value from srcu_read_lock() must be passed unaltered > - * to the matching srcu_read_unlock(). Note that srcu_read_lock() and > - * the matching srcu_read_unlock() must occur in the same context, for > - * example, it is illegal to invoke srcu_read_unlock() in an irq handler > - * if the matching srcu_read_lock() was invoked in process context. Or, > - * for that matter to invoke srcu_read_unlock() from one task and the > - * matching srcu_read_lock() from another. > + * The return value from srcu_read_lock() is guaranteed to be > + * non-negative. This value must be passed unaltered to the matching > + * srcu_read_unlock(). Note that srcu_read_lock() and the matching > + * srcu_read_unlock() must occur in the same context, for example, it is > + * illegal to invoke srcu_read_unlock() in an irq handler if the matching > + * srcu_read_lock() was invoked in process context. Or, for that matter to > + * invoke srcu_read_unlock() from one task and the matching srcu_read_lock() > + * from another. For uprobe work I'm using __srcu_read_lock() and __srcu_read_unlock(). Presumably the same non-negative index will be returned/consumed there as well, right? Can we add a blurb to that effect for them as well? Otherwise LGTM, thanks! Acked-by: Andrii Nakryiko <andrii@kernel.org> > */ > static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp) > {
On Mon, Oct 21, 2024 at 04:50:44PM -0700, Andrii Nakryiko wrote: > On Mon, Oct 21, 2024 at 3:13 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > > > For almost 20 years, the int return value from srcu_read_lock() has > > been always either zero or one. This commit therefore documents the > > fact that it will be non-negative. > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: Andrii Nakryiko <andrii@kernel.org > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > > index bab1dae3f69e6..512a8c54ba5ba 100644 > > --- a/include/linux/srcu.h > > +++ b/include/linux/srcu.h > > @@ -238,13 +238,14 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor); > > * a mutex that is held elsewhere while calling synchronize_srcu() or > > * synchronize_srcu_expedited(). > > * > > - * The return value from srcu_read_lock() must be passed unaltered > > - * to the matching srcu_read_unlock(). Note that srcu_read_lock() and > > - * the matching srcu_read_unlock() must occur in the same context, for > > - * example, it is illegal to invoke srcu_read_unlock() in an irq handler > > - * if the matching srcu_read_lock() was invoked in process context. Or, > > - * for that matter to invoke srcu_read_unlock() from one task and the > > - * matching srcu_read_lock() from another. > > + * The return value from srcu_read_lock() is guaranteed to be > > + * non-negative. This value must be passed unaltered to the matching > > + * srcu_read_unlock(). Note that srcu_read_lock() and the matching > > + * srcu_read_unlock() must occur in the same context, for example, it is > > + * illegal to invoke srcu_read_unlock() in an irq handler if the matching > > + * srcu_read_lock() was invoked in process context. Or, for that matter to > > + * invoke srcu_read_unlock() from one task and the matching srcu_read_lock() > > + * from another. > > For uprobe work I'm using __srcu_read_lock() and __srcu_read_unlock(). > Presumably the same non-negative index will be returned/consumed there > as well, right? Can we add a blurb to that effect for them as well? Does the change shown below cover it? > Otherwise LGTM, thanks! > > Acked-by: Andrii Nakryiko <andrii@kernel.org> Thank you -- I will apply on my next rebase. Thanx, Paul > > */ > > static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp) > > { ------------------------------------------------------------------------ diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index 07147efcb64d3..3d587bf2b2c12 100644 --- a/kernel/rcu/srcutree.c +++ b/kernel/rcu/srcutree.c @@ -738,7 +738,8 @@ EXPORT_SYMBOL_GPL(srcu_check_read_flavor); /* * Counts the new reader in the appropriate per-CPU element of the * srcu_struct. - * Returns an index that must be passed to the matching srcu_read_unlock(). + * Returns a guaranteed non-negative index that must be passed to the + * matching srcu_read_unlock(). */ int __srcu_read_lock(struct srcu_struct *ssp) {
On Mon, Oct 21, 2024 at 5:21 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > On Mon, Oct 21, 2024 at 04:50:44PM -0700, Andrii Nakryiko wrote: > > On Mon, Oct 21, 2024 at 3:13 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > > > > > For almost 20 years, the int return value from srcu_read_lock() has > > > been always either zero or one. This commit therefore documents the > > > fact that it will be non-negative. > > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > Cc: Peter Zijlstra <peterz@infradead.org> > > > Cc: Andrii Nakryiko <andrii@kernel.org > > > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > > > index bab1dae3f69e6..512a8c54ba5ba 100644 > > > --- a/include/linux/srcu.h > > > +++ b/include/linux/srcu.h > > > @@ -238,13 +238,14 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor); > > > * a mutex that is held elsewhere while calling synchronize_srcu() or > > > * synchronize_srcu_expedited(). > > > * > > > - * The return value from srcu_read_lock() must be passed unaltered > > > - * to the matching srcu_read_unlock(). Note that srcu_read_lock() and > > > - * the matching srcu_read_unlock() must occur in the same context, for > > > - * example, it is illegal to invoke srcu_read_unlock() in an irq handler > > > - * if the matching srcu_read_lock() was invoked in process context. Or, > > > - * for that matter to invoke srcu_read_unlock() from one task and the > > > - * matching srcu_read_lock() from another. > > > + * The return value from srcu_read_lock() is guaranteed to be > > > + * non-negative. This value must be passed unaltered to the matching > > > + * srcu_read_unlock(). Note that srcu_read_lock() and the matching > > > + * srcu_read_unlock() must occur in the same context, for example, it is > > > + * illegal to invoke srcu_read_unlock() in an irq handler if the matching > > > + * srcu_read_lock() was invoked in process context. Or, for that matter to > > > + * invoke srcu_read_unlock() from one task and the matching srcu_read_lock() > > > + * from another. > > > > For uprobe work I'm using __srcu_read_lock() and __srcu_read_unlock(). > > Presumably the same non-negative index will be returned/consumed there > > as well, right? Can we add a blurb to that effect for them as well? > > Does the change shown below cover it? Yep, looks good, thank you! You might want to fix s/srcu_read_unlock/__srcu_read_unlock/, while at it, but that's orthogonal. > > > Otherwise LGTM, thanks! > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Thank you -- I will apply on my next rebase. > > Thanx, Paul > > > > */ > > > static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp) > > > { > > ------------------------------------------------------------------------ > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 07147efcb64d3..3d587bf2b2c12 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -738,7 +738,8 @@ EXPORT_SYMBOL_GPL(srcu_check_read_flavor); > /* > * Counts the new reader in the appropriate per-CPU element of the > * srcu_struct. > - * Returns an index that must be passed to the matching srcu_read_unlock(). > + * Returns a guaranteed non-negative index that must be passed to the > + * matching srcu_read_unlock(). > */ > int __srcu_read_lock(struct srcu_struct *ssp) > {
On Mon, Oct 21, 2024 at 07:01:02PM -0700, Andrii Nakryiko wrote: > On Mon, Oct 21, 2024 at 5:21 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > > > On Mon, Oct 21, 2024 at 04:50:44PM -0700, Andrii Nakryiko wrote: > > > On Mon, Oct 21, 2024 at 3:13 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > > > > > > > For almost 20 years, the int return value from srcu_read_lock() has > > > > been always either zero or one. This commit therefore documents the > > > > fact that it will be non-negative. > > > > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > > Cc: Peter Zijlstra <peterz@infradead.org> > > > > Cc: Andrii Nakryiko <andrii@kernel.org > > > > > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > > > > index bab1dae3f69e6..512a8c54ba5ba 100644 > > > > --- a/include/linux/srcu.h > > > > +++ b/include/linux/srcu.h > > > > @@ -238,13 +238,14 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor); > > > > * a mutex that is held elsewhere while calling synchronize_srcu() or > > > > * synchronize_srcu_expedited(). > > > > * > > > > - * The return value from srcu_read_lock() must be passed unaltered > > > > - * to the matching srcu_read_unlock(). Note that srcu_read_lock() and > > > > - * the matching srcu_read_unlock() must occur in the same context, for > > > > - * example, it is illegal to invoke srcu_read_unlock() in an irq handler > > > > - * if the matching srcu_read_lock() was invoked in process context. Or, > > > > - * for that matter to invoke srcu_read_unlock() from one task and the > > > > - * matching srcu_read_lock() from another. > > > > + * The return value from srcu_read_lock() is guaranteed to be > > > > + * non-negative. This value must be passed unaltered to the matching > > > > + * srcu_read_unlock(). Note that srcu_read_lock() and the matching > > > > + * srcu_read_unlock() must occur in the same context, for example, it is > > > > + * illegal to invoke srcu_read_unlock() in an irq handler if the matching > > > > + * srcu_read_lock() was invoked in process context. Or, for that matter to > > > > + * invoke srcu_read_unlock() from one task and the matching srcu_read_lock() > > > > + * from another. > > > > > > For uprobe work I'm using __srcu_read_lock() and __srcu_read_unlock(). > > > Presumably the same non-negative index will be returned/consumed there > > > as well, right? Can we add a blurb to that effect for them as well? > > > > Does the change shown below cover it? > > Yep, looks good, thank you! You might want to fix > s/srcu_read_unlock/__srcu_read_unlock/, while at it, but that's > orthogonal. As long as we are in the area... Please see below for the update. Thoughts? Thanx, Paul ------------------------------------------------------------------------ commit 4433b7d3785d8d2a700f5ed5ca234c64bc63180e Author: Paul E. McKenney <paulmck@kernel.org> Date: Mon Oct 21 15:09:39 2024 -0700 srcu: Guarantee non-negative return value from srcu_read_lock() For almost 20 years, the int return value from srcu_read_lock() has been always either zero or one. This commit therefore documents the fact that it will be non-negative, and does the same for the underlying __srcu_read_lock(). [ paulmck: Apply Andrii Nakryiko feedback. ] Signed-off-by: Paul E. McKenney <paulmck@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Acked-by: Andrii Nakryiko <andrii@kernel.org> diff --git a/include/linux/srcu.h b/include/linux/srcu.h index bab1dae3f69e6..512a8c54ba5ba 100644 --- a/include/linux/srcu.h +++ b/include/linux/srcu.h @@ -238,13 +238,14 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor); * a mutex that is held elsewhere while calling synchronize_srcu() or * synchronize_srcu_expedited(). * - * The return value from srcu_read_lock() must be passed unaltered - * to the matching srcu_read_unlock(). Note that srcu_read_lock() and - * the matching srcu_read_unlock() must occur in the same context, for - * example, it is illegal to invoke srcu_read_unlock() in an irq handler - * if the matching srcu_read_lock() was invoked in process context. Or, - * for that matter to invoke srcu_read_unlock() from one task and the - * matching srcu_read_lock() from another. + * The return value from srcu_read_lock() is guaranteed to be + * non-negative. This value must be passed unaltered to the matching + * srcu_read_unlock(). Note that srcu_read_lock() and the matching + * srcu_read_unlock() must occur in the same context, for example, it is + * illegal to invoke srcu_read_unlock() in an irq handler if the matching + * srcu_read_lock() was invoked in process context. Or, for that matter to + * invoke srcu_read_unlock() from one task and the matching srcu_read_lock() + * from another. */ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp) { diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index 07147efcb64d3..ae17c214e0de5 100644 --- a/kernel/rcu/srcutree.c +++ b/kernel/rcu/srcutree.c @@ -738,7 +738,8 @@ EXPORT_SYMBOL_GPL(srcu_check_read_flavor); /* * Counts the new reader in the appropriate per-CPU element of the * srcu_struct. - * Returns an index that must be passed to the matching srcu_read_unlock(). + * Returns a guaranteed non-negative index that must be passed to the + * matching __srcu_read_unlock(). */ int __srcu_read_lock(struct srcu_struct *ssp) {
On Mon, Oct 21, 2024 at 8:30 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > On Mon, Oct 21, 2024 at 07:01:02PM -0700, Andrii Nakryiko wrote: > > On Mon, Oct 21, 2024 at 5:21 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > > > > > On Mon, Oct 21, 2024 at 04:50:44PM -0700, Andrii Nakryiko wrote: > > > > On Mon, Oct 21, 2024 at 3:13 PM Paul E. McKenney <paulmck@kernel.org> wrote: > > > > > > > > > > For almost 20 years, the int return value from srcu_read_lock() has > > > > > been always either zero or one. This commit therefore documents the > > > > > fact that it will be non-negative. > > > > > > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > > > Cc: Peter Zijlstra <peterz@infradead.org> > > > > > Cc: Andrii Nakryiko <andrii@kernel.org > > > > > > > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > > > > > index bab1dae3f69e6..512a8c54ba5ba 100644 > > > > > --- a/include/linux/srcu.h > > > > > +++ b/include/linux/srcu.h > > > > > @@ -238,13 +238,14 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor); > > > > > * a mutex that is held elsewhere while calling synchronize_srcu() or > > > > > * synchronize_srcu_expedited(). > > > > > * > > > > > - * The return value from srcu_read_lock() must be passed unaltered > > > > > - * to the matching srcu_read_unlock(). Note that srcu_read_lock() and > > > > > - * the matching srcu_read_unlock() must occur in the same context, for > > > > > - * example, it is illegal to invoke srcu_read_unlock() in an irq handler > > > > > - * if the matching srcu_read_lock() was invoked in process context. Or, > > > > > - * for that matter to invoke srcu_read_unlock() from one task and the > > > > > - * matching srcu_read_lock() from another. > > > > > + * The return value from srcu_read_lock() is guaranteed to be > > > > > + * non-negative. This value must be passed unaltered to the matching > > > > > + * srcu_read_unlock(). Note that srcu_read_lock() and the matching > > > > > + * srcu_read_unlock() must occur in the same context, for example, it is > > > > > + * illegal to invoke srcu_read_unlock() in an irq handler if the matching > > > > > + * srcu_read_lock() was invoked in process context. Or, for that matter to > > > > > + * invoke srcu_read_unlock() from one task and the matching srcu_read_lock() > > > > > + * from another. > > > > > > > > For uprobe work I'm using __srcu_read_lock() and __srcu_read_unlock(). > > > > Presumably the same non-negative index will be returned/consumed there > > > > as well, right? Can we add a blurb to that effect for them as well? > > > > > > Does the change shown below cover it? > > > > Yep, looks good, thank you! You might want to fix > > s/srcu_read_unlock/__srcu_read_unlock/, while at it, but that's > > orthogonal. > > As long as we are in the area... Please see below for the update. > > Thoughts? > LGTM Acked-by: Andrii Nakryiko <andrii@kernel.org> > Thanx, Paul > > ------------------------------------------------------------------------ > > commit 4433b7d3785d8d2a700f5ed5ca234c64bc63180e > Author: Paul E. McKenney <paulmck@kernel.org> > Date: Mon Oct 21 15:09:39 2024 -0700 > > srcu: Guarantee non-negative return value from srcu_read_lock() > > For almost 20 years, the int return value from srcu_read_lock() has > been always either zero or one. This commit therefore documents the > fact that it will be non-negative, and does the same for the underlying > __srcu_read_lock(). > > [ paulmck: Apply Andrii Nakryiko feedback. ] > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > index bab1dae3f69e6..512a8c54ba5ba 100644 > --- a/include/linux/srcu.h > +++ b/include/linux/srcu.h > @@ -238,13 +238,14 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor); > * a mutex that is held elsewhere while calling synchronize_srcu() or > * synchronize_srcu_expedited(). > * > - * The return value from srcu_read_lock() must be passed unaltered > - * to the matching srcu_read_unlock(). Note that srcu_read_lock() and > - * the matching srcu_read_unlock() must occur in the same context, for > - * example, it is illegal to invoke srcu_read_unlock() in an irq handler > - * if the matching srcu_read_lock() was invoked in process context. Or, > - * for that matter to invoke srcu_read_unlock() from one task and the > - * matching srcu_read_lock() from another. > + * The return value from srcu_read_lock() is guaranteed to be > + * non-negative. This value must be passed unaltered to the matching > + * srcu_read_unlock(). Note that srcu_read_lock() and the matching > + * srcu_read_unlock() must occur in the same context, for example, it is > + * illegal to invoke srcu_read_unlock() in an irq handler if the matching > + * srcu_read_lock() was invoked in process context. Or, for that matter to > + * invoke srcu_read_unlock() from one task and the matching srcu_read_lock() > + * from another. > */ > static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp) > { > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 07147efcb64d3..ae17c214e0de5 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -738,7 +738,8 @@ EXPORT_SYMBOL_GPL(srcu_check_read_flavor); > /* > * Counts the new reader in the appropriate per-CPU element of the > * srcu_struct. > - * Returns an index that must be passed to the matching srcu_read_unlock(). > + * Returns a guaranteed non-negative index that must be passed to the > + * matching __srcu_read_unlock(). > */ > int __srcu_read_lock(struct srcu_struct *ssp) > {
On Mon, Oct 21, 2024 at 03:13:05PM -0700, Paul E. McKenney wrote: > For almost 20 years, the int return value from srcu_read_lock() has > been always either zero or one. This commit therefore documents the > fact that it will be non-negative. If it is always zero or one, wouldn't bool the better return value type?
On Mon, Oct 21, 2024 at 11:51:40PM -0700, Christoph Hellwig wrote: > On Mon, Oct 21, 2024 at 03:13:05PM -0700, Paul E. McKenney wrote: > > For almost 20 years, the int return value from srcu_read_lock() has > > been always either zero or one. This commit therefore documents the > > fact that it will be non-negative. > > If it is always zero or one, wouldn't bool the better return value > type? What is returned is an array index -- and SRCU is currently built using an array of size 2. Using larger arrays is conceivable (IIRC some versions of preemptible RCU used up to 4 or something). So while the values 0,1 are possible inside bool, that does not reflect the nature of the numbers, which is an array index. Mapping that onto bool would be slightly confusing (and limit possible future extention of using larger arrays for SRCU).
On Mon, Oct 21, 2024 at 08:30:43PM -0700, Paul E. McKenney wrote: > Thoughts? Thanks Paul! Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> > ------------------------------------------------------------------------ > > commit 4433b7d3785d8d2a700f5ed5ca234c64bc63180e > Author: Paul E. McKenney <paulmck@kernel.org> > Date: Mon Oct 21 15:09:39 2024 -0700 > > srcu: Guarantee non-negative return value from srcu_read_lock() > > For almost 20 years, the int return value from srcu_read_lock() has > been always either zero or one. This commit therefore documents the > fact that it will be non-negative, and does the same for the underlying > __srcu_read_lock(). > > [ paulmck: Apply Andrii Nakryiko feedback. ] > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > index bab1dae3f69e6..512a8c54ba5ba 100644 > --- a/include/linux/srcu.h > +++ b/include/linux/srcu.h > @@ -238,13 +238,14 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor); > * a mutex that is held elsewhere while calling synchronize_srcu() or > * synchronize_srcu_expedited(). > * > - * The return value from srcu_read_lock() must be passed unaltered > - * to the matching srcu_read_unlock(). Note that srcu_read_lock() and > - * the matching srcu_read_unlock() must occur in the same context, for > - * example, it is illegal to invoke srcu_read_unlock() in an irq handler > - * if the matching srcu_read_lock() was invoked in process context. Or, > - * for that matter to invoke srcu_read_unlock() from one task and the > - * matching srcu_read_lock() from another. > + * The return value from srcu_read_lock() is guaranteed to be > + * non-negative. This value must be passed unaltered to the matching > + * srcu_read_unlock(). Note that srcu_read_lock() and the matching > + * srcu_read_unlock() must occur in the same context, for example, it is > + * illegal to invoke srcu_read_unlock() in an irq handler if the matching > + * srcu_read_lock() was invoked in process context. Or, for that matter to > + * invoke srcu_read_unlock() from one task and the matching srcu_read_lock() > + * from another. > */ > static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp) > { > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 07147efcb64d3..ae17c214e0de5 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -738,7 +738,8 @@ EXPORT_SYMBOL_GPL(srcu_check_read_flavor); > /* > * Counts the new reader in the appropriate per-CPU element of the > * srcu_struct. > - * Returns an index that must be passed to the matching srcu_read_unlock(). > + * Returns a guaranteed non-negative index that must be passed to the > + * matching __srcu_read_unlock(). > */ > int __srcu_read_lock(struct srcu_struct *ssp) > {
On Tue, Oct 22, 2024 at 09:06:35AM +0200, Peter Zijlstra wrote: > What is returned is an array index -- and SRCU is currently built using > an array of size 2. Using larger arrays is conceivable (IIRC some > versions of preemptible RCU used up to 4 or something). > > So while the values 0,1 are possible inside bool, that does not reflect > the nature of the numbers, which is an array index. Mapping that onto > bool would be slightly confusing (and limit possible future extention of > using larger arrays for SRCU). Ok, make sense. Maybe add this to the comment if we're updating іt. But using an unsigned return value might still be useful.
On Tue, Oct 22, 2024 at 12:07:35AM -0700, Christoph Hellwig wrote: > On Tue, Oct 22, 2024 at 09:06:35AM +0200, Peter Zijlstra wrote: > > What is returned is an array index -- and SRCU is currently built using > > an array of size 2. Using larger arrays is conceivable (IIRC some > > versions of preemptible RCU used up to 4 or something). > > > > So while the values 0,1 are possible inside bool, that does not reflect > > the nature of the numbers, which is an array index. Mapping that onto > > bool would be slightly confusing (and limit possible future extention of > > using larger arrays for SRCU). > > Ok, make sense. Maybe add this to the comment if we're updating іt. > But using an unsigned return value might still be useful. Ah, well, the thing that got us here is that we (Andrii and me) wanted to use -1 as an 'invalid' value to indicate SRCU is not currently in use. So it all being int is really rather convenient :-)
On Tue, Oct 22, 2024 at 09:10:18AM +0200, Peter Zijlstra wrote: > Ah, well, the thing that got us here is that we (Andrii and me) wanted > to use -1 as an 'invalid' value to indicate SRCU is not currently in > use. > > So it all being int is really rather convenient :-) Then please document that use. Maybe even with a symolic name for -1 that clearly describes these uses.
On Tue, Oct 22, 2024 at 12:13:12AM -0700, Christoph Hellwig wrote: > On Tue, Oct 22, 2024 at 09:10:18AM +0200, Peter Zijlstra wrote: > > Ah, well, the thing that got us here is that we (Andrii and me) wanted > > to use -1 as an 'invalid' value to indicate SRCU is not currently in > > use. > > > > So it all being int is really rather convenient :-) > > Then please document that use. Maybe even with a symolic name for > -1 that clearly describes these uses. Would this work? #define SRCU_INVALID_INDEX -1 Whatever the name, maybe Peter and Andrii define this under #ifndef right now, and we get it into include/linux/srcu.h over time. Or is there a better way? Or name, for that matter. Thanx, Paul
On Tue, Oct 22, 2024 at 09:07:17AM +0200, Peter Zijlstra wrote: > On Mon, Oct 21, 2024 at 08:30:43PM -0700, Paul E. McKenney wrote: > > Thoughts? > > Thanks Paul! > > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> Thank you, Peter! I will apply this on my next rebase. Thanx, Paul > > ------------------------------------------------------------------------ > > > > commit 4433b7d3785d8d2a700f5ed5ca234c64bc63180e > > Author: Paul E. McKenney <paulmck@kernel.org> > > Date: Mon Oct 21 15:09:39 2024 -0700 > > > > srcu: Guarantee non-negative return value from srcu_read_lock() > > > > For almost 20 years, the int return value from srcu_read_lock() has > > been always either zero or one. This commit therefore documents the > > fact that it will be non-negative, and does the same for the underlying > > __srcu_read_lock(). > > > > [ paulmck: Apply Andrii Nakryiko feedback. ] > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > > index bab1dae3f69e6..512a8c54ba5ba 100644 > > --- a/include/linux/srcu.h > > +++ b/include/linux/srcu.h > > @@ -238,13 +238,14 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor); > > * a mutex that is held elsewhere while calling synchronize_srcu() or > > * synchronize_srcu_expedited(). > > * > > - * The return value from srcu_read_lock() must be passed unaltered > > - * to the matching srcu_read_unlock(). Note that srcu_read_lock() and > > - * the matching srcu_read_unlock() must occur in the same context, for > > - * example, it is illegal to invoke srcu_read_unlock() in an irq handler > > - * if the matching srcu_read_lock() was invoked in process context. Or, > > - * for that matter to invoke srcu_read_unlock() from one task and the > > - * matching srcu_read_lock() from another. > > + * The return value from srcu_read_lock() is guaranteed to be > > + * non-negative. This value must be passed unaltered to the matching > > + * srcu_read_unlock(). Note that srcu_read_lock() and the matching > > + * srcu_read_unlock() must occur in the same context, for example, it is > > + * illegal to invoke srcu_read_unlock() in an irq handler if the matching > > + * srcu_read_lock() was invoked in process context. Or, for that matter to > > + * invoke srcu_read_unlock() from one task and the matching srcu_read_lock() > > + * from another. > > */ > > static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp) > > { > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index 07147efcb64d3..ae17c214e0de5 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -738,7 +738,8 @@ EXPORT_SYMBOL_GPL(srcu_check_read_flavor); > > /* > > * Counts the new reader in the appropriate per-CPU element of the > > * srcu_struct. > > - * Returns an index that must be passed to the matching srcu_read_unlock(). > > + * Returns a guaranteed non-negative index that must be passed to the > > + * matching __srcu_read_unlock(). > > */ > > int __srcu_read_lock(struct srcu_struct *ssp) > > {
On Tue, Oct 22, 2024 at 7:26 AM Paul E. McKenney <paulmck@kernel.org> wrote: > > On Tue, Oct 22, 2024 at 12:13:12AM -0700, Christoph Hellwig wrote: > > On Tue, Oct 22, 2024 at 09:10:18AM +0200, Peter Zijlstra wrote: > > > Ah, well, the thing that got us here is that we (Andrii and me) wanted > > > to use -1 as an 'invalid' value to indicate SRCU is not currently in > > > use. > > > > > > So it all being int is really rather convenient :-) > > > > Then please document that use. Maybe even with a symolic name for > > -1 that clearly describes these uses. > > Would this work? > > #define SRCU_INVALID_INDEX -1 > But why? It's a nice property to have an int-returning API where valid values are only >= 0, so callers are free to use the entire negative range (not just -1) for whatever they need to store in case there is no srcu_idx value. Why are we complicating something that's simple and straightforward? It's similar with any fd- and id- returning API. Marking it as u16 would be fine, but unusual (and probably suboptimal due to common u16 to int conversion). Marking it as unsigned int would be bad, because it implies that all 32 bits can be used for valid value, making it impossible to use <0 for something else. Just documenting that valid int values are always >= is good and convenient, why not sticking to just that? > Whatever the name, maybe Peter and Andrii define this under #ifndef > right now, and we get it into include/linux/srcu.h over time. > > Or is there a better way? Or name, for that matter. > > Thanx, Paul
diff --git a/include/linux/srcu.h b/include/linux/srcu.h index bab1dae3f69e6..512a8c54ba5ba 100644 --- a/include/linux/srcu.h +++ b/include/linux/srcu.h @@ -238,13 +238,14 @@ void srcu_check_read_flavor(struct srcu_struct *ssp, int read_flavor); * a mutex that is held elsewhere while calling synchronize_srcu() or * synchronize_srcu_expedited(). * - * The return value from srcu_read_lock() must be passed unaltered - * to the matching srcu_read_unlock(). Note that srcu_read_lock() and - * the matching srcu_read_unlock() must occur in the same context, for - * example, it is illegal to invoke srcu_read_unlock() in an irq handler - * if the matching srcu_read_lock() was invoked in process context. Or, - * for that matter to invoke srcu_read_unlock() from one task and the - * matching srcu_read_lock() from another. + * The return value from srcu_read_lock() is guaranteed to be + * non-negative. This value must be passed unaltered to the matching + * srcu_read_unlock(). Note that srcu_read_lock() and the matching + * srcu_read_unlock() must occur in the same context, for example, it is + * illegal to invoke srcu_read_unlock() in an irq handler if the matching + * srcu_read_lock() was invoked in process context. Or, for that matter to + * invoke srcu_read_unlock() from one task and the matching srcu_read_lock() + * from another. */ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp) {
For almost 20 years, the int return value from srcu_read_lock() has been always either zero or one. This commit therefore documents the fact that it will be non-negative. Signed-off-by: Paul E. McKenney <paulmck@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Andrii Nakryiko <andrii@kernel.org