Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
On Fri, Jul 12, 2019 at 03:40:40PM -0400, Joel Fernandes wrote:
> On Fri, Jul 12, 2019 at 10:46:30AM -0700, Paul E. McKenney wrote:
> > On Fri, Jul 12, 2019 at 01:06:31PM -0400, Joel Fernandes wrote:
> > > On Fri, Jul 12, 2019 at 09:45:31AM -0700, Paul E. McKenney wrote:
> > > > On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> > > > > On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> > > > > > On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google)
> > > > > > wrote:
> > > > > > > +int rcu_read_lock_any_held(void)
> > > > > > > +{
> > > > > > > + int lockdep_opinion = 0;
> > > > > > > +
> > > > > > > + if (!debug_lockdep_rcu_enabled())
> > > > > > > + return 1;
> > > > > > > + if (!rcu_is_watching())
> > > > > > > + return 0;
> > > > > > > + if (!rcu_lockdep_current_cpu_online())
> > > > > > > + return 0;
> > > > > > > +
> > > > > > > + /* Preemptible RCU flavor */
> > > > > > > + if (lock_is_held(&rcu_lock_map))
> > > > > >
> > > > > > you forgot debug_locks here.
> > > > >
> > > > > Actually, it turns out debug_locks checking is not even needed. If
> > > > > debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we
> > > > > would not
> > > > > get to this point.
> > > > >
> > > > > > > + return 1;
> > > > > > > +
> > > > > > > + /* BH flavor */
> > > > > > > + if (in_softirq() || irqs_disabled())
> > > > > >
> > > > > > I'm not sure I'd put irqs_disabled() under BH, also this entire
> > > > > > condition is superfluous, see below.
> > > > > >
> > > > > > > + return 1;
> > > > > > > +
> > > > > > > + /* Sched flavor */
> > > > > > > + if (debug_locks)
> > > > > > > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > > > > > + return lockdep_opinion || !preemptible();
> > > > > >
> > > > > > that !preemptible() turns into:
> > > > > >
> > > > > > !(preempt_count()==0 && !irqs_disabled())
> > > > > >
> > > > > > which is:
> > > > > >
> > > > > > preempt_count() != 0 || irqs_disabled()
> > > > > >
> > > > > > and already includes irqs_disabled() and in_softirq().
> > > > > >
> > > > > > > +}
> > > > > >
> > > > > > So maybe something lke:
> > > > > >
> > > > > > if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> > > > > > lock_is_held(&rcu_sched_lock_map)))
> > > > > > return true;
> > > > >
> > > > > Agreed, I will do it this way (without the debug_locks) like:
> > > > >
> > > > > ---8<---
> > > > >
> > > > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > > > > index ba861d1716d3..339aebc330db 100644
> > > > > --- a/kernel/rcu/update.c
> > > > > +++ b/kernel/rcu/update.c
> > > > > @@ -296,27 +296,15 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
> > > > >
> > > > > int rcu_read_lock_any_held(void)
> > > > > {
> > > > > - int lockdep_opinion = 0;
> > > > > -
> > > > > if (!debug_lockdep_rcu_enabled())
> > > > > return 1;
> > > > > if (!rcu_is_watching())
> > > > > return 0;
> > > > > if (!rcu_lockdep_current_cpu_online())
> > > > > return 0;
> > > > > -
> > > > > - /* Preemptible RCU flavor */
> > > > > - if (lock_is_held(&rcu_lock_map))
> > > > > - return 1;
> > > > > -
> > > > > - /* BH flavor */
> > > > > - if (in_softirq() || irqs_disabled())
> > > > > - return 1;
> > > > > -
> > > > > - /* Sched flavor */
> > > > > - if (debug_locks)
> > > > > - lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > > > - return lockdep_opinion || !preemptible();
> > > > > + if (lock_is_held(&rcu_lock_map) ||
> > > > > lock_is_held(&rcu_sched_lock_map))
> > > >
> > > > OK, I will bite... Why not also lock_is_held(&rcu_bh_lock_map)?
> > >
> > > Hmm, I was borrowing the strategy from rcu_read_lock_bh_held() which does
> > > not
> > > check for a lock held in this map.
> > >
> > > Honestly, even lock_is_held(&rcu_sched_lock_map) seems unnecessary per-se
> > > since !preemptible() will catch that? rcu_read_lock_sched() disables
> > > preemption already, so lockdep's opinion of the matter seems redundant
> > > there.
> >
> > Good point! At least as long as the lockdep splats list RCU-bh among
> > the locks held, which they did last I checked.
> >
> > Of course, you could make the same argument for getting rid of
> > rcu_sched_lock_map. Does it make sense to have the one without
> > the other?
>
> It probably makes it inconsistent in the least. I will add the check for
> the rcu_bh_lock_map in a separate patch, if that's Ok with you - since I also
> want to update the rcu_read_lock_bh_held() logic in the same patch.
>
> That rcu_read_lock_bh_held() could also just return !preemptible as Peter
> suggested for the bh case.
Although that seems reasonable, please check the call sites.
> > > Sorry I already sent out patches again before seeing your comment but I
> > > can
> >
Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
On Fri, Jul 12, 2019 at 10:46:30AM -0700, Paul E. McKenney wrote:
> On Fri, Jul 12, 2019 at 01:06:31PM -0400, Joel Fernandes wrote:
> > On Fri, Jul 12, 2019 at 09:45:31AM -0700, Paul E. McKenney wrote:
> > > On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> > > > On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> > > > > On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google)
> > > > > wrote:
> > > > > > +int rcu_read_lock_any_held(void)
> > > > > > +{
> > > > > > + int lockdep_opinion = 0;
> > > > > > +
> > > > > > + if (!debug_lockdep_rcu_enabled())
> > > > > > + return 1;
> > > > > > + if (!rcu_is_watching())
> > > > > > + return 0;
> > > > > > + if (!rcu_lockdep_current_cpu_online())
> > > > > > + return 0;
> > > > > > +
> > > > > > + /* Preemptible RCU flavor */
> > > > > > + if (lock_is_held(&rcu_lock_map))
> > > > >
> > > > > you forgot debug_locks here.
> > > >
> > > > Actually, it turns out debug_locks checking is not even needed. If
> > > > debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we
> > > > would not
> > > > get to this point.
> > > >
> > > > > > + return 1;
> > > > > > +
> > > > > > + /* BH flavor */
> > > > > > + if (in_softirq() || irqs_disabled())
> > > > >
> > > > > I'm not sure I'd put irqs_disabled() under BH, also this entire
> > > > > condition is superfluous, see below.
> > > > >
> > > > > > + return 1;
> > > > > > +
> > > > > > + /* Sched flavor */
> > > > > > + if (debug_locks)
> > > > > > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > > > > + return lockdep_opinion || !preemptible();
> > > > >
> > > > > that !preemptible() turns into:
> > > > >
> > > > > !(preempt_count()==0 && !irqs_disabled())
> > > > >
> > > > > which is:
> > > > >
> > > > > preempt_count() != 0 || irqs_disabled()
> > > > >
> > > > > and already includes irqs_disabled() and in_softirq().
> > > > >
> > > > > > +}
> > > > >
> > > > > So maybe something lke:
> > > > >
> > > > > if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> > > > > lock_is_held(&rcu_sched_lock_map)))
> > > > > return true;
> > > >
> > > > Agreed, I will do it this way (without the debug_locks) like:
> > > >
> > > > ---8<---
> > > >
> > > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > > > index ba861d1716d3..339aebc330db 100644
> > > > --- a/kernel/rcu/update.c
> > > > +++ b/kernel/rcu/update.c
> > > > @@ -296,27 +296,15 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
> > > >
> > > > int rcu_read_lock_any_held(void)
> > > > {
> > > > - int lockdep_opinion = 0;
> > > > -
> > > > if (!debug_lockdep_rcu_enabled())
> > > > return 1;
> > > > if (!rcu_is_watching())
> > > > return 0;
> > > > if (!rcu_lockdep_current_cpu_online())
> > > > return 0;
> > > > -
> > > > - /* Preemptible RCU flavor */
> > > > - if (lock_is_held(&rcu_lock_map))
> > > > - return 1;
> > > > -
> > > > - /* BH flavor */
> > > > - if (in_softirq() || irqs_disabled())
> > > > - return 1;
> > > > -
> > > > - /* Sched flavor */
> > > > - if (debug_locks)
> > > > - lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > > - return lockdep_opinion || !preemptible();
> > > > + if (lock_is_held(&rcu_lock_map) ||
> > > > lock_is_held(&rcu_sched_lock_map))
> > >
> > > OK, I will bite... Why not also lock_is_held(&rcu_bh_lock_map)?
> >
> > Hmm, I was borrowing the strategy from rcu_read_lock_bh_held() which does
> > not
> > check for a lock held in this map.
> >
> > Honestly, even lock_is_held(&rcu_sched_lock_map) seems unnecessary per-se
> > since !preemptible() will catch that? rcu_read_lock_sched() disables
> > preemption already, so lockdep's opinion of the matter seems redundant
> > there.
>
> Good point! At least as long as the lockdep splats list RCU-bh among
> the locks held, which they did last I checked.
>
> Of course, you could make the same argument for getting rid of
> rcu_sched_lock_map. Does it make sense to have the one without
> the other?
It probably makes it inconsistent in the least. I will add the check for
the rcu_bh_lock_map in a separate patch, if that's Ok with you - since I also
want to update the rcu_read_lock_bh_held() logic in the same patch.
That rcu_read_lock_bh_held() could also just return !preemptible as Peter
suggested for the bh case.
> > Sorry I already sent out patches again before seeing your comment but I can
> > rework and resend them based on any other suggestions.
>
> Not a problem!
Thanks. Depending on whether there is any other feedback, I will work on the
bh_ stuff as a separate patch on top of this series, or work it into the next
series revision if I'm reposting. Hopefully that sounds Ok to you.
t
Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
On Fri, Jul 12, 2019 at 01:06:31PM -0400, Joel Fernandes wrote:
> On Fri, Jul 12, 2019 at 09:45:31AM -0700, Paul E. McKenney wrote:
> > On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> > > On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> > > > On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> > > > > +int rcu_read_lock_any_held(void)
> > > > > +{
> > > > > + int lockdep_opinion = 0;
> > > > > +
> > > > > + if (!debug_lockdep_rcu_enabled())
> > > > > + return 1;
> > > > > + if (!rcu_is_watching())
> > > > > + return 0;
> > > > > + if (!rcu_lockdep_current_cpu_online())
> > > > > + return 0;
> > > > > +
> > > > > + /* Preemptible RCU flavor */
> > > > > + if (lock_is_held(&rcu_lock_map))
> > > >
> > > > you forgot debug_locks here.
> > >
> > > Actually, it turns out debug_locks checking is not even needed. If
> > > debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we would
> > > not
> > > get to this point.
> > >
> > > > > + return 1;
> > > > > +
> > > > > + /* BH flavor */
> > > > > + if (in_softirq() || irqs_disabled())
> > > >
> > > > I'm not sure I'd put irqs_disabled() under BH, also this entire
> > > > condition is superfluous, see below.
> > > >
> > > > > + return 1;
> > > > > +
> > > > > + /* Sched flavor */
> > > > > + if (debug_locks)
> > > > > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > > > + return lockdep_opinion || !preemptible();
> > > >
> > > > that !preemptible() turns into:
> > > >
> > > > !(preempt_count()==0 && !irqs_disabled())
> > > >
> > > > which is:
> > > >
> > > > preempt_count() != 0 || irqs_disabled()
> > > >
> > > > and already includes irqs_disabled() and in_softirq().
> > > >
> > > > > +}
> > > >
> > > > So maybe something lke:
> > > >
> > > > if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> > > > lock_is_held(&rcu_sched_lock_map)))
> > > > return true;
> > >
> > > Agreed, I will do it this way (without the debug_locks) like:
> > >
> > > ---8<---
> > >
> > > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > > index ba861d1716d3..339aebc330db 100644
> > > --- a/kernel/rcu/update.c
> > > +++ b/kernel/rcu/update.c
> > > @@ -296,27 +296,15 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
> > >
> > > int rcu_read_lock_any_held(void)
> > > {
> > > - int lockdep_opinion = 0;
> > > -
> > > if (!debug_lockdep_rcu_enabled())
> > > return 1;
> > > if (!rcu_is_watching())
> > > return 0;
> > > if (!rcu_lockdep_current_cpu_online())
> > > return 0;
> > > -
> > > - /* Preemptible RCU flavor */
> > > - if (lock_is_held(&rcu_lock_map))
> > > - return 1;
> > > -
> > > - /* BH flavor */
> > > - if (in_softirq() || irqs_disabled())
> > > - return 1;
> > > -
> > > - /* Sched flavor */
> > > - if (debug_locks)
> > > - lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > - return lockdep_opinion || !preemptible();
> > > + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
> >
> > OK, I will bite... Why not also lock_is_held(&rcu_bh_lock_map)?
>
> Hmm, I was borrowing the strategy from rcu_read_lock_bh_held() which does not
> check for a lock held in this map.
>
> Honestly, even lock_is_held(&rcu_sched_lock_map) seems unnecessary per-se
> since !preemptible() will catch that? rcu_read_lock_sched() disables
> preemption already, so lockdep's opinion of the matter seems redundant there.
Good point! At least as long as the lockdep splats list RCU-bh among
the locks held, which they did last I checked.
Of course, you could make the same argument for getting rid of
rcu_sched_lock_map. Does it make sense to have the one without
the other?
> Sorry I already sent out patches again before seeing your comment but I can
> rework and resend them based on any other suggestions.
Not a problem!
Thax, Paul
Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
On Fri, Jul 12, 2019 at 09:45:31AM -0700, Paul E. McKenney wrote:
> On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> > On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> > > On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> > > > +int rcu_read_lock_any_held(void)
> > > > +{
> > > > + int lockdep_opinion = 0;
> > > > +
> > > > + if (!debug_lockdep_rcu_enabled())
> > > > + return 1;
> > > > + if (!rcu_is_watching())
> > > > + return 0;
> > > > + if (!rcu_lockdep_current_cpu_online())
> > > > + return 0;
> > > > +
> > > > + /* Preemptible RCU flavor */
> > > > + if (lock_is_held(&rcu_lock_map))
> > >
> > > you forgot debug_locks here.
> >
> > Actually, it turns out debug_locks checking is not even needed. If
> > debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we would
> > not
> > get to this point.
> >
> > > > + return 1;
> > > > +
> > > > + /* BH flavor */
> > > > + if (in_softirq() || irqs_disabled())
> > >
> > > I'm not sure I'd put irqs_disabled() under BH, also this entire
> > > condition is superfluous, see below.
> > >
> > > > + return 1;
> > > > +
> > > > + /* Sched flavor */
> > > > + if (debug_locks)
> > > > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > > + return lockdep_opinion || !preemptible();
> > >
> > > that !preemptible() turns into:
> > >
> > > !(preempt_count()==0 && !irqs_disabled())
> > >
> > > which is:
> > >
> > > preempt_count() != 0 || irqs_disabled()
> > >
> > > and already includes irqs_disabled() and in_softirq().
> > >
> > > > +}
> > >
> > > So maybe something lke:
> > >
> > > if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> > > lock_is_held(&rcu_sched_lock_map)))
> > > return true;
> >
> > Agreed, I will do it this way (without the debug_locks) like:
> >
> > ---8<---
> >
> > diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> > index ba861d1716d3..339aebc330db 100644
> > --- a/kernel/rcu/update.c
> > +++ b/kernel/rcu/update.c
> > @@ -296,27 +296,15 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
> >
> > int rcu_read_lock_any_held(void)
> > {
> > - int lockdep_opinion = 0;
> > -
> > if (!debug_lockdep_rcu_enabled())
> > return 1;
> > if (!rcu_is_watching())
> > return 0;
> > if (!rcu_lockdep_current_cpu_online())
> > return 0;
> > -
> > - /* Preemptible RCU flavor */
> > - if (lock_is_held(&rcu_lock_map))
> > - return 1;
> > -
> > - /* BH flavor */
> > - if (in_softirq() || irqs_disabled())
> > - return 1;
> > -
> > - /* Sched flavor */
> > - if (debug_locks)
> > - lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > - return lockdep_opinion || !preemptible();
> > + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
>
> OK, I will bite... Why not also lock_is_held(&rcu_bh_lock_map)?
Hmm, I was borrowing the strategy from rcu_read_lock_bh_held() which does not
check for a lock held in this map.
Honestly, even lock_is_held(&rcu_sched_lock_map) seems unnecessary per-se
since !preemptible() will catch that? rcu_read_lock_sched() disables
preemption already, so lockdep's opinion of the matter seems redundant there.
Sorry I already sent out patches again before seeing your comment but I can
rework and resend them based on any other suggestions.
thanks,
- Joel
Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> > On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> > > +int rcu_read_lock_any_held(void)
> > > +{
> > > + int lockdep_opinion = 0;
> > > +
> > > + if (!debug_lockdep_rcu_enabled())
> > > + return 1;
> > > + if (!rcu_is_watching())
> > > + return 0;
> > > + if (!rcu_lockdep_current_cpu_online())
> > > + return 0;
> > > +
> > > + /* Preemptible RCU flavor */
> > > + if (lock_is_held(&rcu_lock_map))
> >
> > you forgot debug_locks here.
>
> Actually, it turns out debug_locks checking is not even needed. If
> debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we would not
> get to this point.
>
> > > + return 1;
> > > +
> > > + /* BH flavor */
> > > + if (in_softirq() || irqs_disabled())
> >
> > I'm not sure I'd put irqs_disabled() under BH, also this entire
> > condition is superfluous, see below.
> >
> > > + return 1;
> > > +
> > > + /* Sched flavor */
> > > + if (debug_locks)
> > > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > > + return lockdep_opinion || !preemptible();
> >
> > that !preemptible() turns into:
> >
> > !(preempt_count()==0 && !irqs_disabled())
> >
> > which is:
> >
> > preempt_count() != 0 || irqs_disabled()
> >
> > and already includes irqs_disabled() and in_softirq().
> >
> > > +}
> >
> > So maybe something lke:
> >
> > if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> > lock_is_held(&rcu_sched_lock_map)))
> > return true;
>
> Agreed, I will do it this way (without the debug_locks) like:
>
> ---8<---
>
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index ba861d1716d3..339aebc330db 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -296,27 +296,15 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
>
> int rcu_read_lock_any_held(void)
> {
> - int lockdep_opinion = 0;
> -
> if (!debug_lockdep_rcu_enabled())
> return 1;
> if (!rcu_is_watching())
> return 0;
> if (!rcu_lockdep_current_cpu_online())
> return 0;
> -
> - /* Preemptible RCU flavor */
> - if (lock_is_held(&rcu_lock_map))
> - return 1;
> -
> - /* BH flavor */
> - if (in_softirq() || irqs_disabled())
> - return 1;
> -
> - /* Sched flavor */
> - if (debug_locks)
> - lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> - return lockdep_opinion || !preemptible();
> + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
OK, I will bite... Why not also lock_is_held(&rcu_bh_lock_map)?
Thanx, Paul
> + return 1;
> + return !preemptible();
> }
> EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
>
>
Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
On Fri, Jul 12, 2019 at 11:10:51AM -0400, Joel Fernandes wrote:
> Agreed, I will do it this way (without the debug_locks) like:
>
> ---8<---
>
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index ba861d1716d3..339aebc330db 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -296,27 +296,15 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
>
> int rcu_read_lock_any_held(void)
> {
> if (!debug_lockdep_rcu_enabled())
> return 1;
> if (!rcu_is_watching())
> return 0;
> if (!rcu_lockdep_current_cpu_online())
> return 0;
> + if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
> + return 1;
> + return !preemptible();
> }
> EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
OK, that looks sane. Thanks!
Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
On Fri, Jul 12, 2019 at 01:11:25PM +0200, Peter Zijlstra wrote:
> On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> > +int rcu_read_lock_any_held(void)
> > +{
> > + int lockdep_opinion = 0;
> > +
> > + if (!debug_lockdep_rcu_enabled())
> > + return 1;
> > + if (!rcu_is_watching())
> > + return 0;
> > + if (!rcu_lockdep_current_cpu_online())
> > + return 0;
> > +
> > + /* Preemptible RCU flavor */
> > + if (lock_is_held(&rcu_lock_map))
>
> you forgot debug_locks here.
Actually, it turns out debug_locks checking is not even needed. If
debug_locks == 0, then debug_lockdep_rcu_enabled() returns 0 and we would not
get to this point.
> > + return 1;
> > +
> > + /* BH flavor */
> > + if (in_softirq() || irqs_disabled())
>
> I'm not sure I'd put irqs_disabled() under BH, also this entire
> condition is superfluous, see below.
>
> > + return 1;
> > +
> > + /* Sched flavor */
> > + if (debug_locks)
> > + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> > + return lockdep_opinion || !preemptible();
>
> that !preemptible() turns into:
>
> !(preempt_count()==0 && !irqs_disabled())
>
> which is:
>
> preempt_count() != 0 || irqs_disabled()
>
> and already includes irqs_disabled() and in_softirq().
>
> > +}
>
> So maybe something lke:
>
> if (debug_locks && (lock_is_held(&rcu_lock_map) ||
> lock_is_held(&rcu_sched_lock_map)))
> return true;
Agreed, I will do it this way (without the debug_locks) like:
---8<---
diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index ba861d1716d3..339aebc330db 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -296,27 +296,15 @@ EXPORT_SYMBOL_GPL(rcu_read_lock_bh_held);
int rcu_read_lock_any_held(void)
{
- int lockdep_opinion = 0;
-
if (!debug_lockdep_rcu_enabled())
return 1;
if (!rcu_is_watching())
return 0;
if (!rcu_lockdep_current_cpu_online())
return 0;
-
- /* Preemptible RCU flavor */
- if (lock_is_held(&rcu_lock_map))
- return 1;
-
- /* BH flavor */
- if (in_softirq() || irqs_disabled())
- return 1;
-
- /* Sched flavor */
- if (debug_locks)
- lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
- return lockdep_opinion || !preemptible();
+ if (lock_is_held(&rcu_lock_map) || lock_is_held(&rcu_sched_lock_map))
+ return 1;
+ return !preemptible();
}
EXPORT_SYMBOL_GPL(rcu_read_lock_any_held);
Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
On Fri, Jul 12, 2019 at 01:01:42PM +0200, Peter Zijlstra wrote: > On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote: > > This patch adds support for checking RCU reader sections in list > > traversal macros. Optionally, if the list macro is called under SRCU or > > other lock/mutex protection, then appropriate lockdep expressions can be > > passed to make the checks pass. > > > > Existing list_for_each_entry_rcu() invocations don't need to pass the > > optional fourth argument (cond) unless they are under some non-RCU > > protection and needs to make lockdep check pass. > > > > Signed-off-by: Joel Fernandes (Google) > > --- > > include/linux/rculist.h | 29 - > > include/linux/rcupdate.h | 7 +++ > > kernel/rcu/Kconfig.debug | 11 +++ > > kernel/rcu/update.c | 26 ++ > > 4 files changed, 68 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h > > index e91ec9ddcd30..78c15ec6b2c9 100644 > > --- a/include/linux/rculist.h > > +++ b/include/linux/rculist.h > > @@ -40,6 +40,23 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head > > *list) > > */ > > #define list_next_rcu(list)(*((struct list_head __rcu > > **)(&(list)->next))) > > > > +/* > > + * Check during list traversal that we are within an RCU reader > > + */ > > + > > +#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6 > > +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0) > > You don't seem to actually use it in this patch; also linux/kernel.h has > COUNT_ARGS(). Yes, I replied after sending patches that I fixed this. I will remove them. thanks, - Joel
Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
On Fri, Jul 12, 2019 at 02:12:00PM +0200, Oleg Nesterov wrote: > On 07/11, Joel Fernandes (Google) wrote: > > > > +int rcu_read_lock_any_held(void) > > rcu_sync_is_idle() wants it. You have my ack in advance ;) Cool, thanks ;) - Joel
Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
On 07/11, Joel Fernandes (Google) wrote: > > +int rcu_read_lock_any_held(void) rcu_sync_is_idle() wants it. You have my ack in advance ;) Oleg.
Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote:
> +int rcu_read_lock_any_held(void)
> +{
> + int lockdep_opinion = 0;
> +
> + if (!debug_lockdep_rcu_enabled())
> + return 1;
> + if (!rcu_is_watching())
> + return 0;
> + if (!rcu_lockdep_current_cpu_online())
> + return 0;
> +
> + /* Preemptible RCU flavor */
> + if (lock_is_held(&rcu_lock_map))
you forgot debug_locks here.
> + return 1;
> +
> + /* BH flavor */
> + if (in_softirq() || irqs_disabled())
I'm not sure I'd put irqs_disabled() under BH, also this entire
condition is superfluous, see below.
> + return 1;
> +
> + /* Sched flavor */
> + if (debug_locks)
> + lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> + return lockdep_opinion || !preemptible();
that !preemptible() turns into:
!(preempt_count()==0 && !irqs_disabled())
which is:
preempt_count() != 0 || irqs_disabled()
and already includes irqs_disabled() and in_softirq().
> +}
So maybe something lke:
if (debug_locks && (lock_is_held(&rcu_lock_map) ||
lock_is_held(&rcu_sched_lock_map)))
return true;
return !preemptible();
Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote: > This patch adds support for checking RCU reader sections in list > traversal macros. Optionally, if the list macro is called under SRCU or > other lock/mutex protection, then appropriate lockdep expressions can be > passed to make the checks pass. > > Existing list_for_each_entry_rcu() invocations don't need to pass the > optional fourth argument (cond) unless they are under some non-RCU > protection and needs to make lockdep check pass. > > Signed-off-by: Joel Fernandes (Google) > --- > include/linux/rculist.h | 29 - > include/linux/rcupdate.h | 7 +++ > kernel/rcu/Kconfig.debug | 11 +++ > kernel/rcu/update.c | 26 ++ > 4 files changed, 68 insertions(+), 5 deletions(-) > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h > index e91ec9ddcd30..78c15ec6b2c9 100644 > --- a/include/linux/rculist.h > +++ b/include/linux/rculist.h > @@ -40,6 +40,23 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head > *list) > */ > #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next))) > > +/* > + * Check during list traversal that we are within an RCU reader > + */ > + > +#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6 > +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0) You don't seem to actually use it in this patch; also linux/kernel.h has COUNT_ARGS().
Re: [PATCH v1 1/6] rcu: Add support for consolidated-RCU reader checking
On Thu, Jul 11, 2019 at 07:43:56PM -0400, Joel Fernandes (Google) wrote: > This patch adds support for checking RCU reader sections in list > traversal macros. Optionally, if the list macro is called under SRCU or > other lock/mutex protection, then appropriate lockdep expressions can be > passed to make the checks pass. > > Existing list_for_each_entry_rcu() invocations don't need to pass the > optional fourth argument (cond) unless they are under some non-RCU > protection and needs to make lockdep check pass. > > Signed-off-by: Joel Fernandes (Google) > --- > include/linux/rculist.h | 29 - > include/linux/rcupdate.h | 7 +++ > kernel/rcu/Kconfig.debug | 11 +++ > kernel/rcu/update.c | 26 ++ > 4 files changed, 68 insertions(+), 5 deletions(-) > > diff --git a/include/linux/rculist.h b/include/linux/rculist.h > index e91ec9ddcd30..78c15ec6b2c9 100644 > --- a/include/linux/rculist.h > +++ b/include/linux/rculist.h > @@ -40,6 +40,23 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head > *list) > */ > #define list_next_rcu(list) (*((struct list_head __rcu **)(&(list)->next))) > > +/* > + * Check during list traversal that we are within an RCU reader > + */ > + > +#define SIXTH_ARG(a1, a2, a3, a4, a5, a6, ...) a6 > +#define COUNT_VARGS(...) SIXTH_ARG(dummy, ## __VA_ARGS__, 4, 3, 2, 1, 0) Fyi, I made a cosmetic change by deleting the above 2 unused macros. - Joel
