From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751564AbcGRNoG (ORCPT <rfc822;w@1wt.eu>);
	Mon, 18 Jul 2016 09:44:06 -0400
Received: from mx1.redhat.com ([209.132.183.28]:52123 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1750998AbcGRNoF (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Mon, 18 Jul 2016 09:44:05 -0400
Date: Mon, 18 Jul 2016 15:44:20 +0200
From: Oleg Nesterov <oleg@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>, 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()
Message-ID: <20160718134419.GB25380@redhat.com>
References: <20160714184351.GA18388@redhat.com> <20160714192018.GM30154@twins.programming.kicks-ass.net> <20160715132709.GA27644@redhat.com> <20160715133928.GH7094@linux.vnet.ibm.com> <20160715134523.GB28589@redhat.com> <20160715153840.GI7094@linux.vnet.ibm.com> <20160715164938.GA4294@redhat.com> <20160715180140.GM7094@linux.vnet.ibm.com> <20160716171007.GA30000@redhat.com> <20160718115427.GB6862@twins.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160718115427.GB6862@twins.programming.kicks-ass.net>
User-Agent: Mutt/1.5.18 (2008-05-17)
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Mon, 18 Jul 2016 13:44:04 +0000 (UTC)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.