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

[Joel Fernandes <joel@joelfernandes.org>]

On Thu, Apr 16, 2020 at 02:41:12PM -0400, Joel Fernandes wrote:
> 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.

I guess a higher level question is, on PREEMPT_RT, do we want to make the
kfree_rcu() code fully preemptible?

I am not saying we should not, but that is another thing to consider.

Obviously, the migrate_disable(); idea would be better then if are Ok with
making it preemptible.

But, then will it be safe for kfree_rcu() callers from hard IRQ context to
call this in PREEMPT_RT? That could would just break then as you cannot sleep
in hard IRQ context even on PREEMPT_RT.

thanks,

 - Joel