On Thu, Apr 16, 2020 at 05:20:27PM +0200, Sebastian Andrzej Siewior wrote: > On 2020-04-16 17:01:43 [+0200], Uladzislau Rezki wrote: > > > > @@ -3139,10 +3136,8 @@ void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func) > > > > unsigned long flags; > > > > struct kfree_rcu_cpu *krcp; > > > > > > > > - local_irq_save(flags); // For safely calling this_cpu_ptr(). > > > > - krcp = this_cpu_ptr(&krc); > > > > - if (krcp->initialized) > > > > - spin_lock(&krcp->lock); > > > > + krcp = raw_cpu_ptr(&krc); > > > > + spin_lock_irqsave(&krcp->lock, flags); > > > > > It is not a good way to access to per-CPU variable. There is a race in > > your code. So, we will rework it anyway soon. To guarantee that we stay > > on the same CPU, first we disable IRQ's then we access per-CPU var and > > take a spinlock. > > What is the worst thing that can happen if a CPU migration happens > between raw_cpu_ptr() and spin_lock_irqsave() ? In the general case, not necessarily relevant to this code, one scenario requiring this sort of thing is where the critical section needs access to per-CPU data that cannot be accessed off-CPU, but needs to update data that is protected by a per-CPU lock. This works well when the data is frequently accessed by its own CPU, but nevertheless might be accessed at any time by some other CPU. Perhaps it is better to think of this as a specialized reader-writer lock protecting per-CPU data. A given CPU is always permitted to read its own data, as is any CPU holding that CPU's lock. To update its own data, a CPU must hold its own lock. Of course, readers in reader-writer locking often do updates. Given that caveat, an example of this pattern may be found in RCU's note_gp_changes() function. But RCU does plenty of acquisitions of irq-disabled locks where the task might be migrated during acquisition, for one example, please see RCU's rcu_report_qs_rdp() function. This is admittedly strictly speaking not a per-CPU case, but each rcu_node structure does have a subset of the CPUs assigned to it, and it is quite possible that the task might migrate to some CPU associated with some other rcu_node structure during the lock acquisition. That is OK, because the per-rcu_node data is strictly protected by this per-rcu_node lock. So, which category does kfree_call_rcu() fit into? Thanx, Paul
