| Message ID | 20190712170024.111093-4-joel@joelfernandes.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Harden list_for_each_entry_rcu() and family | expand |
On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > The rcu/sync code was doing its own check whether we are in a reader > section. With RCU consolidating flavors and the generic helper added in > this series, this is no longer need. We can just use the generic helper > and it results in a nice cleanup. > > Cc: Oleg Nesterov <oleg@redhat.com> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> Hi Oleg, Slightly unrelated to the patch, I tried hard to understand this comment below in percpu_down_read() but no dice. I do understand how rcu sync and percpu rwsem works, however the comment below didn't make much sense to me. For one, there's no readers_fast anymore so I did not follow what readers_fast means. Could the comment be updated to reflect latest changes? Also could you help understand how is a writer not able to change sem->state and count the per-cpu read counters at the same time as the comment tries to say? /* * We are in an RCU-sched read-side critical section, so the writer * cannot both change sem->state from readers_fast and start checking * counters while we are here. So if we see !sem->state, we know that * the writer won't be checking until we're past the preempt_enable() * and that once the synchronize_rcu() is done, the writer will see * anything we did within this RCU-sched read-size critical section. */ Also, I guess we could get rid of all of the gp_ops struct stuff now that since all the callbacks are the same now. I will post that as a follow-up patch to this series. thanks! - Joel > --- > Please note: Only build and boot tested this particular patch so far. > > include/linux/rcu_sync.h | 5 ++--- > kernel/rcu/sync.c | 22 ---------------------- > 2 files changed, 2 insertions(+), 25 deletions(-) > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > index 6fc53a1345b3..c954f1efc919 100644 > --- a/include/linux/rcu_sync.h > +++ b/include/linux/rcu_sync.h > @@ -39,9 +39,8 @@ extern void rcu_sync_lockdep_assert(struct rcu_sync *); > */ > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > { > -#ifdef CONFIG_PROVE_RCU > - rcu_sync_lockdep_assert(rsp); > -#endif > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), > + "suspicious rcu_sync_is_idle() usage"); > return !rsp->gp_state; /* GP_IDLE */ > } > > diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c > index a8304d90573f..535e02601f56 100644 > --- a/kernel/rcu/sync.c > +++ b/kernel/rcu/sync.c > @@ -10,37 +10,25 @@ > #include <linux/rcu_sync.h> > #include <linux/sched.h> > > -#ifdef CONFIG_PROVE_RCU > -#define __INIT_HELD(func) .held = func, > -#else > -#define __INIT_HELD(func) > -#endif > - > static const struct { > void (*sync)(void); > void (*call)(struct rcu_head *, void (*)(struct rcu_head *)); > void (*wait)(void); > -#ifdef CONFIG_PROVE_RCU > - int (*held)(void); > -#endif > } gp_ops[] = { > [RCU_SYNC] = { > .sync = synchronize_rcu, > .call = call_rcu, > .wait = rcu_barrier, > - __INIT_HELD(rcu_read_lock_held) > }, > [RCU_SCHED_SYNC] = { > .sync = synchronize_rcu, > .call = call_rcu, > .wait = rcu_barrier, > - __INIT_HELD(rcu_read_lock_sched_held) > }, > [RCU_BH_SYNC] = { > .sync = synchronize_rcu, > .call = call_rcu, > .wait = rcu_barrier, > - __INIT_HELD(rcu_read_lock_bh_held) > }, > }; > > @@ -49,16 +37,6 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY }; > > #define rss_lock gp_wait.lock > > -#ifdef CONFIG_PROVE_RCU > -void rcu_sync_lockdep_assert(struct rcu_sync *rsp) > -{ > - RCU_LOCKDEP_WARN(!gp_ops[rsp->gp_type].held(), > - "suspicious rcu_sync_is_idle() usage"); > -} > - > -EXPORT_SYMBOL_GPL(rcu_sync_lockdep_assert); > -#endif > - > /** > * rcu_sync_init() - Initialize an rcu_sync structure > * @rsp: Pointer to rcu_sync structure to be initialized > -- > 2.22.0.510.g264f2c817a-goog >
On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote: > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > > The rcu/sync code was doing its own check whether we are in a reader > > section. With RCU consolidating flavors and the generic helper added in > > this series, this is no longer need. We can just use the generic helper > > and it results in a nice cleanup. > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > Hi Oleg, > Slightly unrelated to the patch, > I tried hard to understand this comment below in percpu_down_read() but no dice. > > I do understand how rcu sync and percpu rwsem works, however the comment > below didn't make much sense to me. For one, there's no readers_fast anymore > so I did not follow what readers_fast means. Could the comment be updated to > reflect latest changes? > Also could you help understand how is a writer not able to change > sem->state and count the per-cpu read counters at the same time as the > comment tries to say? > > /* > * We are in an RCU-sched read-side critical section, so the writer > * cannot both change sem->state from readers_fast and start checking > * counters while we are here. So if we see !sem->state, we know that > * the writer won't be checking until we're past the preempt_enable() > * and that once the synchronize_rcu() is done, the writer will see > * anything we did within this RCU-sched read-size critical section. > */ > > Also, > I guess we could get rid of all of the gp_ops struct stuff now that since all > the callbacks are the same now. I will post that as a follow-up patch to this > series. Hello, Joel, Oleg has a set of patches updating this code that just hit mainline this week. These patches get rid of the code that previously handled RCU's multiple flavors. Or are you looking at current mainline and me just missing your point? Thanx, Paul > thanks! > > - Joel > > > > --- > > Please note: Only build and boot tested this particular patch so far. > > > > include/linux/rcu_sync.h | 5 ++--- > > kernel/rcu/sync.c | 22 ---------------------- > > 2 files changed, 2 insertions(+), 25 deletions(-) > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > > index 6fc53a1345b3..c954f1efc919 100644 > > --- a/include/linux/rcu_sync.h > > +++ b/include/linux/rcu_sync.h > > @@ -39,9 +39,8 @@ extern void rcu_sync_lockdep_assert(struct rcu_sync *); > > */ > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > > { > > -#ifdef CONFIG_PROVE_RCU > > - rcu_sync_lockdep_assert(rsp); > > -#endif > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), > > + "suspicious rcu_sync_is_idle() usage"); > > return !rsp->gp_state; /* GP_IDLE */ > > } > > > > diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c > > index a8304d90573f..535e02601f56 100644 > > --- a/kernel/rcu/sync.c > > +++ b/kernel/rcu/sync.c > > @@ -10,37 +10,25 @@ > > #include <linux/rcu_sync.h> > > #include <linux/sched.h> > > > > -#ifdef CONFIG_PROVE_RCU > > -#define __INIT_HELD(func) .held = func, > > -#else > > -#define __INIT_HELD(func) > > -#endif > > - > > static const struct { > > void (*sync)(void); > > void (*call)(struct rcu_head *, void (*)(struct rcu_head *)); > > void (*wait)(void); > > -#ifdef CONFIG_PROVE_RCU > > - int (*held)(void); > > -#endif > > } gp_ops[] = { > > [RCU_SYNC] = { > > .sync = synchronize_rcu, > > .call = call_rcu, > > .wait = rcu_barrier, > > - __INIT_HELD(rcu_read_lock_held) > > }, > > [RCU_SCHED_SYNC] = { > > .sync = synchronize_rcu, > > .call = call_rcu, > > .wait = rcu_barrier, > > - __INIT_HELD(rcu_read_lock_sched_held) > > }, > > [RCU_BH_SYNC] = { > > .sync = synchronize_rcu, > > .call = call_rcu, > > .wait = rcu_barrier, > > - __INIT_HELD(rcu_read_lock_bh_held) > > }, > > }; > > > > @@ -49,16 +37,6 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY }; > > > > #define rss_lock gp_wait.lock > > > > -#ifdef CONFIG_PROVE_RCU > > -void rcu_sync_lockdep_assert(struct rcu_sync *rsp) > > -{ > > - RCU_LOCKDEP_WARN(!gp_ops[rsp->gp_type].held(), > > - "suspicious rcu_sync_is_idle() usage"); > > -} > > - > > -EXPORT_SYMBOL_GPL(rcu_sync_lockdep_assert); > > -#endif > > - > > /** > > * rcu_sync_init() - Initialize an rcu_sync structure > > * @rsp: Pointer to rcu_sync structure to be initialized > > -- > > 2.22.0.510.g264f2c817a-goog > > >
On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote: > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote: > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > > > The rcu/sync code was doing its own check whether we are in a reader > > > section. With RCU consolidating flavors and the generic helper added in > > > this series, this is no longer need. We can just use the generic helper > > > and it results in a nice cleanup. > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > Hi Oleg, > > Slightly unrelated to the patch, > > I tried hard to understand this comment below in percpu_down_read() but no dice. > > > > I do understand how rcu sync and percpu rwsem works, however the comment > > below didn't make much sense to me. For one, there's no readers_fast anymore > > so I did not follow what readers_fast means. Could the comment be updated to > > reflect latest changes? > > Also could you help understand how is a writer not able to change > > sem->state and count the per-cpu read counters at the same time as the > > comment tries to say? > > > > /* > > * We are in an RCU-sched read-side critical section, so the writer > > * cannot both change sem->state from readers_fast and start checking > > * counters while we are here. So if we see !sem->state, we know that > > * the writer won't be checking until we're past the preempt_enable() > > * and that once the synchronize_rcu() is done, the writer will see > > * anything we did within this RCU-sched read-size critical section. > > */ > > > > Also, > > I guess we could get rid of all of the gp_ops struct stuff now that since all > > the callbacks are the same now. I will post that as a follow-up patch to this > > series. > > Hello, Joel, > > Oleg has a set of patches updating this code that just hit mainline > this week. These patches get rid of the code that previously handled > RCU's multiple flavors. Or are you looking at current mainline and > me just missing your point? > Hi Paul, You are right on point. I have a bad habit of not rebasing my trees. In this case the feature branch of mine in concern was based on v5.1. Needless to say, I need to rebase my tree. Yes, this sync clean up patch does conflict when I rebase, but other patches rebase just fine. The 2 options I see are: 1. Let us drop this patch for now and I resend it later. 2. I resend all patches based on Linus's master branch. thanks, - Joel
On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote: > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote: > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote: > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > section. With RCU consolidating flavors and the generic helper added in > > > > this series, this is no longer need. We can just use the generic helper > > > > and it results in a nice cleanup. > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > Hi Oleg, > > > Slightly unrelated to the patch, > > > I tried hard to understand this comment below in percpu_down_read() but no dice. > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment > > > below didn't make much sense to me. For one, there's no readers_fast anymore > > > so I did not follow what readers_fast means. Could the comment be updated to > > > reflect latest changes? > > > Also could you help understand how is a writer not able to change > > > sem->state and count the per-cpu read counters at the same time as the > > > comment tries to say? > > > > > > /* > > > * We are in an RCU-sched read-side critical section, so the writer > > > * cannot both change sem->state from readers_fast and start checking > > > * counters while we are here. So if we see !sem->state, we know that > > > * the writer won't be checking until we're past the preempt_enable() > > > * and that once the synchronize_rcu() is done, the writer will see > > > * anything we did within this RCU-sched read-size critical section. > > > */ > > > > > > Also, > > > I guess we could get rid of all of the gp_ops struct stuff now that since all > > > the callbacks are the same now. I will post that as a follow-up patch to this > > > series. > > > > Hello, Joel, > > > > Oleg has a set of patches updating this code that just hit mainline > > this week. These patches get rid of the code that previously handled > > RCU's multiple flavors. Or are you looking at current mainline and > > me just missing your point? > > > > Hi Paul, > You are right on point. I have a bad habit of not rebasing my trees. In this > case the feature branch of mine in concern was based on v5.1. Needless to > say, I need to rebase my tree. > > Yes, this sync clean up patch does conflict when I rebase, but other patches > rebase just fine. > > The 2 options I see are: > 1. Let us drop this patch for now and I resend it later. > 2. I resend all patches based on Linus's master branch. Below is the updated patch based on Linus master branch: ---8<----------------------- From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001 From: "Joel Fernandes (Google)" <joel@joelfernandes.org> Date: Fri, 12 Jul 2019 12:13:27 -0400 Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section The rcu/sync code was doing its own check whether we are in a reader section. With RCU consolidating flavors and the generic helper added in this series, this is no longer need. We can just use the generic helper and it results in a nice cleanup. Cc: Oleg Nesterov <oleg@redhat.com> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- include/linux/rcu_sync.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h index 9b83865d24f9..0027d4c8087c 100644 --- a/include/linux/rcu_sync.h +++ b/include/linux/rcu_sync.h @@ -31,9 +31,7 @@ struct rcu_sync { */ static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) { - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && - !rcu_read_lock_bh_held() && - !rcu_read_lock_sched_held(), + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), "suspicious rcu_sync_is_idle() usage"); return !READ_ONCE(rsp->gp_state); /* GP_IDLE */ }
On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote: > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote: > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote: > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote: > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > > section. With RCU consolidating flavors and the generic helper added in > > > > > this series, this is no longer need. We can just use the generic helper > > > > > and it results in a nice cleanup. > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > Hi Oleg, > > > > Slightly unrelated to the patch, > > > > I tried hard to understand this comment below in percpu_down_read() but no dice. > > > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment > > > > below didn't make much sense to me. For one, there's no readers_fast anymore > > > > so I did not follow what readers_fast means. Could the comment be updated to > > > > reflect latest changes? > > > > Also could you help understand how is a writer not able to change > > > > sem->state and count the per-cpu read counters at the same time as the > > > > comment tries to say? > > > > > > > > /* > > > > * We are in an RCU-sched read-side critical section, so the writer > > > > * cannot both change sem->state from readers_fast and start checking > > > > * counters while we are here. So if we see !sem->state, we know that > > > > * the writer won't be checking until we're past the preempt_enable() > > > > * and that once the synchronize_rcu() is done, the writer will see > > > > * anything we did within this RCU-sched read-size critical section. > > > > */ > > > > > > > > Also, > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all > > > > the callbacks are the same now. I will post that as a follow-up patch to this > > > > series. > > > > > > Hello, Joel, > > > > > > Oleg has a set of patches updating this code that just hit mainline > > > this week. These patches get rid of the code that previously handled > > > RCU's multiple flavors. Or are you looking at current mainline and > > > me just missing your point? > > > > > > > Hi Paul, > > You are right on point. I have a bad habit of not rebasing my trees. In this > > case the feature branch of mine in concern was based on v5.1. Needless to > > say, I need to rebase my tree. > > > > Yes, this sync clean up patch does conflict when I rebase, but other patches > > rebase just fine. > > > > The 2 options I see are: > > 1. Let us drop this patch for now and I resend it later. > > 2. I resend all patches based on Linus's master branch. > > Below is the updated patch based on Linus master branch: > > ---8<----------------------- > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001 > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > Date: Fri, 12 Jul 2019 12:13:27 -0400 > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section > > The rcu/sync code was doing its own check whether we are in a reader > section. With RCU consolidating flavors and the generic helper added in > this series, this is no longer need. We can just use the generic helper > and it results in a nice cleanup. > > Cc: Oleg Nesterov <oleg@redhat.com> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > --- > include/linux/rcu_sync.h | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > index 9b83865d24f9..0027d4c8087c 100644 > --- a/include/linux/rcu_sync.h > +++ b/include/linux/rcu_sync.h > @@ -31,9 +31,7 @@ struct rcu_sync { > */ > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > { > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && > - !rcu_read_lock_bh_held() && > - !rcu_read_lock_sched_held(), > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), I believe that replacing rcu_read_lock_sched_held() with preemptible() in a CONFIG_PREEMPT=n kernel will give you false-positive splats here. If you have not already done so, could you please give it a try? Thanx, Paul > "suspicious rcu_sync_is_idle() usage"); > return !READ_ONCE(rsp->gp_state); /* GP_IDLE */ > } > -- > 2.22.0.510.g264f2c817a-goog >
On Sat, Jul 13, 2019 at 01:21:14AM -0700, Paul E. McKenney wrote: > On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote: > > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote: > > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote: > > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote: > > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > > > section. With RCU consolidating flavors and the generic helper added in > > > > > > this series, this is no longer need. We can just use the generic helper > > > > > > and it results in a nice cleanup. > > > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > > > Hi Oleg, > > > > > Slightly unrelated to the patch, > > > > > I tried hard to understand this comment below in percpu_down_read() but no dice. > > > > > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment > > > > > below didn't make much sense to me. For one, there's no readers_fast anymore > > > > > so I did not follow what readers_fast means. Could the comment be updated to > > > > > reflect latest changes? > > > > > Also could you help understand how is a writer not able to change > > > > > sem->state and count the per-cpu read counters at the same time as the > > > > > comment tries to say? > > > > > > > > > > /* > > > > > * We are in an RCU-sched read-side critical section, so the writer > > > > > * cannot both change sem->state from readers_fast and start checking > > > > > * counters while we are here. So if we see !sem->state, we know that > > > > > * the writer won't be checking until we're past the preempt_enable() > > > > > * and that once the synchronize_rcu() is done, the writer will see > > > > > * anything we did within this RCU-sched read-size critical section. > > > > > */ > > > > > > > > > > Also, > > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all > > > > > the callbacks are the same now. I will post that as a follow-up patch to this > > > > > series. > > > > > > > > Hello, Joel, > > > > > > > > Oleg has a set of patches updating this code that just hit mainline > > > > this week. These patches get rid of the code that previously handled > > > > RCU's multiple flavors. Or are you looking at current mainline and > > > > me just missing your point? > > > > > > > > > > Hi Paul, > > > You are right on point. I have a bad habit of not rebasing my trees. In this > > > case the feature branch of mine in concern was based on v5.1. Needless to > > > say, I need to rebase my tree. > > > > > > Yes, this sync clean up patch does conflict when I rebase, but other patches > > > rebase just fine. > > > > > > The 2 options I see are: > > > 1. Let us drop this patch for now and I resend it later. > > > 2. I resend all patches based on Linus's master branch. > > > > Below is the updated patch based on Linus master branch: > > > > ---8<----------------------- > > > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001 > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > Date: Fri, 12 Jul 2019 12:13:27 -0400 > > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section > > > > The rcu/sync code was doing its own check whether we are in a reader > > section. With RCU consolidating flavors and the generic helper added in > > this series, this is no longer need. We can just use the generic helper > > and it results in a nice cleanup. > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > --- > > include/linux/rcu_sync.h | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > > index 9b83865d24f9..0027d4c8087c 100644 > > --- a/include/linux/rcu_sync.h > > +++ b/include/linux/rcu_sync.h > > @@ -31,9 +31,7 @@ struct rcu_sync { > > */ > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > > { > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && > > - !rcu_read_lock_bh_held() && > > - !rcu_read_lock_sched_held(), > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), > > I believe that replacing rcu_read_lock_sched_held() with preemptible() > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here. > If you have not already done so, could you please give it a try? Hi Paul, I don't think it will cause splats for !CONFIG_PREEMPT. Currently, rcu_read_lock_any_held() introduced in this patch returns true if !preemptible(). This means that: The following expression above: RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...) Becomes: RCU_LOCKDEP_WARN(preemptible(), ...) For, CONFIG_PREEMPT=n kernels, this means: RCU_LOCKDEP_WARN(0, ...) Which would mean no splats. Or, did I miss the point? thanks, - Joel
On Sat, Jul 13, 2019 at 09:30:49AM -0400, Joel Fernandes wrote: > On Sat, Jul 13, 2019 at 01:21:14AM -0700, Paul E. McKenney wrote: > > On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote: > > > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote: > > > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote: > > > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote: > > > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > > > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > > > > section. With RCU consolidating flavors and the generic helper added in > > > > > > > this series, this is no longer need. We can just use the generic helper > > > > > > > and it results in a nice cleanup. > > > > > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > > > > > Hi Oleg, > > > > > > Slightly unrelated to the patch, > > > > > > I tried hard to understand this comment below in percpu_down_read() but no dice. > > > > > > > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment > > > > > > below didn't make much sense to me. For one, there's no readers_fast anymore > > > > > > so I did not follow what readers_fast means. Could the comment be updated to > > > > > > reflect latest changes? > > > > > > Also could you help understand how is a writer not able to change > > > > > > sem->state and count the per-cpu read counters at the same time as the > > > > > > comment tries to say? > > > > > > > > > > > > /* > > > > > > * We are in an RCU-sched read-side critical section, so the writer > > > > > > * cannot both change sem->state from readers_fast and start checking > > > > > > * counters while we are here. So if we see !sem->state, we know that > > > > > > * the writer won't be checking until we're past the preempt_enable() > > > > > > * and that once the synchronize_rcu() is done, the writer will see > > > > > > * anything we did within this RCU-sched read-size critical section. > > > > > > */ > > > > > > > > > > > > Also, > > > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all > > > > > > the callbacks are the same now. I will post that as a follow-up patch to this > > > > > > series. > > > > > > > > > > Hello, Joel, > > > > > > > > > > Oleg has a set of patches updating this code that just hit mainline > > > > > this week. These patches get rid of the code that previously handled > > > > > RCU's multiple flavors. Or are you looking at current mainline and > > > > > me just missing your point? > > > > > > > > > > > > > Hi Paul, > > > > You are right on point. I have a bad habit of not rebasing my trees. In this > > > > case the feature branch of mine in concern was based on v5.1. Needless to > > > > say, I need to rebase my tree. > > > > > > > > Yes, this sync clean up patch does conflict when I rebase, but other patches > > > > rebase just fine. > > > > > > > > The 2 options I see are: > > > > 1. Let us drop this patch for now and I resend it later. > > > > 2. I resend all patches based on Linus's master branch. > > > > > > Below is the updated patch based on Linus master branch: > > > > > > ---8<----------------------- > > > > > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001 > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > > Date: Fri, 12 Jul 2019 12:13:27 -0400 > > > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section > > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > section. With RCU consolidating flavors and the generic helper added in > > > this series, this is no longer need. We can just use the generic helper > > > and it results in a nice cleanup. > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > --- > > > include/linux/rcu_sync.h | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > > > index 9b83865d24f9..0027d4c8087c 100644 > > > --- a/include/linux/rcu_sync.h > > > +++ b/include/linux/rcu_sync.h > > > @@ -31,9 +31,7 @@ struct rcu_sync { > > > */ > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > > > { > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && > > > - !rcu_read_lock_bh_held() && > > > - !rcu_read_lock_sched_held(), > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), > > > > I believe that replacing rcu_read_lock_sched_held() with preemptible() > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here. > > If you have not already done so, could you please give it a try? > > Hi Paul, > I don't think it will cause splats for !CONFIG_PREEMPT. > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if > !preemptible(). This means that: > > The following expression above: > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...) > > Becomes: > RCU_LOCKDEP_WARN(preemptible(), ...) > > For, CONFIG_PREEMPT=n kernels, this means: > RCU_LOCKDEP_WARN(0, ...) > > Which would mean no splats. Or, did I miss the point? I suggest trying it out on a CONFIG_PREEMPT=n kernel. Thanx, Paul
On Sat, Jul 13, 2019 at 07:41:08AM -0700, Paul E. McKenney wrote: > On Sat, Jul 13, 2019 at 09:30:49AM -0400, Joel Fernandes wrote: > > On Sat, Jul 13, 2019 at 01:21:14AM -0700, Paul E. McKenney wrote: > > > On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote: > > > > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote: > > > > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote: > > > > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote: > > > > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > > > > > section. With RCU consolidating flavors and the generic helper added in > > > > > > > > this series, this is no longer need. We can just use the generic helper > > > > > > > > and it results in a nice cleanup. > > > > > > > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > > > > > > > Hi Oleg, > > > > > > > Slightly unrelated to the patch, > > > > > > > I tried hard to understand this comment below in percpu_down_read() but no dice. > > > > > > > > > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment > > > > > > > below didn't make much sense to me. For one, there's no readers_fast anymore > > > > > > > so I did not follow what readers_fast means. Could the comment be updated to > > > > > > > reflect latest changes? > > > > > > > Also could you help understand how is a writer not able to change > > > > > > > sem->state and count the per-cpu read counters at the same time as the > > > > > > > comment tries to say? > > > > > > > > > > > > > > /* > > > > > > > * We are in an RCU-sched read-side critical section, so the writer > > > > > > > * cannot both change sem->state from readers_fast and start checking > > > > > > > * counters while we are here. So if we see !sem->state, we know that > > > > > > > * the writer won't be checking until we're past the preempt_enable() > > > > > > > * and that once the synchronize_rcu() is done, the writer will see > > > > > > > * anything we did within this RCU-sched read-size critical section. > > > > > > > */ > > > > > > > > > > > > > > Also, > > > > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all > > > > > > > the callbacks are the same now. I will post that as a follow-up patch to this > > > > > > > series. > > > > > > > > > > > > Hello, Joel, > > > > > > > > > > > > Oleg has a set of patches updating this code that just hit mainline > > > > > > this week. These patches get rid of the code that previously handled > > > > > > RCU's multiple flavors. Or are you looking at current mainline and > > > > > > me just missing your point? > > > > > > > > > > > > > > > > Hi Paul, > > > > > You are right on point. I have a bad habit of not rebasing my trees. In this > > > > > case the feature branch of mine in concern was based on v5.1. Needless to > > > > > say, I need to rebase my tree. > > > > > > > > > > Yes, this sync clean up patch does conflict when I rebase, but other patches > > > > > rebase just fine. > > > > > > > > > > The 2 options I see are: > > > > > 1. Let us drop this patch for now and I resend it later. > > > > > 2. I resend all patches based on Linus's master branch. > > > > > > > > Below is the updated patch based on Linus master branch: > > > > > > > > ---8<----------------------- > > > > > > > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001 > > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > > > Date: Fri, 12 Jul 2019 12:13:27 -0400 > > > > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > section. With RCU consolidating flavors and the generic helper added in > > > > this series, this is no longer need. We can just use the generic helper > > > > and it results in a nice cleanup. > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > --- > > > > include/linux/rcu_sync.h | 4 +--- > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > > > > index 9b83865d24f9..0027d4c8087c 100644 > > > > --- a/include/linux/rcu_sync.h > > > > +++ b/include/linux/rcu_sync.h > > > > @@ -31,9 +31,7 @@ struct rcu_sync { > > > > */ > > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > > > > { > > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && > > > > - !rcu_read_lock_bh_held() && > > > > - !rcu_read_lock_sched_held(), > > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), > > > > > > I believe that replacing rcu_read_lock_sched_held() with preemptible() > > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here. > > > If you have not already done so, could you please give it a try? > > > > Hi Paul, > > I don't think it will cause splats for !CONFIG_PREEMPT. > > > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if > > !preemptible(). This means that: > > > > The following expression above: > > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...) > > > > Becomes: > > RCU_LOCKDEP_WARN(preemptible(), ...) > > > > For, CONFIG_PREEMPT=n kernels, this means: > > RCU_LOCKDEP_WARN(0, ...) > > > > Which would mean no splats. Or, did I miss the point? > > I suggest trying it out on a CONFIG_PREEMPT=n kernel. Sure, will do, sorry did not try it out yet because was busy with weekend chores but will do soon, thanks!
On Sat, Jul 13, 2019 at 11:36:06AM -0400, Joel Fernandes wrote: > On Sat, Jul 13, 2019 at 07:41:08AM -0700, Paul E. McKenney wrote: > > On Sat, Jul 13, 2019 at 09:30:49AM -0400, Joel Fernandes wrote: > > > On Sat, Jul 13, 2019 at 01:21:14AM -0700, Paul E. McKenney wrote: > > > > On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote: > > > > > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote: > > > > > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote: > > > > > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote: > > > > > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > > > > > > section. With RCU consolidating flavors and the generic helper added in > > > > > > > > > this series, this is no longer need. We can just use the generic helper > > > > > > > > > and it results in a nice cleanup. > > > > > > > > > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > > > > > > > > > Hi Oleg, > > > > > > > > Slightly unrelated to the patch, > > > > > > > > I tried hard to understand this comment below in percpu_down_read() but no dice. > > > > > > > > > > > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment > > > > > > > > below didn't make much sense to me. For one, there's no readers_fast anymore > > > > > > > > so I did not follow what readers_fast means. Could the comment be updated to > > > > > > > > reflect latest changes? > > > > > > > > Also could you help understand how is a writer not able to change > > > > > > > > sem->state and count the per-cpu read counters at the same time as the > > > > > > > > comment tries to say? > > > > > > > > > > > > > > > > /* > > > > > > > > * We are in an RCU-sched read-side critical section, so the writer > > > > > > > > * cannot both change sem->state from readers_fast and start checking > > > > > > > > * counters while we are here. So if we see !sem->state, we know that > > > > > > > > * the writer won't be checking until we're past the preempt_enable() > > > > > > > > * and that once the synchronize_rcu() is done, the writer will see > > > > > > > > * anything we did within this RCU-sched read-size critical section. > > > > > > > > */ > > > > > > > > > > > > > > > > Also, > > > > > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all > > > > > > > > the callbacks are the same now. I will post that as a follow-up patch to this > > > > > > > > series. > > > > > > > > > > > > > > Hello, Joel, > > > > > > > > > > > > > > Oleg has a set of patches updating this code that just hit mainline > > > > > > > this week. These patches get rid of the code that previously handled > > > > > > > RCU's multiple flavors. Or are you looking at current mainline and > > > > > > > me just missing your point? > > > > > > > > > > > > > > > > > > > Hi Paul, > > > > > > You are right on point. I have a bad habit of not rebasing my trees. In this > > > > > > case the feature branch of mine in concern was based on v5.1. Needless to > > > > > > say, I need to rebase my tree. > > > > > > > > > > > > Yes, this sync clean up patch does conflict when I rebase, but other patches > > > > > > rebase just fine. > > > > > > > > > > > > The 2 options I see are: > > > > > > 1. Let us drop this patch for now and I resend it later. > > > > > > 2. I resend all patches based on Linus's master branch. > > > > > > > > > > Below is the updated patch based on Linus master branch: > > > > > > > > > > ---8<----------------------- > > > > > > > > > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001 > > > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > > > > Date: Fri, 12 Jul 2019 12:13:27 -0400 > > > > > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section > > > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > > section. With RCU consolidating flavors and the generic helper added in > > > > > this series, this is no longer need. We can just use the generic helper > > > > > and it results in a nice cleanup. > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > --- > > > > > include/linux/rcu_sync.h | 4 +--- > > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > > > > > index 9b83865d24f9..0027d4c8087c 100644 > > > > > --- a/include/linux/rcu_sync.h > > > > > +++ b/include/linux/rcu_sync.h > > > > > @@ -31,9 +31,7 @@ struct rcu_sync { > > > > > */ > > > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > > > > > { > > > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && > > > > > - !rcu_read_lock_bh_held() && > > > > > - !rcu_read_lock_sched_held(), > > > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), > > > > > > > > I believe that replacing rcu_read_lock_sched_held() with preemptible() > > > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here. > > > > If you have not already done so, could you please give it a try? > > > > > > Hi Paul, > > > I don't think it will cause splats for !CONFIG_PREEMPT. > > > > > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if > > > !preemptible(). This means that: > > > > > > The following expression above: > > > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...) > > > > > > Becomes: > > > RCU_LOCKDEP_WARN(preemptible(), ...) > > > > > > For, CONFIG_PREEMPT=n kernels, this means: > > > RCU_LOCKDEP_WARN(0, ...) > > > > > > Which would mean no splats. Or, did I miss the point? > > > > I suggest trying it out on a CONFIG_PREEMPT=n kernel. > > Sure, will do, sorry did not try it out yet because was busy with weekend > chores but will do soon, thanks! I am not faulting you for taking the weekend off, actually. ;-) Thanx, Paul
On Sat, Jul 13, 2019 at 08:50:10AM -0700, Paul E. McKenney wrote: > On Sat, Jul 13, 2019 at 11:36:06AM -0400, Joel Fernandes wrote: > > On Sat, Jul 13, 2019 at 07:41:08AM -0700, Paul E. McKenney wrote: > > > On Sat, Jul 13, 2019 at 09:30:49AM -0400, Joel Fernandes wrote: > > > > On Sat, Jul 13, 2019 at 01:21:14AM -0700, Paul E. McKenney wrote: > > > > > On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote: > > > > > > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote: > > > > > > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote: > > > > > > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote: > > > > > > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > > > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > > > > > > > section. With RCU consolidating flavors and the generic helper added in > > > > > > > > > > this series, this is no longer need. We can just use the generic helper > > > > > > > > > > and it results in a nice cleanup. > > > > > > > > > > > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > > > > > > > > > > > Hi Oleg, > > > > > > > > > Slightly unrelated to the patch, > > > > > > > > > I tried hard to understand this comment below in percpu_down_read() but no dice. > > > > > > > > > > > > > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment > > > > > > > > > below didn't make much sense to me. For one, there's no readers_fast anymore > > > > > > > > > so I did not follow what readers_fast means. Could the comment be updated to > > > > > > > > > reflect latest changes? > > > > > > > > > Also could you help understand how is a writer not able to change > > > > > > > > > sem->state and count the per-cpu read counters at the same time as the > > > > > > > > > comment tries to say? > > > > > > > > > > > > > > > > > > /* > > > > > > > > > * We are in an RCU-sched read-side critical section, so the writer > > > > > > > > > * cannot both change sem->state from readers_fast and start checking > > > > > > > > > * counters while we are here. So if we see !sem->state, we know that > > > > > > > > > * the writer won't be checking until we're past the preempt_enable() > > > > > > > > > * and that once the synchronize_rcu() is done, the writer will see > > > > > > > > > * anything we did within this RCU-sched read-size critical section. > > > > > > > > > */ > > > > > > > > > > > > > > > > > > Also, > > > > > > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all > > > > > > > > > the callbacks are the same now. I will post that as a follow-up patch to this > > > > > > > > > series. > > > > > > > > > > > > > > > > Hello, Joel, > > > > > > > > > > > > > > > > Oleg has a set of patches updating this code that just hit mainline > > > > > > > > this week. These patches get rid of the code that previously handled > > > > > > > > RCU's multiple flavors. Or are you looking at current mainline and > > > > > > > > me just missing your point? > > > > > > > > > > > > > > > > > > > > > > Hi Paul, > > > > > > > You are right on point. I have a bad habit of not rebasing my trees. In this > > > > > > > case the feature branch of mine in concern was based on v5.1. Needless to > > > > > > > say, I need to rebase my tree. > > > > > > > > > > > > > > Yes, this sync clean up patch does conflict when I rebase, but other patches > > > > > > > rebase just fine. > > > > > > > > > > > > > > The 2 options I see are: > > > > > > > 1. Let us drop this patch for now and I resend it later. > > > > > > > 2. I resend all patches based on Linus's master branch. > > > > > > > > > > > > Below is the updated patch based on Linus master branch: > > > > > > > > > > > > ---8<----------------------- > > > > > > > > > > > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001 > > > > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > > > > > Date: Fri, 12 Jul 2019 12:13:27 -0400 > > > > > > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section > > > > > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > > > section. With RCU consolidating flavors and the generic helper added in > > > > > > this series, this is no longer need. We can just use the generic helper > > > > > > and it results in a nice cleanup. > > > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > --- > > > > > > include/linux/rcu_sync.h | 4 +--- > > > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > > > > > > index 9b83865d24f9..0027d4c8087c 100644 > > > > > > --- a/include/linux/rcu_sync.h > > > > > > +++ b/include/linux/rcu_sync.h > > > > > > @@ -31,9 +31,7 @@ struct rcu_sync { > > > > > > */ > > > > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > > > > > > { > > > > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && > > > > > > - !rcu_read_lock_bh_held() && > > > > > > - !rcu_read_lock_sched_held(), > > > > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), > > > > > > > > > > I believe that replacing rcu_read_lock_sched_held() with preemptible() > > > > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here. > > > > > If you have not already done so, could you please give it a try? > > > > > > > > Hi Paul, > > > > I don't think it will cause splats for !CONFIG_PREEMPT. > > > > > > > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if > > > > !preemptible(). This means that: > > > > > > > > The following expression above: > > > > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...) > > > > > > > > Becomes: > > > > RCU_LOCKDEP_WARN(preemptible(), ...) > > > > > > > > For, CONFIG_PREEMPT=n kernels, this means: > > > > RCU_LOCKDEP_WARN(0, ...) > > > > > > > > Which would mean no splats. Or, did I miss the point? > > > > > > I suggest trying it out on a CONFIG_PREEMPT=n kernel. > > > > Sure, will do, sorry did not try it out yet because was busy with weekend > > chores but will do soon, thanks! > > I am not faulting you for taking the weekend off, actually. ;-) ;-) I tried doing RCU_LOCKDEP_WARN(preemptible(), ...) in this code path and I don't get any splats. I also disassembled the code and it seems to me RCU_LOCKDEP_WARN() becomes a NOOP which also the above reasoning confirms. thanks, - Joel
On Sat, Jul 13, 2019 at 12:13:16PM -0400, Joel Fernandes wrote: > On Sat, Jul 13, 2019 at 08:50:10AM -0700, Paul E. McKenney wrote: > > On Sat, Jul 13, 2019 at 11:36:06AM -0400, Joel Fernandes wrote: > > > On Sat, Jul 13, 2019 at 07:41:08AM -0700, Paul E. McKenney wrote: > > > > On Sat, Jul 13, 2019 at 09:30:49AM -0400, Joel Fernandes wrote: > > > > > On Sat, Jul 13, 2019 at 01:21:14AM -0700, Paul E. McKenney wrote: > > > > > > On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote: > > > > > > > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote: > > > > > > > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote: > > > > > > > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote: > > > > > > > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > > > > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > > > > > > > > section. With RCU consolidating flavors and the generic helper added in > > > > > > > > > > > this series, this is no longer need. We can just use the generic helper > > > > > > > > > > > and it results in a nice cleanup. > > > > > > > > > > > > > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > > > > > > > > > > > > > Hi Oleg, > > > > > > > > > > Slightly unrelated to the patch, > > > > > > > > > > I tried hard to understand this comment below in percpu_down_read() but no dice. > > > > > > > > > > > > > > > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment > > > > > > > > > > below didn't make much sense to me. For one, there's no readers_fast anymore > > > > > > > > > > so I did not follow what readers_fast means. Could the comment be updated to > > > > > > > > > > reflect latest changes? > > > > > > > > > > Also could you help understand how is a writer not able to change > > > > > > > > > > sem->state and count the per-cpu read counters at the same time as the > > > > > > > > > > comment tries to say? > > > > > > > > > > > > > > > > > > > > /* > > > > > > > > > > * We are in an RCU-sched read-side critical section, so the writer > > > > > > > > > > * cannot both change sem->state from readers_fast and start checking > > > > > > > > > > * counters while we are here. So if we see !sem->state, we know that > > > > > > > > > > * the writer won't be checking until we're past the preempt_enable() > > > > > > > > > > * and that once the synchronize_rcu() is done, the writer will see > > > > > > > > > > * anything we did within this RCU-sched read-size critical section. > > > > > > > > > > */ > > > > > > > > > > > > > > > > > > > > Also, > > > > > > > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all > > > > > > > > > > the callbacks are the same now. I will post that as a follow-up patch to this > > > > > > > > > > series. > > > > > > > > > > > > > > > > > > Hello, Joel, > > > > > > > > > > > > > > > > > > Oleg has a set of patches updating this code that just hit mainline > > > > > > > > > this week. These patches get rid of the code that previously handled > > > > > > > > > RCU's multiple flavors. Or are you looking at current mainline and > > > > > > > > > me just missing your point? > > > > > > > > > > > > > > > > > > > > > > > > > Hi Paul, > > > > > > > > You are right on point. I have a bad habit of not rebasing my trees. In this > > > > > > > > case the feature branch of mine in concern was based on v5.1. Needless to > > > > > > > > say, I need to rebase my tree. > > > > > > > > > > > > > > > > Yes, this sync clean up patch does conflict when I rebase, but other patches > > > > > > > > rebase just fine. > > > > > > > > > > > > > > > > The 2 options I see are: > > > > > > > > 1. Let us drop this patch for now and I resend it later. > > > > > > > > 2. I resend all patches based on Linus's master branch. > > > > > > > > > > > > > > Below is the updated patch based on Linus master branch: > > > > > > > > > > > > > > ---8<----------------------- > > > > > > > > > > > > > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001 > > > > > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > > > > > > Date: Fri, 12 Jul 2019 12:13:27 -0400 > > > > > > > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section > > > > > > > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > > > > section. With RCU consolidating flavors and the generic helper added in > > > > > > > this series, this is no longer need. We can just use the generic helper > > > > > > > and it results in a nice cleanup. > > > > > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > --- > > > > > > > include/linux/rcu_sync.h | 4 +--- > > > > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > > > > > > > index 9b83865d24f9..0027d4c8087c 100644 > > > > > > > --- a/include/linux/rcu_sync.h > > > > > > > +++ b/include/linux/rcu_sync.h > > > > > > > @@ -31,9 +31,7 @@ struct rcu_sync { > > > > > > > */ > > > > > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > > > > > > > { > > > > > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && > > > > > > > - !rcu_read_lock_bh_held() && > > > > > > > - !rcu_read_lock_sched_held(), > > > > > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), > > > > > > > > > > > > I believe that replacing rcu_read_lock_sched_held() with preemptible() > > > > > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here. > > > > > > If you have not already done so, could you please give it a try? > > > > > > > > > > Hi Paul, > > > > > I don't think it will cause splats for !CONFIG_PREEMPT. > > > > > > > > > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if > > > > > !preemptible(). This means that: > > > > > > > > > > The following expression above: > > > > > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...) > > > > > > > > > > Becomes: > > > > > RCU_LOCKDEP_WARN(preemptible(), ...) > > > > > > > > > > For, CONFIG_PREEMPT=n kernels, this means: > > > > > RCU_LOCKDEP_WARN(0, ...) > > > > > > > > > > Which would mean no splats. Or, did I miss the point? > > > > > > > > I suggest trying it out on a CONFIG_PREEMPT=n kernel. > > > > > > Sure, will do, sorry did not try it out yet because was busy with weekend > > > chores but will do soon, thanks! > > > > I am not faulting you for taking the weekend off, actually. ;-) > > ;-) > > I tried doing RCU_LOCKDEP_WARN(preemptible(), ...) in this code path and I > don't get any splats. I also disassembled the code and it seems to me > RCU_LOCKDEP_WARN() becomes a NOOP which also the above reasoning confirms. OK, very good. Could you do the same thing for the RCU_LOCKDEP_WARN() in synchronize_rcu()? Why or why not? (No need to work this on your Sunday.) Thanx, Paul
On Sat, Jul 13, 2019 at 02:28:12PM -0700, Paul E. McKenney wrote: > On Sat, Jul 13, 2019 at 12:13:16PM -0400, Joel Fernandes wrote: > > On Sat, Jul 13, 2019 at 08:50:10AM -0700, Paul E. McKenney wrote: > > > On Sat, Jul 13, 2019 at 11:36:06AM -0400, Joel Fernandes wrote: > > > > On Sat, Jul 13, 2019 at 07:41:08AM -0700, Paul E. McKenney wrote: > > > > > On Sat, Jul 13, 2019 at 09:30:49AM -0400, Joel Fernandes wrote: > > > > > > On Sat, Jul 13, 2019 at 01:21:14AM -0700, Paul E. McKenney wrote: > > > > > > > On Fri, Jul 12, 2019 at 11:10:08PM -0400, Joel Fernandes wrote: > > > > > > > > On Fri, Jul 12, 2019 at 11:01:50PM -0400, Joel Fernandes wrote: > > > > > > > > > On Fri, Jul 12, 2019 at 04:32:06PM -0700, Paul E. McKenney wrote: > > > > > > > > > > On Fri, Jul 12, 2019 at 05:35:59PM -0400, Joel Fernandes wrote: > > > > > > > > > > > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > > > > > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > > > > > > > > > section. With RCU consolidating flavors and the generic helper added in > > > > > > > > > > > > this series, this is no longer need. We can just use the generic helper > > > > > > > > > > > > and it results in a nice cleanup. > > > > > > > > > > > > > > > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > > > > > > > > > > > > > > > Hi Oleg, > > > > > > > > > > > Slightly unrelated to the patch, > > > > > > > > > > > I tried hard to understand this comment below in percpu_down_read() but no dice. > > > > > > > > > > > > > > > > > > > > > > I do understand how rcu sync and percpu rwsem works, however the comment > > > > > > > > > > > below didn't make much sense to me. For one, there's no readers_fast anymore > > > > > > > > > > > so I did not follow what readers_fast means. Could the comment be updated to > > > > > > > > > > > reflect latest changes? > > > > > > > > > > > Also could you help understand how is a writer not able to change > > > > > > > > > > > sem->state and count the per-cpu read counters at the same time as the > > > > > > > > > > > comment tries to say? > > > > > > > > > > > > > > > > > > > > > > /* > > > > > > > > > > > * We are in an RCU-sched read-side critical section, so the writer > > > > > > > > > > > * cannot both change sem->state from readers_fast and start checking > > > > > > > > > > > * counters while we are here. So if we see !sem->state, we know that > > > > > > > > > > > * the writer won't be checking until we're past the preempt_enable() > > > > > > > > > > > * and that once the synchronize_rcu() is done, the writer will see > > > > > > > > > > > * anything we did within this RCU-sched read-size critical section. > > > > > > > > > > > */ > > > > > > > > > > > > > > > > > > > > > > Also, > > > > > > > > > > > I guess we could get rid of all of the gp_ops struct stuff now that since all > > > > > > > > > > > the callbacks are the same now. I will post that as a follow-up patch to this > > > > > > > > > > > series. > > > > > > > > > > > > > > > > > > > > Hello, Joel, > > > > > > > > > > > > > > > > > > > > Oleg has a set of patches updating this code that just hit mainline > > > > > > > > > > this week. These patches get rid of the code that previously handled > > > > > > > > > > RCU's multiple flavors. Or are you looking at current mainline and > > > > > > > > > > me just missing your point? > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Paul, > > > > > > > > > You are right on point. I have a bad habit of not rebasing my trees. In this > > > > > > > > > case the feature branch of mine in concern was based on v5.1. Needless to > > > > > > > > > say, I need to rebase my tree. > > > > > > > > > > > > > > > > > > Yes, this sync clean up patch does conflict when I rebase, but other patches > > > > > > > > > rebase just fine. > > > > > > > > > > > > > > > > > > The 2 options I see are: > > > > > > > > > 1. Let us drop this patch for now and I resend it later. > > > > > > > > > 2. I resend all patches based on Linus's master branch. > > > > > > > > > > > > > > > > Below is the updated patch based on Linus master branch: > > > > > > > > > > > > > > > > ---8<----------------------- > > > > > > > > > > > > > > > > >From 5f40c9a07fcf3d6dafc2189599d0ba9443097d0f Mon Sep 17 00:00:00 2001 > > > > > > > > From: "Joel Fernandes (Google)" <joel@joelfernandes.org> > > > > > > > > Date: Fri, 12 Jul 2019 12:13:27 -0400 > > > > > > > > Subject: [PATCH v2.1 3/9] rcu/sync: Remove custom check for reader-section > > > > > > > > > > > > > > > > The rcu/sync code was doing its own check whether we are in a reader > > > > > > > > section. With RCU consolidating flavors and the generic helper added in > > > > > > > > this series, this is no longer need. We can just use the generic helper > > > > > > > > and it results in a nice cleanup. > > > > > > > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > --- > > > > > > > > include/linux/rcu_sync.h | 4 +--- > > > > > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > > > > > > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > > > > > > > > index 9b83865d24f9..0027d4c8087c 100644 > > > > > > > > --- a/include/linux/rcu_sync.h > > > > > > > > +++ b/include/linux/rcu_sync.h > > > > > > > > @@ -31,9 +31,7 @@ struct rcu_sync { > > > > > > > > */ > > > > > > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > > > > > > > > { > > > > > > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && > > > > > > > > - !rcu_read_lock_bh_held() && > > > > > > > > - !rcu_read_lock_sched_held(), > > > > > > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), > > > > > > > > > > > > > > I believe that replacing rcu_read_lock_sched_held() with preemptible() > > > > > > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here. > > > > > > > If you have not already done so, could you please give it a try? > > > > > > > > > > > > Hi Paul, > > > > > > I don't think it will cause splats for !CONFIG_PREEMPT. > > > > > > > > > > > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if > > > > > > !preemptible(). This means that: > > > > > > > > > > > > The following expression above: > > > > > > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...) > > > > > > > > > > > > Becomes: > > > > > > RCU_LOCKDEP_WARN(preemptible(), ...) > > > > > > > > > > > > For, CONFIG_PREEMPT=n kernels, this means: > > > > > > RCU_LOCKDEP_WARN(0, ...) > > > > > > > > > > > > Which would mean no splats. Or, did I miss the point? > > > > > > > > > > I suggest trying it out on a CONFIG_PREEMPT=n kernel. > > > > > > > > Sure, will do, sorry did not try it out yet because was busy with weekend > > > > chores but will do soon, thanks! > > > > > > I am not faulting you for taking the weekend off, actually. ;-) > > > > ;-) > > > > I tried doing RCU_LOCKDEP_WARN(preemptible(), ...) in this code path and I > > don't get any splats. I also disassembled the code and it seems to me > > RCU_LOCKDEP_WARN() becomes a NOOP which also the above reasoning confirms. > > OK, very good. Could you do the same thing for the RCU_LOCKDEP_WARN() > in synchronize_rcu()? Why or why not? > Hi Paul, Yes synchronize_rcu() can also make use of this technique since it is strictly illegal to call synchronize_rcu() within a reader section. I will add this to the set of my patches as well and send them all out next week, along with the rcu-sync and bh clean ups we discussed. thanks, - Joel
On Sun, Jul 14, 2019 at 02:10:53PM -0400, Joel Fernandes wrote: > On Sat, Jul 13, 2019 at 02:28:12PM -0700, Paul E. McKenney wrote: [snip] > > > > > > > > > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > > --- > > > > > > > > > include/linux/rcu_sync.h | 4 +--- > > > > > > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > > > > > > > > > index 9b83865d24f9..0027d4c8087c 100644 > > > > > > > > > --- a/include/linux/rcu_sync.h > > > > > > > > > +++ b/include/linux/rcu_sync.h > > > > > > > > > @@ -31,9 +31,7 @@ struct rcu_sync { > > > > > > > > > */ > > > > > > > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > > > > > > > > > { > > > > > > > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && > > > > > > > > > - !rcu_read_lock_bh_held() && > > > > > > > > > - !rcu_read_lock_sched_held(), > > > > > > > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), > > > > > > > > > > > > > > > > I believe that replacing rcu_read_lock_sched_held() with preemptible() > > > > > > > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here. > > > > > > > > If you have not already done so, could you please give it a try? > > > > > > > > > > > > > > Hi Paul, > > > > > > > I don't think it will cause splats for !CONFIG_PREEMPT. > > > > > > > > > > > > > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if > > > > > > > !preemptible(). This means that: > > > > > > > > > > > > > > The following expression above: > > > > > > > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...) > > > > > > > > > > > > > > Becomes: > > > > > > > RCU_LOCKDEP_WARN(preemptible(), ...) > > > > > > > > > > > > > > For, CONFIG_PREEMPT=n kernels, this means: > > > > > > > RCU_LOCKDEP_WARN(0, ...) > > > > > > > > > > > > > > Which would mean no splats. Or, did I miss the point? > > > > > > > > > > > > I suggest trying it out on a CONFIG_PREEMPT=n kernel. > > > > > > > > > > Sure, will do, sorry did not try it out yet because was busy with weekend > > > > > chores but will do soon, thanks! > > > > > > > > I am not faulting you for taking the weekend off, actually. ;-) > > > > > > ;-) > > > > > > I tried doing RCU_LOCKDEP_WARN(preemptible(), ...) in this code path and I > > > don't get any splats. I also disassembled the code and it seems to me > > > RCU_LOCKDEP_WARN() becomes a NOOP which also the above reasoning confirms. > > > > OK, very good. Could you do the same thing for the RCU_LOCKDEP_WARN() > > in synchronize_rcu()? Why or why not? > > > > Hi Paul, > > Yes synchronize_rcu() can also make use of this technique since it is > strictly illegal to call synchronize_rcu() within a reader section. > > I will add this to the set of my patches as well and send them all out next > week, along with the rcu-sync and bh clean ups we discussed. After sending this email, it occurs to me it wont work in synchronize_rcu() for !CONFIG_PREEMPT kernels. This is because in a !CONFIG_PREEMPT kernel, executing in kernel mode itself looks like being in an RCU reader. So we should leave that as is. However it will work fine for rcu_sync_is_idle (for CONFIG_PREEMPT=n kernels) as I mentioned earlier. Were trying to throw me a Quick-Quiz ? ;-) In that case, hope I passed! thanks, - Joel
On Sun, Jul 14, 2019 at 02:38:20PM -0400, Joel Fernandes wrote: > On Sun, Jul 14, 2019 at 02:10:53PM -0400, Joel Fernandes wrote: > > On Sat, Jul 13, 2019 at 02:28:12PM -0700, Paul E. McKenney wrote: > [snip] > > > > > > > > > > > > > > > > > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > > > > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > > > > > > > > > --- > > > > > > > > > > include/linux/rcu_sync.h | 4 +--- > > > > > > > > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > > > > > > > > > > index 9b83865d24f9..0027d4c8087c 100644 > > > > > > > > > > --- a/include/linux/rcu_sync.h > > > > > > > > > > +++ b/include/linux/rcu_sync.h > > > > > > > > > > @@ -31,9 +31,7 @@ struct rcu_sync { > > > > > > > > > > */ > > > > > > > > > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > > > > > > > > > > { > > > > > > > > > > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && > > > > > > > > > > - !rcu_read_lock_bh_held() && > > > > > > > > > > - !rcu_read_lock_sched_held(), > > > > > > > > > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), > > > > > > > > > > > > > > > > > > I believe that replacing rcu_read_lock_sched_held() with preemptible() > > > > > > > > > in a CONFIG_PREEMPT=n kernel will give you false-positive splats here. > > > > > > > > > If you have not already done so, could you please give it a try? > > > > > > > > > > > > > > > > Hi Paul, > > > > > > > > I don't think it will cause splats for !CONFIG_PREEMPT. > > > > > > > > > > > > > > > > Currently, rcu_read_lock_any_held() introduced in this patch returns true if > > > > > > > > !preemptible(). This means that: > > > > > > > > > > > > > > > > The following expression above: > > > > > > > > RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(),...) > > > > > > > > > > > > > > > > Becomes: > > > > > > > > RCU_LOCKDEP_WARN(preemptible(), ...) > > > > > > > > > > > > > > > > For, CONFIG_PREEMPT=n kernels, this means: > > > > > > > > RCU_LOCKDEP_WARN(0, ...) > > > > > > > > > > > > > > > > Which would mean no splats. Or, did I miss the point? > > > > > > > > > > > > > > I suggest trying it out on a CONFIG_PREEMPT=n kernel. > > > > > > > > > > > > Sure, will do, sorry did not try it out yet because was busy with weekend > > > > > > chores but will do soon, thanks! > > > > > > > > > > I am not faulting you for taking the weekend off, actually. ;-) > > > > > > > > ;-) > > > > > > > > I tried doing RCU_LOCKDEP_WARN(preemptible(), ...) in this code path and I > > > > don't get any splats. I also disassembled the code and it seems to me > > > > RCU_LOCKDEP_WARN() becomes a NOOP which also the above reasoning confirms. > > > > > > OK, very good. Could you do the same thing for the RCU_LOCKDEP_WARN() > > > in synchronize_rcu()? Why or why not? > > > > > > > Hi Paul, > > > > Yes synchronize_rcu() can also make use of this technique since it is > > strictly illegal to call synchronize_rcu() within a reader section. > > > > I will add this to the set of my patches as well and send them all out next > > week, along with the rcu-sync and bh clean ups we discussed. > > After sending this email, it occurs to me it wont work in synchronize_rcu() > for !CONFIG_PREEMPT kernels. This is because in a !CONFIG_PREEMPT kernel, > executing in kernel mode itself looks like being in an RCU reader. So we > should leave that as is. However it will work fine for rcu_sync_is_idle (for > CONFIG_PREEMPT=n kernels) as I mentioned earlier. > > Were trying to throw me a Quick-Quiz ? ;-) In that case, hope I passed! You did pass. This time. ;-) Thanx, Paul
On 07/12, Joel Fernandes wrote: > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > { > - RCU_LOCKDEP_WARN(!rcu_read_lock_held() && > - !rcu_read_lock_bh_held() && > - !rcu_read_lock_sched_held(), > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), Yes, this is what I meant. Sorry for confusion, I should have mentioned that rcu_sync_is_idle() was recently updated when I suggested to use the new helper. Acked-by: Oleg Nesterov <oleg@redhat.com>
On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > The rcu/sync code was doing its own check whether we are in a reader > section. With RCU consolidating flavors and the generic helper added in > this series, this is no longer need. We can just use the generic helper > and it results in a nice cleanup. > > Cc: Oleg Nesterov <oleg@redhat.com> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> This needs to be forward-ported to current mainline. (Or, I believe equivalently for this file, to branch "dev" of -rcu.) Especially given that you have Oleg's Ack, I would be happy to take the forward-ported version. Thanx, Paul > --- > Please note: Only build and boot tested this particular patch so far. > > include/linux/rcu_sync.h | 5 ++--- > kernel/rcu/sync.c | 22 ---------------------- > 2 files changed, 2 insertions(+), 25 deletions(-) > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > index 6fc53a1345b3..c954f1efc919 100644 > --- a/include/linux/rcu_sync.h > +++ b/include/linux/rcu_sync.h > @@ -39,9 +39,8 @@ extern void rcu_sync_lockdep_assert(struct rcu_sync *); > */ > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > { > -#ifdef CONFIG_PROVE_RCU > - rcu_sync_lockdep_assert(rsp); > -#endif > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), > + "suspicious rcu_sync_is_idle() usage"); > return !rsp->gp_state; /* GP_IDLE */ > } > > diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c > index a8304d90573f..535e02601f56 100644 > --- a/kernel/rcu/sync.c > +++ b/kernel/rcu/sync.c > @@ -10,37 +10,25 @@ > #include <linux/rcu_sync.h> > #include <linux/sched.h> > > -#ifdef CONFIG_PROVE_RCU > -#define __INIT_HELD(func) .held = func, > -#else > -#define __INIT_HELD(func) > -#endif > - > static const struct { > void (*sync)(void); > void (*call)(struct rcu_head *, void (*)(struct rcu_head *)); > void (*wait)(void); > -#ifdef CONFIG_PROVE_RCU > - int (*held)(void); > -#endif > } gp_ops[] = { > [RCU_SYNC] = { > .sync = synchronize_rcu, > .call = call_rcu, > .wait = rcu_barrier, > - __INIT_HELD(rcu_read_lock_held) > }, > [RCU_SCHED_SYNC] = { > .sync = synchronize_rcu, > .call = call_rcu, > .wait = rcu_barrier, > - __INIT_HELD(rcu_read_lock_sched_held) > }, > [RCU_BH_SYNC] = { > .sync = synchronize_rcu, > .call = call_rcu, > .wait = rcu_barrier, > - __INIT_HELD(rcu_read_lock_bh_held) > }, > }; > > @@ -49,16 +37,6 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY }; > > #define rss_lock gp_wait.lock > > -#ifdef CONFIG_PROVE_RCU > -void rcu_sync_lockdep_assert(struct rcu_sync *rsp) > -{ > - RCU_LOCKDEP_WARN(!gp_ops[rsp->gp_type].held(), > - "suspicious rcu_sync_is_idle() usage"); > -} > - > -EXPORT_SYMBOL_GPL(rcu_sync_lockdep_assert); > -#endif > - > /** > * rcu_sync_init() - Initialize an rcu_sync structure > * @rsp: Pointer to rcu_sync structure to be initialized > -- > 2.22.0.510.g264f2c817a-goog >
On Tue, Jul 16, 2019 at 11:26:42AM -0700, Paul E. McKenney wrote: > On Fri, Jul 12, 2019 at 01:00:18PM -0400, Joel Fernandes (Google) wrote: > > The rcu/sync code was doing its own check whether we are in a reader > > section. With RCU consolidating flavors and the generic helper added in > > this series, this is no longer need. We can just use the generic helper > > and it results in a nice cleanup. > > > > Cc: Oleg Nesterov <oleg@redhat.com> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > This needs to be forward-ported to current mainline. (Or, I believe > equivalently for this file, to branch "dev" of -rcu.) > > Especially given that you have Oleg's Ack, I would be happy to > take the forward-ported version. Never mind, I am one version behind. Apologies for the noise! Thanx, Paul > > --- > > Please note: Only build and boot tested this particular patch so far. > > > > include/linux/rcu_sync.h | 5 ++--- > > kernel/rcu/sync.c | 22 ---------------------- > > 2 files changed, 2 insertions(+), 25 deletions(-) > > > > diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h > > index 6fc53a1345b3..c954f1efc919 100644 > > --- a/include/linux/rcu_sync.h > > +++ b/include/linux/rcu_sync.h > > @@ -39,9 +39,8 @@ extern void rcu_sync_lockdep_assert(struct rcu_sync *); > > */ > > static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) > > { > > -#ifdef CONFIG_PROVE_RCU > > - rcu_sync_lockdep_assert(rsp); > > -#endif > > + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), > > + "suspicious rcu_sync_is_idle() usage"); > > return !rsp->gp_state; /* GP_IDLE */ > > } > > > > diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c > > index a8304d90573f..535e02601f56 100644 > > --- a/kernel/rcu/sync.c > > +++ b/kernel/rcu/sync.c > > @@ -10,37 +10,25 @@ > > #include <linux/rcu_sync.h> > > #include <linux/sched.h> > > > > -#ifdef CONFIG_PROVE_RCU > > -#define __INIT_HELD(func) .held = func, > > -#else > > -#define __INIT_HELD(func) > > -#endif > > - > > static const struct { > > void (*sync)(void); > > void (*call)(struct rcu_head *, void (*)(struct rcu_head *)); > > void (*wait)(void); > > -#ifdef CONFIG_PROVE_RCU > > - int (*held)(void); > > -#endif > > } gp_ops[] = { > > [RCU_SYNC] = { > > .sync = synchronize_rcu, > > .call = call_rcu, > > .wait = rcu_barrier, > > - __INIT_HELD(rcu_read_lock_held) > > }, > > [RCU_SCHED_SYNC] = { > > .sync = synchronize_rcu, > > .call = call_rcu, > > .wait = rcu_barrier, > > - __INIT_HELD(rcu_read_lock_sched_held) > > }, > > [RCU_BH_SYNC] = { > > .sync = synchronize_rcu, > > .call = call_rcu, > > .wait = rcu_barrier, > > - __INIT_HELD(rcu_read_lock_bh_held) > > }, > > }; > > > > @@ -49,16 +37,6 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY }; > > > > #define rss_lock gp_wait.lock > > > > -#ifdef CONFIG_PROVE_RCU > > -void rcu_sync_lockdep_assert(struct rcu_sync *rsp) > > -{ > > - RCU_LOCKDEP_WARN(!gp_ops[rsp->gp_type].held(), > > - "suspicious rcu_sync_is_idle() usage"); > > -} > > - > > -EXPORT_SYMBOL_GPL(rcu_sync_lockdep_assert); > > -#endif > > - > > /** > > * rcu_sync_init() - Initialize an rcu_sync structure > > * @rsp: Pointer to rcu_sync structure to be initialized > > -- > > 2.22.0.510.g264f2c817a-goog > >
diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h index 6fc53a1345b3..c954f1efc919 100644 --- a/include/linux/rcu_sync.h +++ b/include/linux/rcu_sync.h @@ -39,9 +39,8 @@ extern void rcu_sync_lockdep_assert(struct rcu_sync *); */ static inline bool rcu_sync_is_idle(struct rcu_sync *rsp) { -#ifdef CONFIG_PROVE_RCU - rcu_sync_lockdep_assert(rsp); -#endif + RCU_LOCKDEP_WARN(!rcu_read_lock_any_held(), + "suspicious rcu_sync_is_idle() usage"); return !rsp->gp_state; /* GP_IDLE */ } diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c index a8304d90573f..535e02601f56 100644 --- a/kernel/rcu/sync.c +++ b/kernel/rcu/sync.c @@ -10,37 +10,25 @@ #include <linux/rcu_sync.h> #include <linux/sched.h> -#ifdef CONFIG_PROVE_RCU -#define __INIT_HELD(func) .held = func, -#else -#define __INIT_HELD(func) -#endif - static const struct { void (*sync)(void); void (*call)(struct rcu_head *, void (*)(struct rcu_head *)); void (*wait)(void); -#ifdef CONFIG_PROVE_RCU - int (*held)(void); -#endif } gp_ops[] = { [RCU_SYNC] = { .sync = synchronize_rcu, .call = call_rcu, .wait = rcu_barrier, - __INIT_HELD(rcu_read_lock_held) }, [RCU_SCHED_SYNC] = { .sync = synchronize_rcu, .call = call_rcu, .wait = rcu_barrier, - __INIT_HELD(rcu_read_lock_sched_held) }, [RCU_BH_SYNC] = { .sync = synchronize_rcu, .call = call_rcu, .wait = rcu_barrier, - __INIT_HELD(rcu_read_lock_bh_held) }, }; @@ -49,16 +37,6 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY }; #define rss_lock gp_wait.lock -#ifdef CONFIG_PROVE_RCU -void rcu_sync_lockdep_assert(struct rcu_sync *rsp) -{ - RCU_LOCKDEP_WARN(!gp_ops[rsp->gp_type].held(), - "suspicious rcu_sync_is_idle() usage"); -} - -EXPORT_SYMBOL_GPL(rcu_sync_lockdep_assert); -#endif - /** * rcu_sync_init() - Initialize an rcu_sync structure * @rsp: Pointer to rcu_sync structure to be initialized
The rcu/sync code was doing its own check whether we are in a reader section. With RCU consolidating flavors and the generic helper added in this series, this is no longer need. We can just use the generic helper and it results in a nice cleanup. Cc: Oleg Nesterov <oleg@redhat.com> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> --- Please note: Only build and boot tested this particular patch so far. include/linux/rcu_sync.h | 5 ++--- kernel/rcu/sync.c | 22 ---------------------- 2 files changed, 2 insertions(+), 25 deletions(-)