| Message ID | 20240430091740.1826862-19-vschneid@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | context_tracking, rcu: Spring cleaning renaming | expand |
Le Tue, Apr 30, 2024 at 11:17:22AM +0200, Valentin Schneider a écrit : > The context_tracking.state RCU_DYNTICKS subvariable has been renamed to > RCU_WATCHING, the dynticks prefix can go. > > Furthermore, the "in_eqs_since" part confuses me, as IIUC this only checks > for a change in watching/eqs state, not that RCU transitionned *into* a > EQS after the snapshot was taken. > > e.g. if > snap = 0b1000 (EQS) > and the following rcu_watching_snap(CPU) is: > 0b1100 (watching) > then > rcu_watching_in_eqs_since(rdp, snap) -> true > > but because RCU was already in EQS at the time of the > snap - it hasn't entered EQS "since" the snap was taken. > > Update the name to reflect that we're only looking at watching/EQS > transitions, not specifically transitions into EQS. Indeed in practice the function only checks a change. But semantically it really checks a trip to eqs because this function is only ever called after a failing call to rcu_dynticks_in_eqs(). So not sure about that one rename. Paul?
On Tue, May 07, 2024 at 03:48:18PM +0200, Frederic Weisbecker wrote: > Le Tue, Apr 30, 2024 at 11:17:22AM +0200, Valentin Schneider a écrit : > > The context_tracking.state RCU_DYNTICKS subvariable has been renamed to > > RCU_WATCHING, the dynticks prefix can go. > > > > Furthermore, the "in_eqs_since" part confuses me, as IIUC this only checks > > for a change in watching/eqs state, not that RCU transitionned *into* a > > EQS after the snapshot was taken. > > > > e.g. if > > snap = 0b1000 (EQS) > > and the following rcu_watching_snap(CPU) is: > > 0b1100 (watching) > > then > > rcu_watching_in_eqs_since(rdp, snap) -> true > > > > but because RCU was already in EQS at the time of the > > snap - it hasn't entered EQS "since" the snap was taken. > > > > Update the name to reflect that we're only looking at watching/EQS > > transitions, not specifically transitions into EQS. > > Indeed in practice the function only checks a change. But semantically it really > checks a trip to eqs because this function is only ever called after a failing > call to rcu_dynticks_in_eqs(). > > So not sure about that one rename. Paul? As you say, Valentin is technically correct. Me, I am having a hard time getting too excited one way or the other. ;-) I suggest thinking in terms of rate-bounding the change. If you do change it, don't change it again for a few years. Either way, should comments be changed or added? Of course, the scientific way to evaluate this is to whose a couple dozen people the old code and a couple dozen other people the new code, and see if one group or the other has statistically significantly lower levels of confusion. I don't see how this is feasible, but it is the (painfully) correct way. On the other hand, it would have the beneficial side effect of getting more people exposed to Linux-kernel-RCU internals. Unfortunately, it might also have the additional side effect of making them (more) annoyed at RCU. ;-) Thanx, Paul
Le Tue, May 07, 2024 at 10:14:08AM -0700, Paul E. McKenney a écrit : > On Tue, May 07, 2024 at 03:48:18PM +0200, Frederic Weisbecker wrote: > > Le Tue, Apr 30, 2024 at 11:17:22AM +0200, Valentin Schneider a écrit : > > > The context_tracking.state RCU_DYNTICKS subvariable has been renamed to > > > RCU_WATCHING, the dynticks prefix can go. > > > > > > Furthermore, the "in_eqs_since" part confuses me, as IIUC this only checks > > > for a change in watching/eqs state, not that RCU transitionned *into* a > > > EQS after the snapshot was taken. > > > > > > e.g. if > > > snap = 0b1000 (EQS) > > > and the following rcu_watching_snap(CPU) is: > > > 0b1100 (watching) > > > then > > > rcu_watching_in_eqs_since(rdp, snap) -> true > > > > > > but because RCU was already in EQS at the time of the > > > snap - it hasn't entered EQS "since" the snap was taken. > > > > > > Update the name to reflect that we're only looking at watching/EQS > > > transitions, not specifically transitions into EQS. > > > > Indeed in practice the function only checks a change. But semantically it really > > checks a trip to eqs because this function is only ever called after a failing > > call to rcu_dynticks_in_eqs(). > > > > So not sure about that one rename. Paul? > > As you say, Valentin is technically correct. Me, I am having a hard > time getting too excited one way or the other. ;-) > > I suggest thinking in terms of rate-bounding the change. If you do > change it, don't change it again for a few years. Makes sense! > > Either way, should comments be changed or added? > > Of course, the scientific way to evaluate this is to whose a couple > dozen people the old code and a couple dozen other people the new code, > and see if one group or the other has statistically significantly lower > levels of confusion. I don't see how this is feasible, but it is the > (painfully) correct way. On the other hand, it would have the beneficial > side effect of getting more people exposed to Linux-kernel-RCU internals. > Unfortunately, it might also have the additional side effect of making > them (more) annoyed at RCU. ;-) Sounds good! I divided myself in two blank RCU subjects for a double blind study and locked those people up overnight with a paper containing both proposals. I opened the door five minutes ago and they both elected by mutual agreement rcu_watching_changed_since()! Also they are thirsty. Congratulations Valentin! :-) > Thanx, Paul
On 08/05/24 12:59, Frederic Weisbecker wrote: > Le Tue, May 07, 2024 at 10:14:08AM -0700, Paul E. McKenney a écrit : >> On Tue, May 07, 2024 at 03:48:18PM +0200, Frederic Weisbecker wrote: >> > Indeed in practice the function only checks a change. But semantically it really >> > checks a trip to eqs because this function is only ever called after a failing >> > call to rcu_dynticks_in_eqs(). >> > >> > So not sure about that one rename. Paul? >> >> As you say, Valentin is technically correct. Me, I am having a hard >> time getting too excited one way or the other. ;-) >> >> I suggest thinking in terms of rate-bounding the change. If you do >> change it, don't change it again for a few years. > > Makes sense! > >> >> Either way, should comments be changed or added? >> >> Of course, the scientific way to evaluate this is to whose a couple >> dozen people the old code and a couple dozen other people the new code, >> and see if one group or the other has statistically significantly lower >> levels of confusion. I don't see how this is feasible, but it is the >> (painfully) correct way. On the other hand, it would have the beneficial >> side effect of getting more people exposed to Linux-kernel-RCU internals. >> Unfortunately, it might also have the additional side effect of making >> them (more) annoyed at RCU. ;-) > > Sounds good! > > I divided myself in two blank RCU subjects for a double blind study > and locked those people up overnight with a paper containing both proposals. > > I opened the door five minutes ago and they both elected by mutual agreement > rcu_watching_changed_since()! Also they are thirsty. > > Congratulations Valentin! :-) :-) Now, not that I like wasting everyone's time, but... I hadn't taken a step back to realize the calling context implied this would always be used to check an entry into EQS, per the waiting loop structures. With this in mind, how about the following? /** * rcu_watching_stopped_since() - Has RCU stopped watching a given CPU since * the specified @snap? * * @rdp: The rcu_data corresponding to the CPU for which to check EQS. * @snap: rcu_watching snapshot taken when the CPU wasn't in an EQS. * * Returns true if the CPU corresponding to @rcu_data has spent some time in an * extended quiescent state since @snap. Note that this doesn't check if it * /still/ is in an EQS, just that it went through one since @snap. * * This is meant to be used in a loop waiting for a CPU to go through an EQS. */ static bool rcu_watching_stopped_since(struct rcu_data *rdp, int snap) { if (WARN_ON_ONCE(rcu_watching_in_eqs(snap))) return true; return snap != rcu_dynticks_snap(rdp->cpu); }
Le Mon, May 13, 2024 at 08:40:09PM +0200, Valentin Schneider a écrit : > On 08/05/24 12:59, Frederic Weisbecker wrote: > > Le Tue, May 07, 2024 at 10:14:08AM -0700, Paul E. McKenney a écrit : > >> On Tue, May 07, 2024 at 03:48:18PM +0200, Frederic Weisbecker wrote: > >> > Indeed in practice the function only checks a change. But semantically it really > >> > checks a trip to eqs because this function is only ever called after a failing > >> > call to rcu_dynticks_in_eqs(). > >> > > >> > So not sure about that one rename. Paul? > >> > >> As you say, Valentin is technically correct. Me, I am having a hard > >> time getting too excited one way or the other. ;-) > >> > >> I suggest thinking in terms of rate-bounding the change. If you do > >> change it, don't change it again for a few years. > > > > Makes sense! > > > >> > >> Either way, should comments be changed or added? > >> > >> Of course, the scientific way to evaluate this is to whose a couple > >> dozen people the old code and a couple dozen other people the new code, > >> and see if one group or the other has statistically significantly lower > >> levels of confusion. I don't see how this is feasible, but it is the > >> (painfully) correct way. On the other hand, it would have the beneficial > >> side effect of getting more people exposed to Linux-kernel-RCU internals. > >> Unfortunately, it might also have the additional side effect of making > >> them (more) annoyed at RCU. ;-) > > > > Sounds good! > > > > I divided myself in two blank RCU subjects for a double blind study > > and locked those people up overnight with a paper containing both proposals. > > > > I opened the door five minutes ago and they both elected by mutual agreement > > rcu_watching_changed_since()! Also they are thirsty. > > > > Congratulations Valentin! :-) > > :-) > > Now, not that I like wasting everyone's time, but... I hadn't taken a step > back to realize the calling context implied this would always be used to > check an entry into EQS, per the waiting loop structures. With this in > mind, how about the following? > > > /** > * rcu_watching_stopped_since() - Has RCU stopped watching a given CPU since > * the specified @snap? > * > * @rdp: The rcu_data corresponding to the CPU for which to check EQS. > * @snap: rcu_watching snapshot taken when the CPU wasn't in an EQS. > * > * Returns true if the CPU corresponding to @rcu_data has spent some time in an @rdp > * extended quiescent state since @snap. Note that this doesn't check if it > * /still/ is in an EQS, just that it went through one since @snap. > * > * This is meant to be used in a loop waiting for a CPU to go through an EQS. > */ > static bool rcu_watching_stopped_since(struct rcu_data *rdp, int snap) > { > if (WARN_ON_ONCE(rcu_watching_in_eqs(snap))) > return true; > > return snap != rcu_dynticks_snap(rdp->cpu); > } > > Yep, looks good to me! Thanks.
diff --git a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst index e8ef12ca1e9da..0533814a1f69a 100644 --- a/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst +++ b/Documentation/RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst @@ -150,7 +150,7 @@ This case is handled by calls to the strongly ordered is invoked within ``rcu_dynticks_eqs_enter()`` at idle-entry time and within ``rcu_dynticks_eqs_exit()`` at idle-exit time. The grace-period kthread invokes ``rcu_watching_snap()`` and -``rcu_dynticks_in_eqs_since()`` (both of which invoke +``rcu_watching_changed_since()`` (both of which invoke an ``atomic_add_return()`` of zero) to detect idle CPUs. +-----------------------------------------------------------------------+ diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index d772755ccd564..ffcde8203c04f 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -320,7 +320,7 @@ static bool rcu_watching_in_eqs(int snap) * structure has spent some time in an extended quiescent state since * rcu_watching_snap() returned the specified snapshot. */ -static bool rcu_dynticks_in_eqs_since(struct rcu_data *rdp, int snap) +static bool rcu_watching_changed_since(struct rcu_data *rdp, int snap) { return snap != rcu_watching_snap(rdp->cpu); } @@ -803,7 +803,7 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp) * read-side critical section that started before the beginning * of the current RCU grace period. */ - if (rcu_dynticks_in_eqs_since(rdp, rdp->dynticks_snap)) { + if (rcu_watching_changed_since(rdp, rdp->dynticks_snap)) { trace_rcu_fqs(rcu_state.name, rdp->gp_seq, rdp->cpu, TPS("dti")); rcu_gpnum_ovf(rnp, rdp); return 1; diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h index 68dea1427c8bd..4046fae99517e 100644 --- a/kernel/rcu/tree_exp.h +++ b/kernel/rcu/tree_exp.h @@ -381,7 +381,7 @@ static void __sync_rcu_exp_select_node_cpus(struct rcu_exp_work *rewp) unsigned long mask = rdp->grpmask; retry_ipi: - if (rcu_dynticks_in_eqs_since(rdp, rdp->exp_dynticks_snap)) { + if (rcu_watching_changed_since(rdp, rdp->exp_dynticks_snap)) { mask_ofl_test |= mask; continue; }
The context_tracking.state RCU_DYNTICKS subvariable has been renamed to RCU_WATCHING, the dynticks prefix can go. Furthermore, the "in_eqs_since" part confuses me, as IIUC this only checks for a change in watching/eqs state, not that RCU transitionned *into* a EQS after the snapshot was taken. e.g. if snap = 0b1000 (EQS) and the following rcu_watching_snap(CPU) is: 0b1100 (watching) then rcu_watching_in_eqs_since(rdp, snap) -> true but because RCU was already in EQS at the time of the snap - it hasn't entered EQS "since" the snap was taken. Update the name to reflect that we're only looking at watching/EQS transitions, not specifically transitions into EQS. Signed-off-by: Valentin Schneider <vschneid@redhat.com> --- .../RCU/Design/Memory-Ordering/Tree-RCU-Memory-Ordering.rst | 2 +- kernel/rcu/tree.c | 4 ++-- kernel/rcu/tree_exp.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)