Received-SPF: pass (google.com: domain of linux-kernel+bounces-57877-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 8 Feb 2024 11:43:10 +0100
From: Frederic Weisbecker <frederic@kernel.org>
To: "Paul E. McKenney" <paulmck@kernel.org>
Cc: Boqun Feng <boqun.feng@gmail.com>, linux-kernel@vger.kernel.org,
	rcu@vger.kernel.org, linux-doc@vger.kernel.org,
	Chen Zhongjin <chenzhongjin@huawei.com>,
	Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Juri Lelli <juri.lelli@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	Valentin Schneider <vschneid@redhat.com>,
	Neeraj Upadhyay <neeraj.iitr10@gmail.com>,
	Joel Fernandes <joel@joelfernandes.org>,
	Josh Triplett <josh@joshtriplett.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Lai Jiangshan <jiangshanlai@gmail.com>,
	Zqiang <qiang.zhang1211@gmail.com>,
	Kent Overstreet <kent.overstreet@linux.dev>,
	Andrew Morton <akpm@linux-foundation.org>,
	Heiko Carstens <hca@linux.ibm.com>, Arnd Bergmann <arnd@arndb.de>,
	Oleg Nesterov <oleg@redhat.com>,
	Christian Brauner <brauner@kernel.org>,
	Suren Baghdasaryan <surenb@google.com>,
	Mike Christie <michael.christie@oracle.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Mateusz Guzik <mjguzik@gmail.com>,
	Nicholas Piggin <npiggin@gmail.com>,
	Peng Zhang <zhangpeng.00@bytedance.com>
Subject: Re: [PATCH 2/2] rcu-tasks: Eliminate deadlocks involving do_exit()
 and RCU tasks
Message-ID: <ZcSwPifss+ho3hRt@lothringen>
References: <20240129225730.3168681-1-boqun.feng@gmail.com>
 <20240129225730.3168681-3-boqun.feng@gmail.com>
 <ZcQJ2Vec1_b5ooS_@pavilion.home>
 <ZcQzyhcaRUSRo8a9@pavilion.home>
 <ZcQ4GMEbLFtkCZXw@pavilion.home>
 <cc25e968-6f43-453e-be9e-2851db39218f@paulmck-laptop>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <cc25e968-6f43-453e-be9e-2851db39218f@paulmck-laptop>

On Thu, Feb 08, 2024 at 01:56:10AM -0800, Paul E. McKenney wrote:
> On Thu, Feb 08, 2024 at 03:10:32AM +0100, Frederic Weisbecker wrote:
> This ordering is not needed.  The lock orders addition to this
> list against removal from tasklist.  If we hold this lock, either
> the task is already on this list or our holding this lock prevents
> it from removing itself from the tasklist.
> 
> We have already scanned the task list, and we have already done
> whatever update we are worried about.
> 
> So, if the task was on the tasklist when we scanned, well and
> good.  If the task was created after we scanned the tasklist,
> then it cannot possibly access whatever we removed.
> 
> But please double-check!!!

Heh, right, another new pattern for me to discover :-/

C r-LOCK

{
}

P0(spinlock_t *LOCK, int *X, int *Y)
{
	int r1;
	int r2;
	
	r1 = READ_ONCE(*X);

	spin_lock(LOCK);
	r2 = READ_ONCE(*Y);
	spin_unlock(LOCK);
}

P1(spinlock_t *LOCK, int *X, int *Y)
{
	spin_lock(LOCK);
	WRITE_ONCE(*Y, 1);
	spin_unlock(LOCK);
	WRITE_ONCE(*X, 1);
}

exists (0:r1=1 /\ 0:r2=0) (* never *)


> 
> > > synchronize_rcu_tasks()                       do_exit()
> > > ----------------------                        ---------
> > > //for_each_process_thread()
> > > READ tasklist                                 WRITE rtpcp->rtp_exit_list
> > > LOCK rtpcp->lock                              UNLOCK rtpcp->lock
> > > smp_mb__after_unlock_lock()                   WRITE tasklist //unhash_process()
> > > READ rtpcp->rtp_exit_list
> > > 
> > > Does this work? Hmm, I'll play with litmus once I have a fresh brain...
> 
> First, thank you very much for the review!!!
> 
> > ie: does smp_mb__after_unlock_lock() order only what precedes the UNLOCK with
> > the UNLOCK itself? (but then the UNLOCK itself can be reordered with anything
> > that follows)? Or does it also order what follows the UNLOCK with the UNLOCK
> > itself? If both, then it looks ok, otherwise...
> 
> If you have this:
> 
> 	earlier_accesses();
> 	spin_lock(...);
> 	ill_considered_memory_accesses();
> 	smp_mb__after_unlock_lock();
> 	later_accesses();
> 
> Then earlier_accesses() will be ordered against later_accesses(), but
> ill_considered_memory_accesses() won't necessarily be ordered.  Also,
> any accesses before any prior release of that same lock will be ordered
> against later_accesses().
> 
> (In real life, ill_considered_memory_accesses() will be fully ordered
> against either spin_lock() on the one hand or smp_mb__after_unlock_lock()
> on the other, with x86 doing the first and PowerPC doing the second.
> So please try to avoid any ill_considered_memory_accesses().)

Thanks a lot for that explanation!


> 
> > Also on the other end, does LOCK/smp_mb__after_unlock_lock() order against what
> > precedes the LOCK? That also is necessary for the above to work.
> 
> It looks like an smp_mb__after_spinlock() would also be needed, for
> example, on ARMv8.
> 
> > Of course by the time I'm writing this email, litmus would have told me
> > already...
> 
> ;-) ;-) ;-)
> 
> But I believe that simple locking covers this case.  Famous last words...

Indeed, looks right!

Thanks!
> 							Thanx, Paul