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

[Oleg Nesterov]

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.