Messages in this thread | | | Date | Mon, 18 Jul 2016 15:44:20 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH] rcu_sync: simplify the state machine, introduce __rcu_sync_enter() |
| |
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.
| |