On Thu, Jan 28, 2021 at 11:12:28AM -0800, Paul E. McKenney wrote:
On Thu, Jan 28, 2021 at 06:12:10PM +0100, Frederic Weisbecker wrote:
Simply checking if the segcblist is enabled is enough to know if we need to initialize it or not. It's safe to check within hotplug machine.
Signed-off-by: Frederic Weisbecker frederic@kernel.org Cc: Josh Triplett josh@joshtriplett.org Cc: Lai Jiangshan jiangshanlai@gmail.com Cc: Joel Fernandes joel@joelfernandes.org Cc: Neeraj Upadhyay neeraju@codeaurora.org Cc: Boqun Feng boqun.feng@gmail.com
Hmmm...
At the start of a CPU-hotplug operation, an incoming CPU's callback list can be in a number of states:
Disabled and empty. This is the case when the boot CPU has not done call_rcu(), when a non-boot CPU first comes online, and when a non-offloaded CPU comes back online. In this case, it is permissible to initialize ->cblist. Because either the CPU is currently running with interrupts disabled (boot CPU) or is not yet running at all (other CPUs), it is not necessary to acquire ->nocb_lock.
Disabled and non-empty. This is the case when the boot CPU has done call_rcu(). It is not permissible to initialize ->cblist because doing so will leak any callbacks posted by early boot invocations of call_rcu().
I don't think that's possible. In this case __call_rcu() has called rcu_segcblist_init() and has enabled the segcblist.
Test for the possibility of leaking by building with CONFIG_PROVE_RCU=y and booting with rcupdate.rcu_self_test=1.
- Enabled, whether empty or not. This is the case when an offloaded CPU comes back online. This is the only case where the ->nocb_lock must be held to modify ->cblist. However, it is not necessarily to modify ->cblist because the rcuoc kthread is on the job.
So I believe that it is necessary to check for both disabled and empty. But don't take my word for it! Build with CONFIG_PROVE_RCU=y and boot with rcupdate.rcu_self_test=1. ;-)
I'm trying that :-)
On Thu, Jan 28, 2021 at 10:34:13PM +0100, Frederic Weisbecker wrote:
On Thu, Jan 28, 2021 at 11:12:28AM -0800, Paul E. McKenney wrote:
On Thu, Jan 28, 2021 at 06:12:10PM +0100, Frederic Weisbecker wrote:
Simply checking if the segcblist is enabled is enough to know if we need to initialize it or not. It's safe to check within hotplug machine.
Signed-off-by: Frederic Weisbecker frederic@kernel.org Cc: Josh Triplett josh@joshtriplett.org Cc: Lai Jiangshan jiangshanlai@gmail.com Cc: Joel Fernandes joel@joelfernandes.org Cc: Neeraj Upadhyay neeraju@codeaurora.org Cc: Boqun Feng boqun.feng@gmail.com
Hmmm...
At the start of a CPU-hotplug operation, an incoming CPU's callback list can be in a number of states:
Disabled and empty. This is the case when the boot CPU has not done call_rcu(), when a non-boot CPU first comes online, and when a non-offloaded CPU comes back online. In this case, it is permissible to initialize ->cblist. Because either the CPU is currently running with interrupts disabled (boot CPU) or is not yet running at all (other CPUs), it is not necessary to acquire ->nocb_lock.
Disabled and non-empty. This is the case when the boot CPU has done call_rcu(). It is not permissible to initialize ->cblist because doing so will leak any callbacks posted by early boot invocations of call_rcu().
I don't think that's possible. In this case __call_rcu() has called rcu_segcblist_init() and has enabled the segcblist.
You are right, rcu_segcblist_init() would have been called in that case and it has: rcu_segcblist_set_flags(rsclp, SEGCBLIST_ENABLED).
Test for the possibility of leaking by building with CONFIG_PROVE_RCU=y and booting with rcupdate.rcu_self_test=1.
- Enabled, whether empty or not. This is the case when an offloaded CPU comes back online. This is the only case where the ->nocb_lock must be held to modify ->cblist. However, it is not necessarily to modify ->cblist because the rcuoc kthread is on the job.
So I believe that it is necessary to check for both disabled and empty. But don't take my word for it! Build with CONFIG_PROVE_RCU=y and boot with rcupdate.rcu_self_test=1. ;-)
I'm trying that :-)
Even better!
Thanx, Paul
On Thu, Jan 28, 2021 at 01:45:24PM -0800, Paul E. McKenney wrote:
On Thu, Jan 28, 2021 at 10:34:13PM +0100, Frederic Weisbecker wrote:
On Thu, Jan 28, 2021 at 11:12:28AM -0800, Paul E. McKenney wrote:
On Thu, Jan 28, 2021 at 06:12:10PM +0100, Frederic Weisbecker wrote:
Simply checking if the segcblist is enabled is enough to know if we need to initialize it or not. It's safe to check within hotplug machine.
Signed-off-by: Frederic Weisbecker frederic@kernel.org Cc: Josh Triplett josh@joshtriplett.org Cc: Lai Jiangshan jiangshanlai@gmail.com Cc: Joel Fernandes joel@joelfernandes.org Cc: Neeraj Upadhyay neeraju@codeaurora.org Cc: Boqun Feng boqun.feng@gmail.com
Hmmm...
At the start of a CPU-hotplug operation, an incoming CPU's callback list can be in a number of states:
Disabled and empty. This is the case when the boot CPU has not done call_rcu(), when a non-boot CPU first comes online, and when a non-offloaded CPU comes back online. In this case, it is permissible to initialize ->cblist. Because either the CPU is currently running with interrupts disabled (boot CPU) or is not yet running at all (other CPUs), it is not necessary to acquire ->nocb_lock.
Disabled and non-empty. This is the case when the boot CPU has done call_rcu(). It is not permissible to initialize ->cblist because doing so will leak any callbacks posted by early boot invocations of call_rcu().
I don't think that's possible. In this case __call_rcu() has called rcu_segcblist_init() and has enabled the segcblist.
You are right, rcu_segcblist_init() would have been called in that case and it has: rcu_segcblist_set_flags(rsclp, SEGCBLIST_ENABLED).
Test for the possibility of leaking by building with CONFIG_PROVE_RCU=y and booting with rcupdate.rcu_self_test=1.
- Enabled, whether empty or not. This is the case when an offloaded CPU comes back online. This is the only case where the ->nocb_lock must be held to modify ->cblist. However, it is not necessarily to modify ->cblist because the rcuoc kthread is on the job.
So I believe that it is necessary to check for both disabled and empty. But don't take my word for it! Build with CONFIG_PROVE_RCU=y and boot with rcupdate.rcu_self_test=1. ;-)
I'm trying that :-)
Even better!
I applied this patch (with the usual wordsmithing as shown below), then ran this:
tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 2 --configs "TREE05" --bootargs "rcu_nocbs=0-7" --trust-make
The resulting console.log file says "Running RCU self tests" and the test runs to completion without complaint. So good show!
Thanx, Paul
------------------------------------------------------------------------
commit 0a43799de756a486e45eba8d9d4286a577e746cd Author: Frederic Weisbecker frederic@kernel.org Date: Thu Jan 28 18:12:10 2021 +0100
rcu/nocb: Only (re-)initialize segcblist when needed on CPU up
At the start of a CPU-hotplug operation, the incoming CPU's callback list can be in a number of states:
1. Disabled and empty. This is the case when the boot CPU has not invoked call_rcu(), when a non-boot CPU first comes online, and when a non-offloaded CPU comes back online. In this case, it is both necessary and permissible to initialize ->cblist. Because either the CPU is currently running with interrupts disabled (boot CPU) or is not yet running at all (other CPUs), it is not necessary to acquire ->nocb_lock.
In this case, initialization is required.
2. Disabled and non-empty. This cannot occur, because early boot call_rcu() invocations enable the callback list before enqueuing their callback.
3. Enabled, whether empty or not. In this case, the callback list has already been initialized. This case occurs when the boot CPU has executed an early boot call_rcu() and also when an offloaded CPU comes back online. In both cases, there is no need to initialize the callback list: In the boot-CPU case, the CPU has not (yet) gone offline, and in the offloaded case, the rcuo kthreads are taking care of business.
Because it is not necessary to initialize the callback list, it is also not necessary to acquire ->nocb_lock.
Therefore, checking if the segcblist is enabled suffices. This commit therefore initializes the callback list at rcutree_prepare_cpu() time only if that list is disabled.
Signed-off-by: Frederic Weisbecker frederic@kernel.org Cc: Josh Triplett josh@joshtriplett.org Cc: Lai Jiangshan jiangshanlai@gmail.com Cc: Joel Fernandes joel@joelfernandes.org Cc: Neeraj Upadhyay neeraju@codeaurora.org Cc: Boqun Feng boqun.feng@gmail.com Signed-off-by: Paul E. McKenney paulmck@kernel.org
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c index 00059df..70ddc33 100644 --- a/kernel/rcu/tree.c +++ b/kernel/rcu/tree.c @@ -4064,14 +4064,13 @@ int rcutree_prepare_cpu(unsigned int cpu) rdp->dynticks_nesting = 1; /* CPU not up, no tearing. */ rcu_dynticks_eqs_online(); raw_spin_unlock_rcu_node(rnp); /* irqs remain disabled. */ + /* - * Lock in case the CB/GP kthreads are still around handling - * old callbacks. + * Only non-NOCB CPUs that didn't have early-boot callbacks need to be + * (re-)initialized. */ - rcu_nocb_lock(rdp); - if (rcu_segcblist_empty(&rdp->cblist)) /* No early-boot CBs? */ + if (!rcu_segcblist_is_enabled(&rdp->cblist)) rcu_segcblist_init(&rdp->cblist); /* Re-enable callbacks. */ - rcu_nocb_unlock(rdp);
/* * Add CPU to leaf rcu_node pending-online bitmask. Any needed
linux-stable-mirror@lists.linaro.org
-
Frederic Weisbecker -
Paul E. McKenney