Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 29 Jul 2021 12:53:31 +0200
From:   Frederic Weisbecker <frederic@kernel.org>
To:     Boqun Feng <boqun.feng@gmail.com>
Cc:     "Paul E. McKenney" <paulmck@kernel.org>, rcu@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel-team@fb.com, mingo@kernel.org,
        jiangshanlai@gmail.com, akpm@linux-foundation.org,
        mathieu.desnoyers@efficios.com, josh@joshtriplett.org,
        tglx@linutronix.de, peterz@infradead.org, rostedt@goodmis.org,
        dhowells@redhat.com, edumazet@google.com, fweisbec@gmail.com,
        oleg@redhat.com, joel@joelfernandes.org,
        Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH rcu 04/18] rcu: Weaken ->dynticks accesses and updates
Message-ID: <20210729105331.GA301667@lothringen>
References: <20210721202042.GA1472052@paulmck-ThinkPad-P17-Gen-1>
 <20210721202127.2129660-4-paulmck@kernel.org>
 <YQJfjFv8lOnkUkhs@boqun-archlinux>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YQJfjFv8lOnkUkhs@boqun-archlinux>
Precedence: bulk

On Thu, Jul 29, 2021 at 03:58:04PM +0800, Boqun Feng wrote:
> > The following litmus test, also adapted from the one supplied off-list
> > by Frederic Weisbecker, models the RCU grace-period kthread detecting
> > a non-idle CPU that is concurrently transitioning to idle:
> > 
> > 	C dynticks-into-idle
> > 
> > 	{
> > 		DYNTICKS=1; (* Initially non-idle. *)
> > 	}
> > 
> > 	P0(int *X, int *DYNTICKS)
> > 	{
> > 		int dynticks;
> > 
> > 		// Non-idle.
> > 		WRITE_ONCE(*X, 1);
> > 		dynticks = READ_ONCE(*DYNTICKS);
> > 		smp_store_release(DYNTICKS, dynticks + 1);
> > 		smp_mb();
> 
> this smp_mb() is not needed, as we rely on the release-acquire pair to
> provide the ordering.
> 
> This means that if we use different implementations (one w/ smp_mb(),
> another w/o) rcu_dynticks_inc() for idle-to-nonidle and nonidle-to-idle,
> we could save a smp_mb(). Thoughts?

That's exactly what I wanted to propose but everybody was sober. Namely order
only the RCU read side critical sections before/after idle together:

     READ side critical section
     //enter idle
     //exit idle
     smp_mb()
     READ side critical section

instead of ordering the RCU read side critical section before idle - with the RCU
idle extended quiescent state - with the RCU read side critical section after idle:

     READ side critical section
     //enter idle
     smp_mb();
     //exit idle
     smp_mb()
     READ side critical section

So the side effect now is that if the write side waits for the reader to
report a quiescent state and scans its dynticks state and see it's not yet in
RCU idle mode, then later on when the read side enters in RCU idle mode we
expect it to see the write side updates.
But after the barrier removal the reader will only see the write side update
once we exit RCU idle mode.

So the following may happen:

	P0(int *X, int *Y, int *DYNTICKS)
	{
		int y;

		WRITE_ONCE(*X, 1);
		smp_store_release(DYNTICKS, 1); // rcu_eqs_enter
		//smp_mb() not there anymore
		y = READ_ONCE(*Y);
		smp_store_release(DYNTICKS, 2); // rcu_eqs_exit()
		smp_mb();
	}

	P1(int *X, int *Y, int *DYNTICKS)
	{
		int x;
		int dynticks;
		
		WRITE_ONCE(*Y, 1);
		smp_mb();
		dynticks = smp_load_acquire(DYNTICKS);
		x = READ_ONCE(*X);
	}

	exists (1:x=0 /\ 0:y=0)

Theoretically it shouldn't matter because the RCU idle mode isn't
supposed to perform RCU reads. But theoretically again once a CPU
has reported a quiescent state, any further read is expected to see
the latest updates from the write side.

So I don't know what to think. In practice I believe it's not a big deal
because RCU idle mode code is usually a fragile path that just handles
cpuidle code to put the CPU in/out low power mode. But what about dragons...