* [PATCH 0/1] rcu_sync: Cleanup the CONFIG_PROVE_RCU checks
@ 2015-09-11 15:59 Oleg Nesterov
2015-09-11 15:59 ` [PATCH 1/1] " Oleg Nesterov
2015-09-11 17:05 ` [PATCH 0/1] " Paul E. McKenney
0 siblings, 2 replies; 4+ messages in thread
From: Oleg Nesterov @ 2015-09-11 15:59 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: Davidlohr Bueso, Peter Zijlstra, linux-kernel
On 09/10, Paul E. McKenney wrote:
>
> On Thu, Sep 10, 2015 at 03:59:42PM +0200, Oleg Nesterov wrote:
> > On 09/09, Paul E. McKenney wrote:
> > >
> > > This is obsolete, but its replacement is the same patch.
> >
> > fbe3b97183f84155d81e506b1aa7d2ce986f7a36 in linux-rcu.git#experimental
> > I guess?
> >
> > > Oleg, Davidlohr, am I missing something on how percpu_rwsem or
> > > locktorture work?
> >
> > No, I think the patch is fine. Thanks for doing this! I was going to
> > send something like this change too. And in fact I am still thinking
> > about another test which plays with rcu_sync only, but probably we
> > need some cleanups first (and we need them anyway). I'll try to do
> > this a bit later.
>
> I would welcome an rcu_sync-specific torture patch!
I want it much more than you ;) I have already warned you, I'll send
more rcu_sync patches. The current code is actually a very early draft
which was written during the discussion with Peter a long ago. I sent
it unchanged because a) it was already reviewed and b) I tested it a
bit in the past.
We can greatly simplify this code and at the same time make it more
useful. Actually I already have the patches. The 1st one removes
rcu_sync->cb_state and gp_ops->sync(). This makes the state machine
almost self-obvious and allows other improvements. See the resulting
(pseudo) code at the end.
But again, I'll try very much to write the test before I send the patch.
Until then, let me send this trivial cleanup. The CONFIG_PROVE_RCU
code looks trivial but imo really annoying. And it is not complete,
so lets document this at least. Plus rcu_lockdep_assert() looks more
consistent.
> > > +void torture_percpu_rwsem_init(void)
> > > +{
> > > + BUG_ON(percpu_init_rwsem(&pcpu_rwsem));
> > > +}
> > > +
> >
> > Aha, we don't really need this... I mean we can use the static initialiser
> > which can also be used by uprobes and cgroups. I'll try to send the patch
> > tomorrow.
>
> Very good, please do!
Hmm. I am lier. I won't send this patch at least today.
The change I had in mind is very simple,
#define DECLARE_PERCPU_RWSEM(sem) \
static DEFINE_PER_CPU(unsigned int, sem##_counters); \
struct percpu_rw_semaphore sem = { \
.fast_read_ctr = &sem##_counters, \
... \
}
and yes, uprobes and cgroups can use it.
But somehow I missed that we can't use it to define a _static_ sem,
static DECLARE_PERCPU_RWSEM(sem);
obviously won't work. And damn, I am shy to admit that I spent several
hours trying to invent something but failed. Perhaps we can add 2 helpers,
DECLARE_PERCPU_RWSEM_GLOBAL() and DECLARE_PERCPU_RWSEM_STATIC().
Oleg.
-------------------------------------------------------------------------------
static const struct {
void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
void (*wait)(void); // TODO: remove this
#ifdef CONFIG_PROVE_RCU
int (*held)(void);
#endif
} gp_ops[] = {
...
};
// COMMENT to explain these states
enum { GP_IDLE = 0, GP_ENTER, GP_PASSED, GP_EXIT, GP_REPLAY };
#define rss_lock gp_wait.lock
// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!1!!!!!!!!
// XXX code must be removed when we split rcu_sync_enter() into start + wait
// !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
static void rcu_sync_func(struct rcu_head *rcu)
{
struct rcu_sync *rsp = container_of(rcu, struct rcu_sync, cb_head);
unsigned long flags;
BUG_ON(rsp->gp_state == GP_IDLE);
BUG_ON(rsp->gp_state == GP_PASSED);
spin_lock_irqsave(&rsp->rss_lock, flags);
if (rsp->gp_count) {
/*
* COMMENT.
*/
rsp->gp_state = GP_PASSED;
wake_up_locked(&rsp->gp_wait);
} else if (rsp->gp_state == GP_REPLAY) {
/*
* A new rcu_sync_exit() has happened; requeue the callback
* to catch a later GP.
*/
rsp->gp_state = GP_EXIT;
gp_ops[rsp->gp_type].call(&rsp->cb_head, rcu_sync_func);
} else {
/*
* We're at least a GP after rcu_sync_exit(); eveybody will now
* have observed the write side critical section. Let 'em rip!.
*/
BUG_ON(rsp->gp_state == GP_ENTER); // XXX
rsp->gp_state = GP_IDLE;
}
spin_unlock_irqrestore(&rsp->rss_lock, flags);
}
static void rcu_sync_call(struct rcu_sync *rsp)
{
// TODO:
// This is called by might_sleep() code outside of ->rss_lock,
// we can avoid ->call() in some cases (say rcu_blocking_is_gp())
gp_ops[rsp->gp_type].call(&rsp->cb_head, rcu_sync_func);
}
void rcu_sync_enter(struct rcu_sync *rsp)
{
int gp_count, gp_state;
spin_lock_irq(&rsp->rss_lock);
gp_count = rsp->gp_count++;
gp_state = rsp->gp_state;
if (gp_state == GP_IDLE)
rsp->gp_state = GP_ENTER;
spin_unlock_irq(&rsp->rss_lock);
BUG_ON(gp_count != 0 && gp_state == GP_IDLE);
BUG_ON(gp_count == 0 && gp_state == GP_PASSED);
BUG_ON(gp_count == 0 && gp_state == GP_ENTER); // XXX
if (gp_state == GP_IDLE)
rcu_sync_call(rsp);
wait_event(rsp->gp_wait, rsp->gp_state != GP_ENTER);
BUG_ON(rsp->gp_state < GP_PASSED);
}
void rcu_sync_exit(struct rcu_sync *rsp)
{
bool need_call;
BUG_ON(rsp->gp_state == GP_IDLE);
BUG_ON(rsp->gp_state == GP_ENTER); // XXX
spin_lock_irq(&rsp->rss_lock);
if (!--rsp->gp_count) {
if (rsp->gp_state == GP_PASSED) {
need_call = true;
rsp->gp_state = GP_EXIT;
} else if (rsp->gp_state == GP_EXIT) {
rsp->gp_state = GP_REPLAY;
}
}
spin_unlock_irq(&rsp->rss_lock);
// Comment to explain why we do not care if another enter()
// and perhaps even exit() comes after spin_unlock().
if (need_call)
rcu_sync_call(rsp);
}
void rcu_sync_dtor(struct rcu_sync *rsp)
{
int gp_state;
BUG_ON(rsp->gp_count);
BUG_ON(rsp->gp_state == GP_ENTER); // XXX
BUG_ON(rsp->gp_state == GP_PASSED);
spin_lock_irq(&rsp->rss_lock);
if (rsp->gp_state == GP_REPLAY)
rsp->gp_state = GP_EXIT;
gp_state = rsp->gp_state;
spin_unlock_irq(&rsp->rss_lock);
// TODO: add another wake_up_locked() into rcu_sync_func(),
// use wait_event + spin_lock_wait, remove gp_ops->wait().
if (gp_state != GP_IDLE) {
gp_ops[rsp->gp_type].wait();
BUG_ON(rsp->gp_state != GP_IDLE);
}
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/1] rcu_sync: Cleanup the CONFIG_PROVE_RCU checks
2015-09-11 15:59 [PATCH 0/1] rcu_sync: Cleanup the CONFIG_PROVE_RCU checks Oleg Nesterov
@ 2015-09-11 15:59 ` Oleg Nesterov
2015-09-11 21:40 ` Paul E. McKenney
2015-09-11 17:05 ` [PATCH 0/1] " Paul E. McKenney
1 sibling, 1 reply; 4+ messages in thread
From: Oleg Nesterov @ 2015-09-11 15:59 UTC (permalink / raw)
To: Paul E. McKenney; +Cc: Davidlohr Bueso, Peter Zijlstra, linux-kernel
1. Rename __rcu_sync_is_idle() to rcu_sync_lockdep_assert() and
change it to use rcu_lockdep_assert().
2. Change rcu_sync_is_idle() to return rsp->gp_state == GP_IDLE
unconditonally, this way we can remove the same check from
rcu_sync_lockdep_assert() and clearly isolate the debugging
code.
Note: rcu_sync_enter()->wait_event(gp_state == GP_PASSED) needs
another CONFIG_PROVE_RCU check, the same we do in ->sync(); but
this needs some simple preparations in the core RCU code to avoid
the code duplication.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
include/linux/rcu_sync.h | 7 +++----
kernel/rcu/sync.c | 6 +++---
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
index 8069d64..a63a33e 100644
--- a/include/linux/rcu_sync.h
+++ b/include/linux/rcu_sync.h
@@ -40,7 +40,7 @@ struct rcu_sync {
enum rcu_sync_type gp_type;
};
-extern bool __rcu_sync_is_idle(struct rcu_sync *);
+extern void rcu_sync_lockdep_assert(struct rcu_sync *);
/**
* rcu_sync_is_idle() - Are readers permitted to use their fastpaths?
@@ -53,10 +53,9 @@ extern bool __rcu_sync_is_idle(struct rcu_sync *);
static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
{
#ifdef CONFIG_PROVE_RCU
- return __rcu_sync_is_idle(rsp);
-#else
- return !rsp->gp_state; /* GP_IDLE */
+ rcu_sync_lockdep_assert(rsp);
#endif
+ return !rsp->gp_state; /* GP_IDLE */
}
extern void rcu_sync_init(struct rcu_sync *, enum rcu_sync_type);
diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
index 1c73c57..a8cf199 100644
--- a/kernel/rcu/sync.c
+++ b/kernel/rcu/sync.c
@@ -63,10 +63,10 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
#define rss_lock gp_wait.lock
#ifdef CONFIG_PROVE_RCU
-bool __rcu_sync_is_idle(struct rcu_sync *rsp)
+void rcu_sync_lockdep_assert(struct rcu_sync *rsp)
{
- WARN_ON(!gp_ops[rsp->gp_type].held());
- return rsp->gp_state == GP_IDLE;
+ rcu_lockdep_assert(gp_ops[rsp->gp_type].held(),
+ "suspicious rcu_sync_is_idle() usage");
}
#endif
--
1.5.5.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 0/1] rcu_sync: Cleanup the CONFIG_PROVE_RCU checks
2015-09-11 15:59 [PATCH 0/1] rcu_sync: Cleanup the CONFIG_PROVE_RCU checks Oleg Nesterov
2015-09-11 15:59 ` [PATCH 1/1] " Oleg Nesterov
@ 2015-09-11 17:05 ` Paul E. McKenney
1 sibling, 0 replies; 4+ messages in thread
From: Paul E. McKenney @ 2015-09-11 17:05 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Davidlohr Bueso, Peter Zijlstra, linux-kernel
On Fri, Sep 11, 2015 at 05:59:01PM +0200, Oleg Nesterov wrote:
> On 09/10, Paul E. McKenney wrote:
> >
> > On Thu, Sep 10, 2015 at 03:59:42PM +0200, Oleg Nesterov wrote:
> > > On 09/09, Paul E. McKenney wrote:
> > > >
> > > > This is obsolete, but its replacement is the same patch.
> > >
> > > fbe3b97183f84155d81e506b1aa7d2ce986f7a36 in linux-rcu.git#experimental
> > > I guess?
> > >
> > > > Oleg, Davidlohr, am I missing something on how percpu_rwsem or
> > > > locktorture work?
> > >
> > > No, I think the patch is fine. Thanks for doing this! I was going to
> > > send something like this change too. And in fact I am still thinking
> > > about another test which plays with rcu_sync only, but probably we
> > > need some cleanups first (and we need them anyway). I'll try to do
> > > this a bit later.
> >
> > I would welcome an rcu_sync-specific torture patch!
>
> I want it much more than you ;) I have already warned you, I'll send
> more rcu_sync patches. The current code is actually a very early draft
> which was written during the discussion with Peter a long ago. I sent
> it unchanged because a) it was already reviewed and b) I tested it a
> bit in the past.
>
> We can greatly simplify this code and at the same time make it more
> useful. Actually I already have the patches. The 1st one removes
> rcu_sync->cb_state and gp_ops->sync(). This makes the state machine
> almost self-obvious and allows other improvements. See the resulting
> (pseudo) code at the end.
>
> But again, I'll try very much to write the test before I send the patch.
That sounds very good! ;-)
> Until then, let me send this trivial cleanup. The CONFIG_PROVE_RCU
> code looks trivial but imo really annoying. And it is not complete,
> so lets document this at least. Plus rcu_lockdep_assert() looks more
> consistent.
>
>
> > > > +void torture_percpu_rwsem_init(void)
> > > > +{
> > > > + BUG_ON(percpu_init_rwsem(&pcpu_rwsem));
> > > > +}
> > > > +
> > >
> > > Aha, we don't really need this... I mean we can use the static initialiser
> > > which can also be used by uprobes and cgroups. I'll try to send the patch
> > > tomorrow.
> >
> > Very good, please do!
>
> Hmm. I am lier. I won't send this patch at least today.
>
> The change I had in mind is very simple,
>
> #define DECLARE_PERCPU_RWSEM(sem) \
> static DEFINE_PER_CPU(unsigned int, sem##_counters); \
> struct percpu_rw_semaphore sem = { \
> .fast_read_ctr = &sem##_counters, \
> ... \
> }
>
> and yes, uprobes and cgroups can use it.
>
> But somehow I missed that we can't use it to define a _static_ sem,
>
> static DECLARE_PERCPU_RWSEM(sem);
>
> obviously won't work. And damn, I am shy to admit that I spent several
> hours trying to invent something but failed. Perhaps we can add 2 helpers,
> DECLARE_PERCPU_RWSEM_GLOBAL() and DECLARE_PERCPU_RWSEM_STATIC().
That is indeed what we do for SRCU for the same reason, DEFINE_SRCU()
and DEFINE_STATIC_SRCU(), but with a common __DEFINE_SRCU() doing the
actual work.
Thanx, Paul
> Oleg.
>
> -------------------------------------------------------------------------------
> static const struct {
> void (*call)(struct rcu_head *, void (*)(struct rcu_head *));
> void (*wait)(void); // TODO: remove this
> #ifdef CONFIG_PROVE_RCU
> int (*held)(void);
> #endif
> } gp_ops[] = {
> ...
> };
>
> // COMMENT to explain these states
> enum { GP_IDLE = 0, GP_ENTER, GP_PASSED, GP_EXIT, GP_REPLAY };
>
> #define rss_lock gp_wait.lock
>
> // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!1!!!!!!!!
> // XXX code must be removed when we split rcu_sync_enter() into start + wait
> // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
>
> static void rcu_sync_func(struct rcu_head *rcu)
> {
> struct rcu_sync *rsp = container_of(rcu, struct rcu_sync, cb_head);
> unsigned long flags;
>
> BUG_ON(rsp->gp_state == GP_IDLE);
> BUG_ON(rsp->gp_state == GP_PASSED);
>
> spin_lock_irqsave(&rsp->rss_lock, flags);
> if (rsp->gp_count) {
> /*
> * COMMENT.
> */
> rsp->gp_state = GP_PASSED;
> wake_up_locked(&rsp->gp_wait);
> } else if (rsp->gp_state == GP_REPLAY) {
> /*
> * A new rcu_sync_exit() has happened; requeue the callback
> * to catch a later GP.
> */
> rsp->gp_state = GP_EXIT;
> gp_ops[rsp->gp_type].call(&rsp->cb_head, rcu_sync_func);
> } else {
> /*
> * We're at least a GP after rcu_sync_exit(); eveybody will now
> * have observed the write side critical section. Let 'em rip!.
> */
> BUG_ON(rsp->gp_state == GP_ENTER); // XXX
> rsp->gp_state = GP_IDLE;
> }
> spin_unlock_irqrestore(&rsp->rss_lock, flags);
> }
>
> static void rcu_sync_call(struct rcu_sync *rsp)
> {
> // TODO:
> // This is called by might_sleep() code outside of ->rss_lock,
> // we can avoid ->call() in some cases (say rcu_blocking_is_gp())
> gp_ops[rsp->gp_type].call(&rsp->cb_head, rcu_sync_func);
> }
>
> void rcu_sync_enter(struct rcu_sync *rsp)
> {
> int gp_count, gp_state;
>
> spin_lock_irq(&rsp->rss_lock);
> gp_count = rsp->gp_count++;
> gp_state = rsp->gp_state;
> if (gp_state == GP_IDLE)
> rsp->gp_state = GP_ENTER;
> spin_unlock_irq(&rsp->rss_lock);
>
> BUG_ON(gp_count != 0 && gp_state == GP_IDLE);
> BUG_ON(gp_count == 0 && gp_state == GP_PASSED);
> BUG_ON(gp_count == 0 && gp_state == GP_ENTER); // XXX
>
> if (gp_state == GP_IDLE)
> rcu_sync_call(rsp);
>
> wait_event(rsp->gp_wait, rsp->gp_state != GP_ENTER);
> BUG_ON(rsp->gp_state < GP_PASSED);
> }
>
> void rcu_sync_exit(struct rcu_sync *rsp)
> {
> bool need_call;
>
> BUG_ON(rsp->gp_state == GP_IDLE);
> BUG_ON(rsp->gp_state == GP_ENTER); // XXX
>
> spin_lock_irq(&rsp->rss_lock);
> if (!--rsp->gp_count) {
> if (rsp->gp_state == GP_PASSED) {
> need_call = true;
> rsp->gp_state = GP_EXIT;
> } else if (rsp->gp_state == GP_EXIT) {
> rsp->gp_state = GP_REPLAY;
> }
> }
> spin_unlock_irq(&rsp->rss_lock);
>
> // Comment to explain why we do not care if another enter()
> // and perhaps even exit() comes after spin_unlock().
> if (need_call)
> rcu_sync_call(rsp);
> }
>
> void rcu_sync_dtor(struct rcu_sync *rsp)
> {
> int gp_state;
>
> BUG_ON(rsp->gp_count);
> BUG_ON(rsp->gp_state == GP_ENTER); // XXX
> BUG_ON(rsp->gp_state == GP_PASSED);
>
> spin_lock_irq(&rsp->rss_lock);
> if (rsp->gp_state == GP_REPLAY)
> rsp->gp_state = GP_EXIT;
> gp_state = rsp->gp_state;
> spin_unlock_irq(&rsp->rss_lock);
>
> // TODO: add another wake_up_locked() into rcu_sync_func(),
> // use wait_event + spin_lock_wait, remove gp_ops->wait().
>
> if (gp_state != GP_IDLE) {
> gp_ops[rsp->gp_type].wait();
> BUG_ON(rsp->gp_state != GP_IDLE);
> }
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] rcu_sync: Cleanup the CONFIG_PROVE_RCU checks
2015-09-11 15:59 ` [PATCH 1/1] " Oleg Nesterov
@ 2015-09-11 21:40 ` Paul E. McKenney
0 siblings, 0 replies; 4+ messages in thread
From: Paul E. McKenney @ 2015-09-11 21:40 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Davidlohr Bueso, Peter Zijlstra, linux-kernel
On Fri, Sep 11, 2015 at 05:59:18PM +0200, Oleg Nesterov wrote:
> 1. Rename __rcu_sync_is_idle() to rcu_sync_lockdep_assert() and
> change it to use rcu_lockdep_assert().
>
> 2. Change rcu_sync_is_idle() to return rsp->gp_state == GP_IDLE
> unconditonally, this way we can remove the same check from
> rcu_sync_lockdep_assert() and clearly isolate the debugging
> code.
>
> Note: rcu_sync_enter()->wait_event(gp_state == GP_PASSED) needs
> another CONFIG_PROVE_RCU check, the same we do in ->sync(); but
> this needs some simple preparations in the core RCU code to avoid
> the code duplication.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Queued for v4.4, thank you, Oleg!
Thanx, Paul
> ---
> include/linux/rcu_sync.h | 7 +++----
> kernel/rcu/sync.c | 6 +++---
> 2 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/rcu_sync.h b/include/linux/rcu_sync.h
> index 8069d64..a63a33e 100644
> --- a/include/linux/rcu_sync.h
> +++ b/include/linux/rcu_sync.h
> @@ -40,7 +40,7 @@ struct rcu_sync {
> enum rcu_sync_type gp_type;
> };
>
> -extern bool __rcu_sync_is_idle(struct rcu_sync *);
> +extern void rcu_sync_lockdep_assert(struct rcu_sync *);
>
> /**
> * rcu_sync_is_idle() - Are readers permitted to use their fastpaths?
> @@ -53,10 +53,9 @@ extern bool __rcu_sync_is_idle(struct rcu_sync *);
> static inline bool rcu_sync_is_idle(struct rcu_sync *rsp)
> {
> #ifdef CONFIG_PROVE_RCU
> - return __rcu_sync_is_idle(rsp);
> -#else
> - return !rsp->gp_state; /* GP_IDLE */
> + rcu_sync_lockdep_assert(rsp);
> #endif
> + return !rsp->gp_state; /* GP_IDLE */
> }
>
> extern void rcu_sync_init(struct rcu_sync *, enum rcu_sync_type);
> diff --git a/kernel/rcu/sync.c b/kernel/rcu/sync.c
> index 1c73c57..a8cf199 100644
> --- a/kernel/rcu/sync.c
> +++ b/kernel/rcu/sync.c
> @@ -63,10 +63,10 @@ enum { CB_IDLE = 0, CB_PENDING, CB_REPLAY };
> #define rss_lock gp_wait.lock
>
> #ifdef CONFIG_PROVE_RCU
> -bool __rcu_sync_is_idle(struct rcu_sync *rsp)
> +void rcu_sync_lockdep_assert(struct rcu_sync *rsp)
> {
> - WARN_ON(!gp_ops[rsp->gp_type].held());
> - return rsp->gp_state == GP_IDLE;
> + rcu_lockdep_assert(gp_ops[rsp->gp_type].held(),
> + "suspicious rcu_sync_is_idle() usage");
> }
> #endif
>
> --
> 1.5.5.1
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-09-11 21:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-11 15:59 [PATCH 0/1] rcu_sync: Cleanup the CONFIG_PROVE_RCU checks Oleg Nesterov
2015-09-11 15:59 ` [PATCH 1/1] " Oleg Nesterov
2015-09-11 21:40 ` Paul E. McKenney
2015-09-11 17:05 ` [PATCH 0/1] " Paul E. McKenney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox