| Message ID | 20230717180317.1097590-2-paulmck@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Miscellaneous fixes for v6.6 | expand |
Hi Paul, On 7/17/23 14:03, Paul E. McKenney wrote: > Make it clear that this function always returns either true or false > without other planned failure modes. > > Reported-by: Masami Hiramatsu <mhiramat@kernel.org> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > --- > kernel/rcu/tree.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > index 1449cb69a0e0..fae9b4e29c93 100644 > --- a/kernel/rcu/tree.c > +++ b/kernel/rcu/tree.c > @@ -679,10 +679,14 @@ static void rcu_disable_urgency_upon_qs(struct rcu_data *rdp) > /** > * rcu_is_watching - see if RCU thinks that the current CPU is not idle Would it be better to modify the 'not idle' to 'not idle from an RCU viewpoint'? This matches the comments in ct_nmi_enter() as well. > * > - * Return true if RCU is watching the running CPU, which means that this > - * CPU can safely enter RCU read-side critical sections. In other words, > - * if the current CPU is not in its idle loop or is in an interrupt or > - * NMI handler, return true. > + * Return @true if RCU is watching the running CPU and @false otherwise. > + * An @true return means that this CPU can safely enter RCU read-side > + * critical sections. > + * > + * More specifically, if the current CPU is not deep within its idle > + * loop, return @true. Note that rcu_is_watching() will return @true if > + * invoked from an interrupt or NMI handler, even if that interrupt or > + * NMI interrupted the CPU while it was deep within its idle loop. But it is more than the idle loop, for ex. NOHZ_FULL CPUs with single task running could be idle from RCU's viewpoint? Could that be clarified more? thanks, - Joel > * > * Make notrace because it can be called by the internal functions of > * ftrace, and making this notrace removes unnecessary recursion calls.
On Tue, Jul 18, 2023 at 08:52:30AM -0400, Joel Fernandes wrote: > Hi Paul, > > On 7/17/23 14:03, Paul E. McKenney wrote: > > Make it clear that this function always returns either true or false > > without other planned failure modes. > > > > Reported-by: Masami Hiramatsu <mhiramat@kernel.org> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > --- > > kernel/rcu/tree.c | 12 ++++++++---- > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > index 1449cb69a0e0..fae9b4e29c93 100644 > > --- a/kernel/rcu/tree.c > > +++ b/kernel/rcu/tree.c > > @@ -679,10 +679,14 @@ static void rcu_disable_urgency_upon_qs(struct rcu_data *rdp) > > /** > > * rcu_is_watching - see if RCU thinks that the current CPU is not idle > > Would it be better to modify the 'not idle' to 'not idle from an RCU > viewpoint'? This matches the comments in ct_nmi_enter() as well. We have the "if RCU thinks that" earlier. But maybe something like this? * rcu_is_watching - RCU read-side critical sections permitted on current CPU? > > * > > - * Return true if RCU is watching the running CPU, which means that this > > - * CPU can safely enter RCU read-side critical sections. In other words, > > - * if the current CPU is not in its idle loop or is in an interrupt or > > - * NMI handler, return true. > > + * Return @true if RCU is watching the running CPU and @false otherwise. > > + * An @true return means that this CPU can safely enter RCU read-side > > + * critical sections. > > + * > > + * More specifically, if the current CPU is not deep within its idle > > + * loop, return @true. Note that rcu_is_watching() will return @true if > > + * invoked from an interrupt or NMI handler, even if that interrupt or > > + * NMI interrupted the CPU while it was deep within its idle loop. > > But it is more than the idle loop, for ex. NOHZ_FULL CPUs with single task > running could be idle from RCU's viewpoint? Could that be clarified more? Perhaps something like this? * Although calls to rcu_is_watching() from most parts of the kernel * will return @true, there are important exceptions. For example, if the * current CPU is deep within its idle loop, in kernel entry/exit code, * or offline, rcu_is_watching() will return @false. (Where nohz_full CPUs are covered by kernel entry/exit code.) Thanx, Paul > thanks, > > - Joel > > > > * > > * Make notrace because it can be called by the internal functions of > > * ftrace, and making this notrace removes unnecessary recursion calls. >
Hi Paul, On 7/18/23 14:12, Paul E. McKenney wrote: > On Tue, Jul 18, 2023 at 08:52:30AM -0400, Joel Fernandes wrote: >> Hi Paul, >> >> On 7/17/23 14:03, Paul E. McKenney wrote: >>> Make it clear that this function always returns either true or false >>> without other planned failure modes. >>> >>> Reported-by: Masami Hiramatsu <mhiramat@kernel.org> >>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org> >>> --- >>> kernel/rcu/tree.c | 12 ++++++++---- >>> 1 file changed, 8 insertions(+), 4 deletions(-) >>> >>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c >>> index 1449cb69a0e0..fae9b4e29c93 100644 >>> --- a/kernel/rcu/tree.c >>> +++ b/kernel/rcu/tree.c >>> @@ -679,10 +679,14 @@ static void rcu_disable_urgency_upon_qs(struct rcu_data *rdp) >>> /** >>> * rcu_is_watching - see if RCU thinks that the current CPU is not idle >> >> Would it be better to modify the 'not idle' to 'not idle from an RCU >> viewpoint'? This matches the comments in ct_nmi_enter() as well. > > We have the "if RCU thinks that" earlier. > > But maybe something like this? > > * rcu_is_watching - RCU read-side critical sections permitted on current CPU? > Yes, that's better. >>> * >>> - * Return true if RCU is watching the running CPU, which means that this >>> - * CPU can safely enter RCU read-side critical sections. In other words, >>> - * if the current CPU is not in its idle loop or is in an interrupt or >>> - * NMI handler, return true. >>> + * Return @true if RCU is watching the running CPU and @false otherwise. >>> + * An @true return means that this CPU can safely enter RCU read-side >>> + * critical sections. >>> + * >>> + * More specifically, if the current CPU is not deep within its idle >>> + * loop, return @true. Note that rcu_is_watching() will return @true if >>> + * invoked from an interrupt or NMI handler, even if that interrupt or >>> + * NMI interrupted the CPU while it was deep within its idle loop. >> >> But it is more than the idle loop, for ex. NOHZ_FULL CPUs with single task >> running could be idle from RCU's viewpoint? Could that be clarified more? > > Perhaps something like this? > > * Although calls to rcu_is_watching() from most parts of the kernel > * will return @true, there are important exceptions. For example, if the > * current CPU is deep within its idle loop, in kernel entry/exit code, > * or offline, rcu_is_watching() will return @false. > > (Where nohz_full CPUs are covered by kernel entry/exit code.) To me, "kernel exit" does not immediately make the nohz_full CPU case obvious. But yes, your suggestion is an improvement so we can go with that. :) Also because we agree on the changes, for next revision: Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> thanks, - Joel
On Tue, Jul 18, 2023 at 09:56:38PM -0400, Joel Fernandes wrote: > Hi Paul, > > On 7/18/23 14:12, Paul E. McKenney wrote: > > On Tue, Jul 18, 2023 at 08:52:30AM -0400, Joel Fernandes wrote: > > > Hi Paul, > > > > > > On 7/17/23 14:03, Paul E. McKenney wrote: > > > > Make it clear that this function always returns either true or false > > > > without other planned failure modes. > > > > > > > > Reported-by: Masami Hiramatsu <mhiramat@kernel.org> > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > > --- > > > > kernel/rcu/tree.c | 12 ++++++++---- > > > > 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c > > > > index 1449cb69a0e0..fae9b4e29c93 100644 > > > > --- a/kernel/rcu/tree.c > > > > +++ b/kernel/rcu/tree.c > > > > @@ -679,10 +679,14 @@ static void rcu_disable_urgency_upon_qs(struct rcu_data *rdp) > > > > /** > > > > * rcu_is_watching - see if RCU thinks that the current CPU is not idle > > > > > > Would it be better to modify the 'not idle' to 'not idle from an RCU > > > viewpoint'? This matches the comments in ct_nmi_enter() as well. > > > > We have the "if RCU thinks that" earlier. > > > > But maybe something like this? > > > > * rcu_is_watching - RCU read-side critical sections permitted on current CPU? > > > > Yes, that's better. > > > > > * > > > > - * Return true if RCU is watching the running CPU, which means that this > > > > - * CPU can safely enter RCU read-side critical sections. In other words, > > > > - * if the current CPU is not in its idle loop or is in an interrupt or > > > > - * NMI handler, return true. > > > > + * Return @true if RCU is watching the running CPU and @false otherwise. > > > > + * An @true return means that this CPU can safely enter RCU read-side > > > > + * critical sections. > > > > + * > > > > + * More specifically, if the current CPU is not deep within its idle > > > > + * loop, return @true. Note that rcu_is_watching() will return @true if > > > > + * invoked from an interrupt or NMI handler, even if that interrupt or > > > > + * NMI interrupted the CPU while it was deep within its idle loop. > > > > > > But it is more than the idle loop, for ex. NOHZ_FULL CPUs with single task > > > running could be idle from RCU's viewpoint? Could that be clarified more? > > > > Perhaps something like this? > > > > * Although calls to rcu_is_watching() from most parts of the kernel > > * will return @true, there are important exceptions. For example, if the > > * current CPU is deep within its idle loop, in kernel entry/exit code, > > * or offline, rcu_is_watching() will return @false. > > > > (Where nohz_full CPUs are covered by kernel entry/exit code.) > > To me, "kernel exit" does not immediately make the nohz_full CPU case > obvious. But yes, your suggestion is an improvement so we can go with that. > :) > > Also because we agree on the changes, for next revision: > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> Very good, thank you! The update should appear shortly. For some definition of "shortly". ;-) Thanx, Paul
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 1449cb69a0e0..fae9b4e29c93 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -679,10 +679,14 @@ static void rcu_disable_urgency_upon_qs(struct rcu_data *rdp) /** * rcu_is_watching - see if RCU thinks that the current CPU is not idle * - * Return true if RCU is watching the running CPU, which means that this - * CPU can safely enter RCU read-side critical sections. In other words, - * if the current CPU is not in its idle loop or is in an interrupt or - * NMI handler, return true. + * Return @true if RCU is watching the running CPU and @false otherwise. + * An @true return means that this CPU can safely enter RCU read-side + * critical sections. + * + * More specifically, if the current CPU is not deep within its idle + * loop, return @true. Note that rcu_is_watching() will return @true if + * invoked from an interrupt or NMI handler, even if that interrupt or + * NMI interrupted the CPU while it was deep within its idle loop. * * Make notrace because it can be called by the internal functions of * ftrace, and making this notrace removes unnecessary recursion calls.
Make it clear that this function always returns either true or false without other planned failure modes. Reported-by: Masami Hiramatsu <mhiramat@kernel.org> Signed-off-by: Paul E. McKenney <paulmck@kernel.org> --- kernel/rcu/tree.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)