Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1443672 > unrolled thread

[PATCH 2/2] locking/percpu-rwsem: Introduce bias knob

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob Peter Zijlstra <peterz@infradead.org> - 2016-07-14 20:40 +0200
    Re: [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob Oleg Nesterov <oleg@redhat.com> - 2016-07-14 20:50 +0200
      Re: [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob Peter Zijlstra <peterz@infradead.org> - 2016-07-14 21:00 +0200
      Re: [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob Peter Zijlstra <peterz@infradead.org> - 2016-07-14 21:30 +0200
        Re: [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 21:30 +0200
          Re: [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob Peter Zijlstra <peterz@infradead.org> - 2016-07-14 21:40 +0200
            Re: [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-14 22:00 +0200
        Re: [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob Oleg Nesterov <oleg@redhat.com> - 2016-07-15 15:30 +0200
          Re: [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-15 15:40 +0200
            Re: [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob Oleg Nesterov <oleg@redhat.com> - 2016-07-15 15:50 +0200
              Re: [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-15 17:40 +0200
                Re: [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob Oleg Nesterov <oleg@redhat.com> - 2016-07-15 18:50 +0200
                  Re: [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-15 20:10 +0200
                    [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() Oleg Nesterov <oleg@redhat.com> - 2016-07-16 19:20 +0200
                      Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() Oleg Nesterov <oleg@redhat.com> - 2016-07-16 20:50 +0200
                      Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() Peter Zijlstra <peterz@infradead.org> - 2016-07-18 14:00 +0200
                        Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() Oleg Nesterov <oleg@redhat.com> - 2016-07-18 15:50 +0200
                      Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-19 23:00 +0200
                        Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() Oleg Nesterov <oleg@redhat.com> - 2016-07-20 17:20 +0200
                          Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-20 23:00 +0200
                            Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() Oleg Nesterov <oleg@redhat.com> - 2016-07-21 19:40 +0200
                        Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() Oleg Nesterov <oleg@redhat.com> - 2016-07-20 19:20 +0200
                          Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-20 23:40 +0200
                            Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() Oleg Nesterov <oleg@redhat.com> - 2016-07-21 19:40 +0200
                              Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-22 05:30 +0200
                                Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter() John Stultz <john.stultz@linaro.org> - 2016-07-25 19:10 +0200
                                  Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() Oleg Nesterov <oleg@redhat.com> - 2016-07-25 19:30 +0200
                                    Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() Peter Zijlstra <peterz@infradead.org> - 2016-08-09 10:50 +0200
                                Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() Oleg Nesterov <oleg@redhat.com> - 2016-07-25 19:10 +0200
                                  Re: [PATCH] rcu_sync: simplify the state machine, introduce  __rcu_sync_enter() "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-07-25 19:50 +0200
        Re: [PATCH 2/2] locking/percpu-rwsem: Introduce bias knob Oleg Nesterov <oleg@redhat.com> - 2016-07-15 15:50 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1448025 — Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()

On 07/20, Paul E. McKenney wrote:
>
> On Wed, Jul 20, 2016 at 05:13:58PM +0200, Oleg Nesterov wrote:
>
> > rcu_sync_enter() or __rcu_sync_enter() is legal in any state, the latter
> > won't block.
>
> Actually, I had no idea that __rcu_sync_enter() was intended for anything
> other than internal use.
>
> Other than that, agreed, with the exception that it is illegal after
> rcu_sync_dtor() has been called.

Yes, sure. rcu_sync_dtor() "destroys" struct rcu_sync, and currently it
is only called by destroy_super_work() right before kfree(). Nothing is
legal after rcu_sync_dtor().

> > > > 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().
> > >
> > > Careful!  This optimization works only for RCU-sched and RCU-bh.
> > > With normal RCU, you can be tripped up by tasks preempted within RCU
> > > read-side critical sections should CONFIG_PREEMPT=y.
> >
> > Yes, thanks, I understand ;) another reason why I do not want to add
> > this optimization into the initial version.
>
> So I should take this as a request to export rcu_blocking_is_gp()?

Would be nice ;) Or we can do something else. Nevermind, this needs
another discussion.

> > > > void rcu_sync_dtor(struct rcu_sync *rsp)
> > > > {
> > > > 	int gp_state;
> > > >
> > > > 	BUG_ON(rsp->gp_count);
> > > > 	BUG_ON(rsp->gp_state == GP_PASSED);
> > > >
> > > > 	spin_lock_irq(&rsp->rss_lock);
> > > > 	if (rsp->gp_state == GP_REPLAY)
> > > > 		rsp->gp_state = GP_EXIT;
> > >
> > > OK, this ensures that the .wait() below will wait for the callback, but
> > > it might result in some RCU read-side critical sections still being in
> > > flight after rcu_sync_dtor() completes.
> >
> > Hmm. Obviously, the caller should prevent this somehow or it is simply
> > buggy. Or I misunderstood.
>
> Hard to say without knowing what the permitted use cases are...
>
> Me, I would make rcu_sync_dtor() wait the extra grace period in this case.
> It should be a low-probability race, and it reduces the _dtor-time
> state space.
>
> What it looks like you are saying is that the caller must not only ensure
> that there will never again be a __rcu_sync_enter(), rcu_sync_enter(),
> or rcu_sync_exit() (or, I suppose, rcu_sync_dtor()) for this rcu_sync
> structure,

Yes, and

> but must also ensure that any relevant RCU read-side critical
> sections have completed.

Ah, now I understand your concerns. Yes, yes, sure. The caller must
ensure that all RCU read-side critical sections which might look at this
rcu_sync via rcu_sync_is_idle() have completed.

Currently the only caller of dtor() is percpu_free_rwsem(). So if you do,
say,

	struct percpu_rw_semaphore *sem = kmalloc(...);

	...

	percpu_free_rwsem(sem);
	kfree(sem);

you obviously need to ensure that percpu_free_rwsem() can't be called
before all readers fully complete their percpu_down_read/percpu_up_read
critical sections, this includes the RCU read-side critical sections.

And this doesn't doesn't really differ from the plain rw_semaphore.


And can't resist... let me add another "TODO" note ;) we actually want
to improve it a bit (probably just a "bool wait" arg) and kill the ugly
super_block->destroy_work which currently does percpu_free_rwsem(). This
should be simple.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1447371 — Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()

Paul, I had to switch to internal bugzillas after the first email, and now
I feel I can't read... I'll try to answer one question right now, tomorrow
I'll reread your email, probably I need to answer something else...

On 07/19, Paul E. McKenney wrote:
>
> On Sat, Jul 16, 2016 at 07:10:07PM +0200, Oleg Nesterov wrote:
>
> > And, there is another possible transition, GP_ENTER -> GP_IDLE, because
> > not it is possible to call __rcu_sync_enter() and rcu_sync_exit() in any
> > state (except obviously they should be balanced), and they do not block.
...
> If you feel strongly about allowing rcu_sync_exit() in GP_ENTER state,
> could you please tell me your use case?  Or am I confused?

See below,

> And I think __rcu_sync_enter() can have more users. Let's look at
> freeze_super(). It calls percpu_down_write() 3 times, and waits for 3 GP's
> sequentally.
>
> > Now we can add 3 __rcu_sync_enter's at the start and 3 rcu_sync_exit's at
> > the end (actually we can do better, just to simplify). And again, note
> > that rcu_sync_exit() will work correctly even if we (say) return -EBUSY,
> > so rcu_sync_wait and/or percpu_down_write() was not called in between,
> > and in this case we won't block waiting for GP.
> >
>
> I am not going to claim to understand freeze_super(), but it does seem
> to have a fair amount of waiting.
>
> But yes, you could put rcu_sync_enter()
                         ^^^^^^^^^^^^^^
__rcu_sync_enter, see below,

> and rcu_sync_exit() before and
> after a series of write-side enter/exit pairs in order to force things
> to stay in writer mode, if that is what you are suggesting.

No, no, this is not what I am trying to suggest.

The problem is that freeze_super() takes 3 semaphores for writing in row,
this means that it needs to wait for 3 GP's sequentally, and it does this
with sb->s_umount held. This is just ugly.

OK, lets suppose it simply does

	freeze_super(sb)
	{
		down_write(&sb->s_umount);
		if (NEED_TO_FREEZE) {
			percpu_down_write(SEM1);
			percpu_down_write(SEM2);
			percpu_down_write(SEM3);
		}
		up_write(&sb->s_umount);
	}

and every percpu_down_write() waits for GP.

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);
			percpu_down_write(SEM2);
			percpu_down_write(SEM3);
		}
		up_write(&sb->s_umount);

		rcu_sync_exit(SEM1);
		rcu_sync_exit(SEM2);
		rcu_sync_exit(SEM3);
		
	}

Again, actually we can do better, just to simplify.

Now. the fisrt percpu_down_write(SEM1) can block waiting for GP or not,
this depends on how many time it spends in down_write().

But the 2nd and the 3rd percpu_down_write() most likely won't block, so
in the likely case freeze_super() will need a single GP pass.

And note that NEED_TO_FREEZE can be false, in this case rcu_sync_exit()
will be called in GP_ENTER state.


To some degree, this is like get_state_synchronize_rcu/cond_synchronize_rcu.
But obviously percpu_down_write() can not use these helpers, and in this
particular case __rcu_sync_enter() is better because it forces the start
of GP pass.

-----------------------------------------------------------------------
As for cgroups, we want to switch cgroup_threadgroup_rwsem into the
slow mode, at least for now.

We could add the additional hooks/hacks into rcu/sync.c but why? We can
do this without any changes outside of cgroup.c right now, just add
rcu_sync_enter() into cgroup_init().

But we do not want to add a pointless synchronize_sched() at boot time,
__rcu_sync_enter() looks much better.

------------------------------------------------------------------------
And even __cgroup_procs_write() could use __rcu_sync_enter(). But lets
ignore this for now.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1447494 — Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()

On Wed, Jul 20, 2016 at 07:16:03PM +0200, Oleg Nesterov wrote:
> Paul, I had to switch to internal bugzillas after the first email, and now
> I feel I can't read... I'll try to answer one question right now, tomorrow
> I'll reread your email, probably I need to answer something else...

I know that feeling of distraction!

> On 07/19, Paul E. McKenney wrote:
> >
> > On Sat, Jul 16, 2016 at 07:10:07PM +0200, Oleg Nesterov wrote:
> >
> > > And, there is another possible transition, GP_ENTER -> GP_IDLE, because
> > > not it is possible to call __rcu_sync_enter() and rcu_sync_exit() in any
> > > state (except obviously they should be balanced), and they do not block.
> ...
> > If you feel strongly about allowing rcu_sync_exit() in GP_ENTER state,
> > could you please tell me your use case?  Or am I confused?
> 
> See below,
> 
> > And I think __rcu_sync_enter() can have more users. Let's look at
> > freeze_super(). It calls percpu_down_write() 3 times, and waits for 3 GP's
> > sequentally.
> >
> > > Now we can add 3 __rcu_sync_enter's at the start and 3 rcu_sync_exit's at
> > > the end (actually we can do better, just to simplify). And again, note
> > > that rcu_sync_exit() will work correctly even if we (say) return -EBUSY,
> > > so rcu_sync_wait and/or percpu_down_write() was not called in between,
> > > and in this case we won't block waiting for GP.
> > >
> >
> > I am not going to claim to understand freeze_super(), but it does seem
> > to have a fair amount of waiting.
> >
> > But yes, you could put rcu_sync_enter()
>                          ^^^^^^^^^^^^^^
> __rcu_sync_enter, see below,
> 
> > and rcu_sync_exit() before and
> > after a series of write-side enter/exit pairs in order to force things
> > to stay in writer mode, if that is what you are suggesting.
> 
> No, no, this is not what I am trying to suggest.
> 
> The problem is that freeze_super() takes 3 semaphores for writing in row,
> this means that it needs to wait for 3 GP's sequentally, and it does this
> with sb->s_umount held. This is just ugly.

Slow, too.  ;-)

> OK, lets suppose it simply does
> 
> 	freeze_super(sb)
> 	{
> 		down_write(&sb->s_umount);
> 		if (NEED_TO_FREEZE) {
> 			percpu_down_write(SEM1);
> 			percpu_down_write(SEM2);
> 			percpu_down_write(SEM3);
> 		}
> 		up_write(&sb->s_umount);
> 	}
> 
> and every percpu_down_write() waits for GP.
> 
> 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.  Or am I confused?

> 			percpu_down_write(SEM2);
> 			percpu_down_write(SEM3);
> 		}
> 		up_write(&sb->s_umount);
> 
> 		rcu_sync_exit(SEM1);

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?

> 		rcu_sync_exit(SEM2);
> 		rcu_sync_exit(SEM3);

And the reason it is OK to invoke rcu_sync_exit() before doing
the percpu_up_write() calls is that those calls will do their own
rcu_sync_exit() calls, and the above calls really don't do anything
other than decrement the count.

> 		
> 	}
> 
> Again, actually we can do better, just to simplify.
> 
> Now. the fisrt percpu_down_write(SEM1) can block waiting for GP or not,
> this depends on how many time it spends in down_write().
> 
> But the 2nd and the 3rd percpu_down_write() most likely won't block, so
> in the likely case freeze_super() will need a single GP pass.
> 
> And note that NEED_TO_FREEZE can be false, in this case rcu_sync_exit()
> will be called in GP_ENTER state.

Understood, the above approach allows the three grace periods to
elapse concurrently, at least assuming RCU is busy at the time.
(If RCU is not busy, the first one gets its own grace period, and
the other two share the next grace period.)

But again why aren't the __rcu_sync_enter() and rcu_sync_exit() calls
inside that "if" statement?

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.

> To some degree, this is like get_state_synchronize_rcu/cond_synchronize_rcu.
> But obviously percpu_down_write() can not use these helpers, and in this
> particular case __rcu_sync_enter() is better because it forces the start
> of GP pass.

OK, I now understand the motivation, thank you for your patience!

> -----------------------------------------------------------------------
> As for cgroups, we want to switch cgroup_threadgroup_rwsem into the
> slow mode, at least for now.
> 
> We could add the additional hooks/hacks into rcu/sync.c but why? We can
> do this without any changes outside of cgroup.c right now, just add
> rcu_sync_enter() into cgroup_init().
> 
> But we do not want to add a pointless synchronize_sched() at boot time,
> __rcu_sync_enter() looks much better.

I do like that approach much better than an rcu_sync_sabotage().

I still have concern with the __rcu_sync_enter() name, and would prefer
something that clearly indicates that it starts the rcu_sync() entry
job, but does not synchronously complete it.

> ------------------------------------------------------------------------
> And even __cgroup_procs_write() could use __rcu_sync_enter(). But lets
> ignore this for now.

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.

							Thanx, Paul

------------------------------------------------------------------------

o       "count" is the value of the rcu_sync structure's ->gp_count field.

o       "state" is the value of the rcu_sync structure's ->gp_state field.

o       "CB?" is "yes" if there is an RCU callback pending and "no" otherwise.



   | count | state     | CB? | next state
---+-------+-----------+-----+------------------------------------
   |       |           |     |
 A |   0   | GP_IDLE   | no  | rcu_sync_enter() -> B (wait)
   |       |           |     | rcu_sync_exit() -> illegal
   |       |           |     | rcu_sync_dtor() -> H
   |       |           |     | callback -> cannot happen
   |       |           |     |
---+-------+-----------+-----+------------------------------------
   |       |           |     |
 B |   1+  | GP_ENTER  | yes | rcu_sync_enter() -> B (wait)
   |       |           |     | last rcu_sync_exit() -> J
   |       |           |     | non-last rcu_sync_exit() -> B
   |       |           |     | rcu_sync_dtor() -> illegal
   |       |           |     | callback -> C (ends _enter() wait)
   |       |           |     |
---+-------+-----------+-----+------------------------------------
   |       |           |     |
 C |   1+  | GP_PASSED | no  | rcu_sync_enter() -> C
   |       |           |     | last rcu_sync_exit() -> E
   |       |           |     | non-last rcu_sync_exit() -> C
   |       |           |     | rcu_sync_dtor() -> illegal
   |       |           |     | callback -> cannot happen
   |       |           |     |
---+-------+-----------+-----+------------------------------------
   |       |           |     |
 D |   0   | GP_REPLAY | yes | rcu_sync_enter() -> F
   |       |           |     | rcu_sync_exit() -> illegal
   |       |           |     | rcu_sync_dtor() -> I (wait ???)
   |       |           |     | callback -> E
   |       |           |     |
---+-------+-----------+-----+------------------------------------
   |       |           |     |
 E |   0   | GP_EXIT   | yes | rcu_sync_enter() -> G
   |       |           |     | rcu_sync_exit() -> illegal
   |       |           |     | rcu_sync_dtor() -> I (wait)
   |       |           |     | callback -> A
   |       |           |     |
---+-------+-----------+-----+------------------------------------
   |       |           |     |
 F |   1+  | GP_REPLAY | yes | rcu_sync_enter() -> F
   |       |           |     | last rcu_sync_exit() -> D
   |       |           |     | non-last rcu_sync_exit() -> F
   |       |           |     | rcu_sync_dtor() -> illegal
   |       |           |     | callback -> C
   |       |           |     |
---+-------+-----------+-----+------------------------------------
   |       |           |     |
 G |   1+  | GP_EXIT   | yes | rcu_sync_enter() -> G
   |       |           |     | last rcu_sync_exit() -> D
   |       |           |     | non-last rcu_sync_exit() -> F
   |       |           |     | rcu_sync_dtor() -> illegal
   |       |           |     | callback -> C
   |       |           |     |
---+-------+-----------+-----+------------------------------------
   |       |           |     |
 H |   0   | GP_IDLE   | no  | rcu_sync_enter() -> illegal
   |       |           |     | rcu_sync_exit() -> illegal
   |       |           |     | rcu_sync_dtor() -> illegal
   |       |           |     | callback -> cannot happen
   |       |           |     |
---+-------+-----------+-----+------------------------------------
   |       |           |     |
 I |   0   | GP_EXIT   | yes | rcu_sync_enter() -> illegal
   |       |           |     | rcu_sync_exit() -> illegal
   |       |           |     | rcu_sync_dtor() -> illegal
   |       |           |     | callback -> H (ends dtor() wait)
---+-------+-----------+-----+------------------------------------
   |       |           |     |
 J |   0   | GP_ENTER  | yes | rcu_sync_enter() -> B (wait)
   |       |           |     | rcu_sync_exit() -> illegal
   |       |           |     | rcu_sync_dtor() -> illegal
   |       |           |     | callback -> A
   |       |           |     |


Assumptions:

o	Initial state is A.

o	Final state is H.

o	There will never be enough unpaired rcu_sync_enter() calls to
	overflow ->gp_count.

o	All calls to rcu_sync_exit() must pair with a preceding call
	to rcu_sync_enter() by that same thread.

o	It is illegal to invoke rcu_sync_dtor() until after the caller
	has ensured that there will be no future calls to either
	rcu_sync_enter() or rcu_sync_exit().

o	It is illegal to invoke rcu_sync_dtor() while there are any
	unpaired calls to rcu_sync_enter().

[toc] | [prev] | [next] | [standalone]

#1448027 — Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()

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.

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.

> 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.

> 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.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1448345 — Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()

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

[toc] | [prev] | [next] | [standalone]

#1449659 — Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()

On Mon, Jul 25, 2016 at 10:01 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> Paul, sorry for delay.
>
> On 07/21, Paul E. McKenney wrote:
>>
>> 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.
>
> Hmm. which global lock? Or did you mean freeze_super(), not rcu_sync?
>
>> 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.
>
> Well, I do not agree, but this wasn't written by me. Just in case, all these
> locks above are not really global, they are per-sb, but this is minor.
>
> And the patches which changed sb->s_writers to use percpu_rw_semaphore/rcu_sync
> didn't change this logic.
>
> Except the old implementation was buggy, and the readers were slower than now.
>
>> I agree that there are use cases for beginning-of-time __rcu_sync_enter()
>> or whatever we end up naming it.
>
> OK, at least iiuc you agree that cgroup_init() can use __rcu_sync_enter().
> As for other potential use-cases, we will disccuss this later. I will have
> to CC you anyway ;)
>
> So I'll send v2 with renames after I test it. Thanks again.

Can you also make clear which patches of PeterZ's I should be adding
as well for testing?  I've lost the plot as to what goes with what..

thanks
-john

[toc] | [prev] | [next] | [standalone]

#1449667 — Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()

On 07/25, John Stultz wrote:
>
> Can you also make clear which patches of PeterZ's I should be adding
> as well for testing?  I've lost the plot as to what goes with what..

Well, my understanding is that Peter is going to send v2 with some minor
changes, I am going to send this patch on top of his series.

In particular 2/2 should set GP_PASSED rather than !GP_IDLE. It would be
nice to also rename rcu_sync_sabotage() to rcu_sync_enter_start(), this
will save another change in cgroup.c, but this is minor.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1458519 — Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()

On Mon, Jul 25, 2016 at 07:26:26PM +0200, Oleg Nesterov wrote:
> On 07/25, John Stultz wrote:
> >
> > Can you also make clear which patches of PeterZ's I should be adding
> > as well for testing?  I've lost the plot as to what goes with what..
> 
> Well, my understanding is that Peter is going to send v2 with some minor
> changes, I am going to send this patch on top of his series.

OK, back from holidays and I finally got around to reading this again.

So let me send out the rwsem optmization patch v2 in a new thread and
lets take it from there.

[toc] | [prev] | [next] | [standalone]

#1449660 — Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()

Paul, sorry for delay.

On 07/21, Paul E. McKenney wrote:
>
> 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.

Hmm. which global lock? Or did you mean freeze_super(), not rcu_sync?

> 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.

Well, I do not agree, but this wasn't written by me. Just in case, all these
locks above are not really global, they are per-sb, but this is minor.

And the patches which changed sb->s_writers to use percpu_rw_semaphore/rcu_sync
didn't change this logic.

Except the old implementation was buggy, and the readers were slower than now.

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

OK, at least iiuc you agree that cgroup_init() can use __rcu_sync_enter().
As for other potential use-cases, we will disccuss this later. I will have
to CC you anyway ;)

So I'll send v2 with renames after I test it. Thanks again.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1449685 — Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter()

On Mon, Jul 25, 2016 at 07:01:17PM +0200, Oleg Nesterov wrote:
> Paul, sorry for delay.
> 
> On 07/21, Paul E. McKenney wrote:
> >
> > 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.
> 
> Hmm. which global lock? Or did you mean freeze_super(), not rcu_sync?

OK, you are right, they are per-superblock locks rather than being global
locks.  Still, given that workloads that hammer a single filesystem hard
are quite common, it might still eventually be of some concern.

> > 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.
> 
> Well, I do not agree, but this wasn't written by me. Just in case, all these
> locks above are not really global, they are per-sb, but this is minor.

Agreed.

> And the patches which changed sb->s_writers to use percpu_rw_semaphore/rcu_sync
> didn't change this logic.
> 
> Except the old implementation was buggy, and the readers were slower than now.
> 
> > I agree that there are use cases for beginning-of-time __rcu_sync_enter()
> > or whatever we end up naming it.
> 
> OK, at least iiuc you agree that cgroup_init() can use __rcu_sync_enter().
> As for other potential use-cases, we will disccuss this later. I will have
> to CC you anyway ;)
> 
> So I'll send v2 with renames after I test it. Thanks again.

Sounds good!

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1444331

On 07/14, Peter Zijlstra wrote:
>
> +void rcu_sync_sabotage(struct rcu_sync *rsp)
> +{
> +	rsp->gp_count++;
> +	rsp->gp_state = !GP_IDLE;
> +}

Ah, I didn't notice this !GP_IDLE...

Please use GP_PASSED, this is what this actually means. And note the
wait_event(GP_PASSED) in rcu_sync_enter().

Otherwise

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web