| 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
On Tue, Oct 22, 2024 at 10:29:13AM -0700, Andrii Nakryiko wrote: > > > > Would this work? > > > > #define SRCU_INVALID_INDEX -1 > > > > But why? Becaue it very clearly documents what is going on. >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. Well, if you have a concrete use case for that we can probably live with it, but I'd rather have that use case extremely well documented, as it will be very puzzling to the reader.
On Oct 22, 2024, at 22:26, 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 Is there any similar guarantee of the return value of get_state_synchronize_rcu or start_poll_synchronize_rcu, like invalid value? > > 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 11:40 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Tue, Oct 22, 2024 at 10:29:13AM -0700, Andrii Nakryiko wrote: > > > > > > Would this work? > > > > > > #define SRCU_INVALID_INDEX -1 > > > > > > > But why? > > Becaue it very clearly documents what is going on. So does saying "returned value is going to be non-negative, always". It's not some weird and unusual thing in C by any means. > > >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. > > Well, if you have a concrete use case for that we can probably live > with it, but I'd rather have that use case extremely well documented, > as it will be very puzzling to the reader. > See [0] for what Peter is proposing. Note hprobe_init() and its use of `srcu_idx < 0` as a mark that there is no SRCU "session" associated with the uprobe. Now, if we say that SRCU_INVALID_INDEX is the only invalid value, it leaves a question: what to do with other negative srcu_idx values? Are they valid? Should I now WARN() on "unknown invalid" values? Why all these complications? I'd rather just not have a unified hprobe_init() at that point, TBH. But anyways, taking a step back. Take idr_alloc()/idr_alloc_cyclic() APIs as an example. They return int, but valid IDs are documented to be positive. This leaves users of this API free to use int to store ID in their data structures, knowing that <= 0 is "no ID", and if necessary, they can have multiple possible "no ID" situations. Same principle here. Why prescribing a randomly picked -1 as the only "valid" invalid value, if anything negative is clearly impossible. [0] https://lore.kernel.org/linux-trace-kernel/20241018101647.GA36494@noisy.programming.kicks-ass.net/
On Wed, Oct 23, 2024 at 02:58:07PM +0800, Alan Huang wrote: > On Oct 22, 2024, at 22:26, 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 > > Is there any similar guarantee of the return value of get_state_synchronize_rcu > or start_poll_synchronize_rcu, like invalid value? Yes, there is a get_completed_synchronize_rcu() function that returns a value that causes poll_state_synchronize_rcu() to always return true. There is also a get_completed_synchronize_rcu_full() function that returns a value that causes poll_state_synchronize_rcu_full() to always return true. There has been some discussion of another set of values that cause poll_state_synchronize_rcu() and poll_state_synchronize_rcu_full() to always return false, but there is not yet a use case for this. Easy to provide if required, but why further explode the RCU API unless it really is required? Thanx, Paul > > 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 Oct 24, 2024, at 00:34, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Oct 22, 2024 at 11:40 PM Christoph Hellwig <hch@infradead.org> wrote: >> >> On Tue, Oct 22, 2024 at 10:29:13AM -0700, Andrii Nakryiko wrote: >>>> >>>> Would this work? >>>> >>>> #define SRCU_INVALID_INDEX -1 >>>> >>> >>> But why? >> >> Becaue it very clearly documents what is going on. > > So does saying "returned value is going to be non-negative, always". > It's not some weird and unusual thing in C by any means. > >> >>> 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. >> >> Well, if you have a concrete use case for that we can probably live >> with it, but I'd rather have that use case extremely well documented, >> as it will be very puzzling to the reader. >> > > See [0] for what Peter is proposing. Note hprobe_init() and its use of > `srcu_idx < 0` as a mark that there is no SRCU "session" associated > with the uprobe. Now, if we say that SRCU_INVALID_INDEX is the only > invalid value, it leaves a question: what to do with other negative > srcu_idx values? Are they valid? Should I now WARN() on "unknown > invalid" values? Why all these complications? I'd rather just not have > a unified hprobe_init() at that point, TBH. > > But anyways, taking a step back. Take idr_alloc()/idr_alloc_cyclic() > APIs as an example. They return int, but valid IDs are documented to > be positive. This leaves users of this API free to use int to store ID > in their data structures, knowing that <= 0 is "no ID", and if > necessary, they can have multiple possible "no ID" situations. > > Same principle here. Why prescribing a randomly picked -1 as the only > "valid" invalid value, if anything negative is clearly impossible. > A IS_INVALID_SRCU_INDEX macro would suffice, you need the condition anyway. > > [0] https://lore.kernel.org/linux-trace-kernel/20241018101647.GA36494@noisy.programming.kicks-ass.net/
On Oct 24, 2024, at 00:40, Paul E. McKenney <paulmck@kernel.org> wrote: > > On Wed, Oct 23, 2024 at 02:58:07PM +0800, Alan Huang wrote: >> On Oct 22, 2024, at 22:26, 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 >> >> Is there any similar guarantee of the return value of get_state_synchronize_rcu >> or start_poll_synchronize_rcu, like invalid value? > > Yes, there is a get_completed_synchronize_rcu() function that returns a > value that causes poll_state_synchronize_rcu() to always return true. > There is also a get_completed_synchronize_rcu_full() function that > returns a value that causes poll_state_synchronize_rcu_full() to always > return true. This is exactly the API I was searching for, didn’t read the doc thoroughly : ) Thanks! > > There has been some discussion of another set of values that cause > poll_state_synchronize_rcu() and poll_state_synchronize_rcu_full() to > always return false, but there is not yet a use case for this. Easy to > provide if required, but why further explode the RCU API unless it really > is required? > > Thanx, Paul > >>> 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 Wed, Oct 23, 2024 at 9:46 AM Alan Huang <mmpgouride@gmail.com> wrote: > > On Oct 24, 2024, at 00:34, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Oct 22, 2024 at 11:40 PM Christoph Hellwig <hch@infradead.org> wrote: > >> > >> On Tue, Oct 22, 2024 at 10:29:13AM -0700, Andrii Nakryiko wrote: > >>>> > >>>> Would this work? > >>>> > >>>> #define SRCU_INVALID_INDEX -1 > >>>> > >>> > >>> But why? > >> > >> Becaue it very clearly documents what is going on. > > > > So does saying "returned value is going to be non-negative, always". > > It's not some weird and unusual thing in C by any means. > > > >> > >>> 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. > >> > >> Well, if you have a concrete use case for that we can probably live > >> with it, but I'd rather have that use case extremely well documented, > >> as it will be very puzzling to the reader. > >> > > > > See [0] for what Peter is proposing. Note hprobe_init() and its use of > > `srcu_idx < 0` as a mark that there is no SRCU "session" associated > > with the uprobe. Now, if we say that SRCU_INVALID_INDEX is the only > > invalid value, it leaves a question: what to do with other negative > > srcu_idx values? Are they valid? Should I now WARN() on "unknown > > invalid" values? Why all these complications? I'd rather just not have > > a unified hprobe_init() at that point, TBH. > > > > But anyways, taking a step back. Take idr_alloc()/idr_alloc_cyclic() > > APIs as an example. They return int, but valid IDs are documented to > > be positive. This leaves users of this API free to use int to store ID > > in their data structures, knowing that <= 0 is "no ID", and if > > necessary, they can have multiple possible "no ID" situations. > > > > Same principle here. Why prescribing a randomly picked -1 as the only > > "valid" invalid value, if anything negative is clearly impossible. > > > > A IS_INVALID_SRCU_INDEX macro would suffice, you need the condition anyway. That condition is `if (srcu_idx < 0)`, no need for ugly macros that obscure things unnecessarily. > > > > > [0] https://lore.kernel.org/linux-trace-kernel/20241018101647.GA36494@noisy.programming.kicks-ass.net/ > >
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