Re: [PATCH 1/3] rcu: Use static initializer for krc.lock

[Joel Fernandes <joel@joelfernandes.org>]

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