lkml.org 
[lkml]   [2016]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.

\
 
 \ /
  Last update: 2016-07-18 16:21    [W:0.105 / U:0.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site