linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	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: Thu, 21 Jul 2016 20:26:41 -0700	[thread overview]
Message-ID: <20160722032641.GE7094@linux.vnet.ibm.com> (raw)
In-Reply-To: <20160721173435.GB22488@redhat.com>

On Thu, Jul 21, 2016 at 07:34:36PM +0200, Oleg Nesterov wrote:
> On 07/20, Paul E. McKenney wrote:
> >
> > On Wed, Jul 20, 2016 at 07:16:03PM +0200, Oleg Nesterov wrote:
> >
> > > Now, suppose we add the additional enter/exit's:
> > >
> > > 	freeze_super(sb)
> > > 	{
> > > 		// this doesn't block
> > > 		__rcu_sync_enter(SEM3);
> > > 		__rcu_sync_enter(SEM2);
> > > 		__rcu_sync_enter(SEM1);
> > >
> > > 		down_write(&sb->s_umount);
> > > 		if (NEED_TO_FREEZE) {
> > > 			percpu_down_write(SEM1);
> >
> > The above waits for the grace period initiated by __rcu_sync_enter(),
> > correct?  Presumably "yes", because it will invoke rcu_sync_enter(), which
> > will see the state as GP_ENTER, and will thus wait.
> 
> But if down_write() blocks and/or NEED_TO_FREEZE takes some time it
> could already see the GP_PASSED state, or at least it can sleep less.
> 
> > But your point is that if !NEED_TO_FREEZE, we will get here without
> > waiting for a grace period.
> >
> > But why aren't the __rcu_sync_enter() and rcu_sync_exit() calls inside
> > the "if" statement?
> 
> Yes, if we do __rcu_sync_enter() inside "if", then rcu_sync_exit() can't
> hit GP_ENTER.
> 
> But why we should disallow this use-case? It does not complicate the code
> at all.

I do agree that it doesn't complicate the current implementation.
But it relies on a global lock, so I am not at all confident that this
implementation is the final word.  I therefore tend to try to avoid
supporting more than is required.

And speaking of global locks, failing to discourage the pattern above
means that the code is unnecessarily acquiring three global locks,
which doesn't seem like a good thing to me.

> And see above, we want to initiate the GP "asap", so that we will sleep
> less later. Although yes, freeze_super() is not the best example. And
> __cgroup_procs_write() too, but note that cgroup_kn_lock_live() is rather
> heavy, takes the global locks, and can fail. So (ignoring the fact we
> are going to switch cgroup_threadgroup_rwsem into the slow mode for now)
> __rcu_sync_enter() at the start could help to lessen the time
> percpu_down_write(cgroup_threadgroup_rwsem) sleeps with the cgroup_mutex
> held.

I agree that there are use cases for beginning-of-time __rcu_sync_enter()
or whatever we end up naming it.

> > That aside, would it make sense to name __rcu_sync_enter() something
> > like rcu_sync_begin_to_enter(), rcu_sync_pre_enter() or some such?
> > Something to make it clear that it just starts the job and that something
> > else is needed to finish it.
> 
> Sure. Agreed, will rename.

Thank you!

> > And here is an updated state table.  I do not yet separately call out
> > __rcu_sync_enter(), though without it the rcu_sync_exit() transition
> > out of state B cannot happen.
> 
> Thanks! I'll try to double-check it.

And thank you again!

							Thanx, Paul

  reply	other threads:[~2016-07-22  3:26 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
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 [this message]
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=20160722032641.GE7094@linux.vnet.ibm.com \
    --to=paulmck@linux.vnet.ibm.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=oleg@redhat.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).