* [PATCH] rcu: update: Check rcu_bh_lock_map state in rcu_read_lock_bh_held @ 2021-06-22 12:05 Neeraj Upadhyay 2021-06-22 17:58 ` Paul E. McKenney 0 siblings, 1 reply; 6+ messages in thread From: Neeraj Upadhyay @ 2021-06-22 12:05 UTC (permalink / raw) To: paulmck, josh, rostedt, mathieu.desnoyers, jiangshanlai, joel Cc: rcu, linux-kernel, urezki, frederic, Neeraj Upadhyay In addition to irq and softirq state, check rcu_bh_lock_map state, to decide whether RCU bh lock is held. Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org> --- kernel/rcu/update.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c index c21b38c..d416f1c 100644 --- a/kernel/rcu/update.c +++ b/kernel/rcu/update.c @@ -333,7 +333,7 @@ int rcu_read_lock_bh_held(void) if (rcu_read_lock_held_common(&ret)) return ret; - return in_softirq() || irqs_disabled(); + return lock_is_held(&rcu_bh_lock_map) || in_softirq() || irqs_disabled(); } EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held); -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] rcu: update: Check rcu_bh_lock_map state in rcu_read_lock_bh_held 2021-06-22 12:05 [PATCH] rcu: update: Check rcu_bh_lock_map state in rcu_read_lock_bh_held Neeraj Upadhyay @ 2021-06-22 17:58 ` Paul E. McKenney 2021-06-22 19:08 ` Neeraj Upadhyay 0 siblings, 1 reply; 6+ messages in thread From: Paul E. McKenney @ 2021-06-22 17:58 UTC (permalink / raw) To: Neeraj Upadhyay Cc: josh, rostedt, mathieu.desnoyers, jiangshanlai, joel, rcu, linux-kernel, urezki, frederic On Tue, Jun 22, 2021 at 05:35:21PM +0530, Neeraj Upadhyay wrote: > In addition to irq and softirq state, check rcu_bh_lock_map > state, to decide whether RCU bh lock is held. > > Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org> My initial reaction was that "in_softirq() || irqs_disabled()" covers it because rcu_read_lock_bh() disables BH. But you are right that it does seem a bit silly to ignore lockdep. So would it also make sense to have a WARN_ON_ONCE() if lockdep claims we are under rcu_read_lock_bh() protection, but "in_softirq() || irqs_disabled()" think otherwise? Thanx, Paul > --- > kernel/rcu/update.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c > index c21b38c..d416f1c 100644 > --- a/kernel/rcu/update.c > +++ b/kernel/rcu/update.c > @@ -333,7 +333,7 @@ int rcu_read_lock_bh_held(void) > > if (rcu_read_lock_held_common(&ret)) > return ret; > - return in_softirq() || irqs_disabled(); > + return lock_is_held(&rcu_bh_lock_map) || in_softirq() || irqs_disabled(); > } > EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held); > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > hosted by The Linux Foundation > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rcu: update: Check rcu_bh_lock_map state in rcu_read_lock_bh_held 2021-06-22 17:58 ` Paul E. McKenney @ 2021-06-22 19:08 ` Neeraj Upadhyay 2021-06-22 23:46 ` Paul E. McKenney 0 siblings, 1 reply; 6+ messages in thread From: Neeraj Upadhyay @ 2021-06-22 19:08 UTC (permalink / raw) To: paulmck Cc: josh, rostedt, mathieu.desnoyers, jiangshanlai, joel, rcu, linux-kernel, urezki, frederic On 6/22/2021 11:28 PM, Paul E. McKenney wrote: > On Tue, Jun 22, 2021 at 05:35:21PM +0530, Neeraj Upadhyay wrote: >> In addition to irq and softirq state, check rcu_bh_lock_map >> state, to decide whether RCU bh lock is held. >> >> Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org> > > My initial reaction was that "in_softirq() || irqs_disabled()" covers > it because rcu_read_lock_bh() disables BH. But you are right that it > does seem a bit silly to ignore lockdep. > > So would it also make sense to have a WARN_ON_ONCE() if lockdep claims > we are under rcu_read_lock_bh() protection, but "in_softirq() || > irqs_disabled()" think otherwise? > > Thanx, Paul > After thinking more on this, looks like one intention of not having lockdep check here was to catch scenarios where some code enables bh after doing rcu_read_lock_bh(), as is mentioned in the comment above rcu_read_lock_bh_held(): Note that if someone uses rcu_read_lock_bh(), but then later enables BH, lockdep (if enabled) will show the situation. This is useful for debug checks in functions that require that they be called within an RCU read-side critical section. Client users seem to be doing lockdep checks on returned value: drivers/net/wireguard/peer.c RCU_LOCKDEP_WARN(!rcu_read_lock_bh_held(), Similarly, any rcu_dereference_check(..., rcu_read_lock_bh_held()) usage also triggers warning, if bh is enabled, inside rcu_read_lock_bh() section. So, using 'in_softirq() || irqs_disabled()' condition looks to be sufficient condition, to mark all read lock bh regions and adding '|| lock_is_held(&rcu_bh_lock_map)' to this condition does not seem to fit well with the RCU_LOCKDEP_WARN(!rcu_read_lock_bh_held()) and rcu_dereference_check(..., rcu_read_lock_bh_held()) calls, if we hit the scenario, where bh lockmap state (shows bh lock acquired) conflicts with the softirq/irq state . Thanks Neeraj >> --- >> kernel/rcu/update.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c >> index c21b38c..d416f1c 100644 >> --- a/kernel/rcu/update.c >> +++ b/kernel/rcu/update.c >> @@ -333,7 +333,7 @@ int rcu_read_lock_bh_held(void) >> >> if (rcu_read_lock_held_common(&ret)) >> return ret; >> - return in_softirq() || irqs_disabled(); >> + return lock_is_held(&rcu_bh_lock_map) || in_softirq() || irqs_disabled(); >> } >> EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held); >> >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >> hosted by The Linux Foundation >> -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rcu: update: Check rcu_bh_lock_map state in rcu_read_lock_bh_held 2021-06-22 19:08 ` Neeraj Upadhyay @ 2021-06-22 23:46 ` Paul E. McKenney 2021-06-23 3:57 ` Neeraj Upadhyay 0 siblings, 1 reply; 6+ messages in thread From: Paul E. McKenney @ 2021-06-22 23:46 UTC (permalink / raw) To: Neeraj Upadhyay Cc: josh, rostedt, mathieu.desnoyers, jiangshanlai, joel, rcu, linux-kernel, urezki, frederic On Wed, Jun 23, 2021 at 12:38:09AM +0530, Neeraj Upadhyay wrote: > > > On 6/22/2021 11:28 PM, Paul E. McKenney wrote: > > On Tue, Jun 22, 2021 at 05:35:21PM +0530, Neeraj Upadhyay wrote: > > > In addition to irq and softirq state, check rcu_bh_lock_map > > > state, to decide whether RCU bh lock is held. > > > > > > Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org> > > > > My initial reaction was that "in_softirq() || irqs_disabled()" covers > > it because rcu_read_lock_bh() disables BH. But you are right that it > > does seem a bit silly to ignore lockdep. > > > > So would it also make sense to have a WARN_ON_ONCE() if lockdep claims > > we are under rcu_read_lock_bh() protection, but "in_softirq() || > > irqs_disabled()" think otherwise? > > After thinking more on this, looks like one intention of not > having lockdep check here was to catch scenarios where some code enables bh > after doing rcu_read_lock_bh(), as is mentioned in the comment above > rcu_read_lock_bh_held(): > > Note that if someone uses > rcu_read_lock_bh(), but then later enables BH, lockdep (if enabled) > will show the situation. This is useful for debug checks in functions > that require that they be called within an RCU read-side critical > section. > > Client users seem to be doing lockdep checks on returned value: > drivers/net/wireguard/peer.c > RCU_LOCKDEP_WARN(!rcu_read_lock_bh_held(), > > Similarly, any rcu_dereference_check(..., rcu_read_lock_bh_held()) usage > also triggers warning, if bh is enabled, inside rcu_read_lock_bh() > section. > > So, using 'in_softirq() || irqs_disabled()' condition looks to be sufficient > condition, to mark all read lock bh regions and adding '|| > lock_is_held(&rcu_bh_lock_map)' to this condition does not seem to fit > well with the RCU_LOCKDEP_WARN(!rcu_read_lock_bh_held()) and > rcu_dereference_check(..., rcu_read_lock_bh_held()) calls, if we hit > the scenario, where bh lockmap state (shows bh lock acquired) conflicts with > the softirq/irq state . That makes sense to me! But should there be checks somewhere for something like "lock_is_held(&rcu_bh_lock_map) && !in_softirq() && !irqs_disabled()"? Thanx, Paul > Thanks > Neeraj > > > > --- > > > kernel/rcu/update.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c > > > index c21b38c..d416f1c 100644 > > > --- a/kernel/rcu/update.c > > > +++ b/kernel/rcu/update.c > > > @@ -333,7 +333,7 @@ int rcu_read_lock_bh_held(void) > > > if (rcu_read_lock_held_common(&ret)) > > > return ret; > > > - return in_softirq() || irqs_disabled(); > > > + return lock_is_held(&rcu_bh_lock_map) || in_softirq() || irqs_disabled(); > > > } > > > EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held); > > > -- > > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > > > hosted by The Linux Foundation > > > > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of > the Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rcu: update: Check rcu_bh_lock_map state in rcu_read_lock_bh_held 2021-06-22 23:46 ` Paul E. McKenney @ 2021-06-23 3:57 ` Neeraj Upadhyay 2021-06-23 15:39 ` Paul E. McKenney 0 siblings, 1 reply; 6+ messages in thread From: Neeraj Upadhyay @ 2021-06-23 3:57 UTC (permalink / raw) To: paulmck Cc: josh, rostedt, mathieu.desnoyers, jiangshanlai, joel, rcu, linux-kernel, urezki, frederic On 6/23/2021 5:16 AM, Paul E. McKenney wrote: > On Wed, Jun 23, 2021 at 12:38:09AM +0530, Neeraj Upadhyay wrote: >> >> >> On 6/22/2021 11:28 PM, Paul E. McKenney wrote: >>> On Tue, Jun 22, 2021 at 05:35:21PM +0530, Neeraj Upadhyay wrote: >>>> In addition to irq and softirq state, check rcu_bh_lock_map >>>> state, to decide whether RCU bh lock is held. >>>> >>>> Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org> >>> >>> My initial reaction was that "in_softirq() || irqs_disabled()" covers >>> it because rcu_read_lock_bh() disables BH. But you are right that it >>> does seem a bit silly to ignore lockdep. >>> >>> So would it also make sense to have a WARN_ON_ONCE() if lockdep claims >>> we are under rcu_read_lock_bh() protection, but "in_softirq() || >>> irqs_disabled()" think otherwise? >> >> After thinking more on this, looks like one intention of not >> having lockdep check here was to catch scenarios where some code enables bh >> after doing rcu_read_lock_bh(), as is mentioned in the comment above >> rcu_read_lock_bh_held(): >> >> Note that if someone uses >> rcu_read_lock_bh(), but then later enables BH, lockdep (if enabled) >> will show the situation. This is useful for debug checks in functions >> that require that they be called within an RCU read-side critical >> section. >> >> Client users seem to be doing lockdep checks on returned value: >> drivers/net/wireguard/peer.c >> RCU_LOCKDEP_WARN(!rcu_read_lock_bh_held(), >> >> Similarly, any rcu_dereference_check(..., rcu_read_lock_bh_held()) usage >> also triggers warning, if bh is enabled, inside rcu_read_lock_bh() >> section. >> >> So, using 'in_softirq() || irqs_disabled()' condition looks to be sufficient >> condition, to mark all read lock bh regions and adding '|| >> lock_is_held(&rcu_bh_lock_map)' to this condition does not seem to fit >> well with the RCU_LOCKDEP_WARN(!rcu_read_lock_bh_held()) and >> rcu_dereference_check(..., rcu_read_lock_bh_held()) calls, if we hit >> the scenario, where bh lockmap state (shows bh lock acquired) conflicts with >> the softirq/irq state . > > That makes sense to me! > > But should there be checks somewhere for something like > "lock_is_held(&rcu_bh_lock_map) && !in_softirq() && !irqs_disabled()"? > > Thanx, Paul > I think this check is good to have inside rcu_read_lock_bh_held(), to highlight this scenario explicitly; I am thinking, if it makes sense to have lock_is_held(&rcu_bh_lock_map) check in rcu_softirq_qs() ? Also, I think this check is more important for rcu_read_lock_sched_held(), where lockdep state is used as a sufficient condition, for marking a RCU sched region. One more api is rcu_read_lock_any_held(), where we can warn on conflicting cases. int rcu_read_lock_sched_held(void) { bool ret; if (rcu_read_lock_held_common(&ret)) return ret; return lock_is_held(&rcu_sched_lock_map) || !preemptible(); } Thanks Neeraj >> Thanks >> Neeraj >> >>>> --- >>>> kernel/rcu/update.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c >>>> index c21b38c..d416f1c 100644 >>>> --- a/kernel/rcu/update.c >>>> +++ b/kernel/rcu/update.c >>>> @@ -333,7 +333,7 @@ int rcu_read_lock_bh_held(void) >>>> if (rcu_read_lock_held_common(&ret)) >>>> return ret; >>>> - return in_softirq() || irqs_disabled(); >>>> + return lock_is_held(&rcu_bh_lock_map) || in_softirq() || irqs_disabled(); >>>> } >>>> EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held); >>>> -- >>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >>>> hosted by The Linux Foundation >>>> >> >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of >> the Code Aurora Forum, hosted by The Linux Foundation -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] rcu: update: Check rcu_bh_lock_map state in rcu_read_lock_bh_held 2021-06-23 3:57 ` Neeraj Upadhyay @ 2021-06-23 15:39 ` Paul E. McKenney 0 siblings, 0 replies; 6+ messages in thread From: Paul E. McKenney @ 2021-06-23 15:39 UTC (permalink / raw) To: Neeraj Upadhyay Cc: josh, rostedt, mathieu.desnoyers, jiangshanlai, joel, rcu, linux-kernel, urezki, frederic On Wed, Jun 23, 2021 at 09:27:11AM +0530, Neeraj Upadhyay wrote: > On 6/23/2021 5:16 AM, Paul E. McKenney wrote: > > On Wed, Jun 23, 2021 at 12:38:09AM +0530, Neeraj Upadhyay wrote: > > > On 6/22/2021 11:28 PM, Paul E. McKenney wrote: > > > > On Tue, Jun 22, 2021 at 05:35:21PM +0530, Neeraj Upadhyay wrote: > > > > > In addition to irq and softirq state, check rcu_bh_lock_map > > > > > state, to decide whether RCU bh lock is held. > > > > > > > > > > Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org> > > > > > > > > My initial reaction was that "in_softirq() || irqs_disabled()" covers > > > > it because rcu_read_lock_bh() disables BH. But you are right that it > > > > does seem a bit silly to ignore lockdep. > > > > > > > > So would it also make sense to have a WARN_ON_ONCE() if lockdep claims > > > > we are under rcu_read_lock_bh() protection, but "in_softirq() || > > > > irqs_disabled()" think otherwise? > > > > > > After thinking more on this, looks like one intention of not > > > having lockdep check here was to catch scenarios where some code enables bh > > > after doing rcu_read_lock_bh(), as is mentioned in the comment above > > > rcu_read_lock_bh_held(): > > > > > > Note that if someone uses > > > rcu_read_lock_bh(), but then later enables BH, lockdep (if enabled) > > > will show the situation. This is useful for debug checks in functions > > > that require that they be called within an RCU read-side critical > > > section. > > > > > > Client users seem to be doing lockdep checks on returned value: > > > drivers/net/wireguard/peer.c > > > RCU_LOCKDEP_WARN(!rcu_read_lock_bh_held(), > > > > > > Similarly, any rcu_dereference_check(..., rcu_read_lock_bh_held()) usage > > > also triggers warning, if bh is enabled, inside rcu_read_lock_bh() > > > section. > > > > > > So, using 'in_softirq() || irqs_disabled()' condition looks to be sufficient > > > condition, to mark all read lock bh regions and adding '|| > > > lock_is_held(&rcu_bh_lock_map)' to this condition does not seem to fit > > > well with the RCU_LOCKDEP_WARN(!rcu_read_lock_bh_held()) and > > > rcu_dereference_check(..., rcu_read_lock_bh_held()) calls, if we hit > > > the scenario, where bh lockmap state (shows bh lock acquired) conflicts with > > > the softirq/irq state . > > > > That makes sense to me! > > > > But should there be checks somewhere for something like > > "lock_is_held(&rcu_bh_lock_map) && !in_softirq() && !irqs_disabled()"? > > > > Thanx, Paul > > > > I think this check is good to have inside rcu_read_lock_bh_held(), to > highlight this scenario explicitly; I am thinking, if it makes sense to > have lock_is_held(&rcu_bh_lock_map) check in rcu_softirq_qs() ? > > Also, I think this check is more important for rcu_read_lock_sched_held(), > where lockdep state is used as a sufficient condition, for marking a RCU > sched region. One more api is rcu_read_lock_any_held(), where we can > warn on conflicting cases. > > int rcu_read_lock_sched_held(void) > { > bool ret; > > if (rcu_read_lock_held_common(&ret)) > return ret; > return lock_is_held(&rcu_sched_lock_map) || !preemptible(); > } Another option would be to check lock_is_held(&rcu_sched_lock_map) anywhere preemption is enabled and lock_is_held(&rcu_bh_lock_map) anywhere BH is enabled. This would (in theory, anyway) catch more bugs. But it is necessary to be careful because these checks must be suppressed if lockdep has been disabled due to a prior lockdep splat. Otherwise, the first lockdep splat is followed by an inundation of false-positive complaints. But the real question is "Which important bugs are missed today, and what change would catch them more reliably?" Thoughts? Thanx, Paul > Thanks > Neeraj > > > > Thanks > > > Neeraj > > > > > > > > --- > > > > > kernel/rcu/update.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c > > > > > index c21b38c..d416f1c 100644 > > > > > --- a/kernel/rcu/update.c > > > > > +++ b/kernel/rcu/update.c > > > > > @@ -333,7 +333,7 @@ int rcu_read_lock_bh_held(void) > > > > > if (rcu_read_lock_held_common(&ret)) > > > > > return ret; > > > > > - return in_softirq() || irqs_disabled(); > > > > > + return lock_is_held(&rcu_bh_lock_map) || in_softirq() || irqs_disabled(); > > > > > } > > > > > EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held); > > > > > -- > > > > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > > > > > hosted by The Linux Foundation > > > > > > > > > > > -- > > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of > > > the Code Aurora Forum, hosted by The Linux Foundation > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of > the Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-23 15:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-22 12:05 [PATCH] rcu: update: Check rcu_bh_lock_map state in rcu_read_lock_bh_held Neeraj Upadhyay 2021-06-22 17:58 ` Paul E. McKenney 2021-06-22 19:08 ` Neeraj Upadhyay 2021-06-22 23:46 ` Paul E. McKenney 2021-06-23 3:57 ` Neeraj Upadhyay 2021-06-23 15:39 ` Paul E. McKenney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).