From: Oleg Nesterov <oleg@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>,
mingo@kernel.org, linux-kernel@vger.kernel.org, tj@kernel.org,
john.stultz@linaro.org, dimitrysh@google.com, romlem@google.com,
ccross@google.com, tkjos@google.com
Subject: Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()
Date: Mon, 18 Jul 2016 15:44:20 +0200 [thread overview]
Message-ID: <20160718134419.GB25380@redhat.com> (raw)
In-Reply-To: <20160718115427.GB6862@twins.programming.kicks-ass.net>
On 07/18, Peter Zijlstra wrote:
>
> I think I ended up with this:
>
> .----> GP_IDLE <--------------.
> | | |
> | | __rcu_sync_enter() | <GP> / rcu_sync_exit()
> | v |
> | GP_ENTER --------------'
> | |
> | <GP> |
> | v
> | GP_PASSED <---------.
> | | |
> | | rcu_sync_exit() | <GP> / __rcu_sync_enter()
> | v |
> `----- GP_EXIT ------------'
> ^
> <GP> | __rcu_sync_enter() + rcu_sync_exit()
> v
> GP_RETRY
Thanks! I'll include this into the changelog.
> > static void rcu_sync_call(struct rcu_sync *rsp)
> > {
> > // TODO: THIS IS SUBOPTIMAL. We want to call it directly
> > // if rcu_blocking_is_gp() == T, but it has might_sleep().
>
> Not sure I get that comment..
I meant, we actually want
static void rcu_sync_call(struct rcu_sync *rsp)
{
if (rcu_blocking_is_gp())
rcu_sync_func(rsp);
else
gp_ops[rsp->gp_type].call(&rsp->cb_head, rcu_sync_func)
}
but this needs some other simple changes. rcu_sync_func() needs
the same spinlock and rcu_blocking_is_gp() calls might_sleep().
We can simply move rcu_sync_call() outside of rsp->rss_lock, but
then we will need more comments to explain why we can't race with
enter/exit from someone else. Or introduce __rcu_blocking_is_gp()
without might_sleep(), but this needs a trivial change outside of
rcu/sync.c.
We will see. This is simple anyway.
> > spin_lock_irqsave(&rsp->rss_lock, flags);
> > if (rsp->gp_count) {
> > /*
> > * We're at least a GP after the first __rcu_sync_enter().
> > */
> > rsp->gp_state = GP_PASSED;
>
> So we can end up here in two ways afaict.
>
> The simple way: someone called __rcu_sync_enter(), we go IDLE -> ENTER
> with a raised count. Once the GP passes, we get here, observe the raised
> count and advance to PASSED.
>
> The more involved way: we were EXIT and someone calls __rcu_sync_enter()
> to raise the count again. The callback from rcu_sync_exit() was still
> pending and once we get here we observe the raised count and voila.
Yes, yes.
> Now, since state != IDLE, I suppose this is valid, but it does hurt my
> brain.
Simply put, if rsp->gp_count != 0 we do not care about the history and
GP_PASSED is always correct when rcu callback is called, this obviously
means that we passed a GP.
Except GP_IDLE -> GP_PASSED transition is wrong, but this must not be
possible because only rcu callback can set GP_IDLE and only if !gp_count,
so we must always have at least one GP in between. Note also BUG_ON()
checks at the start.
> So I think its solid, but you failed to mention one state transition,
> which seems ok in any case.
Great, thanks for review.
I'll send the actual patch on top of your changes.
Oleg.
next prev parent reply other threads:[~2016-07-18 13:44 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-14 18:25 [PATCH 0/2] locking/percpu-rwsem: Optimizations/tweaks Peter Zijlstra
2016-07-14 18:25 ` [PATCH 1/2] locking/percpu-rwsem: Optimize readers and reduce global impact Peter Zijlstra
2016-07-15 16:30 ` Oleg Nesterov
2016-07-15 19:47 ` Peter Zijlstra
2016-07-18 18:23 ` kbuild test robot
2016-07-18 22:51 ` kbuild test robot
2016-07-14 18:25 ` [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob Peter Zijlstra
2016-07-14 18:37 ` Peter Zijlstra
2016-07-14 18:43 ` Oleg Nesterov
2016-07-14 18:56 ` Peter Zijlstra
2016-07-14 19:20 ` Peter Zijlstra
2016-07-14 19:29 ` Paul E. McKenney
2016-07-14 19:38 ` Peter Zijlstra
2016-07-14 19:54 ` Paul E. McKenney
2016-07-15 13:27 ` Oleg Nesterov
2016-07-15 13:39 ` Paul E. McKenney
2016-07-15 13:45 ` Oleg Nesterov
2016-07-15 15:38 ` Paul E. McKenney
2016-07-15 16:49 ` Oleg Nesterov
2016-07-15 18:01 ` Paul E. McKenney
2016-07-16 17:10 ` [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter() Oleg Nesterov
2016-07-16 18:40 ` Oleg Nesterov
2016-07-18 11:54 ` Peter Zijlstra
2016-07-18 13:44 ` Oleg Nesterov [this message]
2016-07-19 20:50 ` Paul E. McKenney
2016-07-20 15:13 ` Oleg Nesterov
2016-07-20 20:58 ` Paul E. McKenney
2016-07-21 17:34 ` Oleg Nesterov
2016-07-20 17:16 ` Oleg Nesterov
2016-07-20 21:31 ` Paul E. McKenney
2016-07-21 17:34 ` Oleg Nesterov
2016-07-22 3:26 ` Paul E. McKenney
2016-07-25 17:01 ` Oleg Nesterov
2016-07-25 17:05 ` John Stultz
2016-07-25 17:26 ` Oleg Nesterov
2016-08-09 8:48 ` Peter Zijlstra
2016-07-25 17:49 ` Paul E. McKenney
2016-07-15 13:42 ` [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob Oleg Nesterov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160718134419.GB25380@redhat.com \
--to=oleg@redhat.com \
--cc=ccross@google.com \
--cc=dimitrysh@google.com \
--cc=john.stultz@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=paulmck@linux.vnet.ibm.com \
--cc=peterz@infradead.org \
--cc=romlem@google.com \
--cc=tj@kernel.org \
--cc=tkjos@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).