On Thu, Apr 16, 2020 at 05:18:24PM +0200, Sebastian Andrzej Siewior wrote: > On 2020-04-16 10:42:54 [-0400], Joel Fernandes wrote: > > Hi Sebastian, > Hi Joel, > > > > @@ -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); > > > > I agree with the patch, except for this bit. Would it be ok to split the > > other parts of this patch into separate patch(es)? > > if you want, I could split it. Part of the goal is to get rid of the > local_irq_save(). I don't mind if it happens a patch later :) Considering the other parts of your patch are less contentious, it makes sense to me to split it, while we discuss this particular part ;) > > For this part of the patch, I am wondering what is the value in it. For one, > > local_irq_save() + spin_lock() is the problem, see > https://www.kernel.org/doc/html/latest/locking/locktypes.html#spinlock-t-and-rwlock-t > > (the link-instead-explaining part is nice) This problem can be fixed simply by using raw_spin_lock above right? That would solve the rtmutex problem in PREEMPT_RT. > > Also on a slightly related note, could you share with me why this_cpu_ptr() > > is unsafe to call in non-preemptible context, but raw_cpu_ptr() is ok? Just > > curious on that. Here I meant 'in preemptible context'. > > I wouldn't use safe/unsafe as a term to describe the situation. Both do > the same however this_cpu_ptr() raises a warning (given it is enabled) > if the current context can migrate to another CPU > (check_preemption_disabled()). > If you know what you do and it does not matter whether migration happens > or not, you can use raw_cpu_ptr() to avoid the warning. In this case it > is okay because CPU migration does not matter here since the data > structure is protected with a lock. Using the data structure from > another CPU (due to accidental migration) is not ideal but _no_ > assumption happens later on for dealing with the current or further > data. The timer/worker will be scheduled on the "wrong" CPU but this > again has no bad consequences. The timer and worker use container_of() > so they operate on the correct resources. Would they instead use > per_cpu_ptr(krc, smp_processor_id()) then it would operate on the wrong > data. > > Regarding the safe/unsafe: Would this_cpu_ptr() trigger a warning and > you add preempt_disable() around the block, where you use the per-CPU > data, then you would silence the warning and everything would look fine. > If this per-CPU data structure would also be accessed from interrupt > context then you would get inconsistent data and no warning. > What I'm trying to say is that even this_cpu_ptr() is not "safe" if used > without a thought. Thanks a lot for the background information on the 2 APIs!! It got confusing to me when rcu_raw_ptr() would not warn but this_cpu_ptr)_ would. I guess it is about semantics, and this_cpu_ptr() is more explicit about accessing the same local CPU's data. - Joel > > > thanks, > > > > - Joel > > Sebastian
