Re: [PATCH 3/3] livepatch: Avoid potential RCU stalls in klp transition

[Josh Poimboeuf <jpoimboe@kernel.org>]

On Wed, Feb 12, 2025 at 10:42:33AM +0800, Yafang Shao wrote:
> On Wed, Feb 12, 2025 at 8:52 AM Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> >
> > On Tue, Feb 11, 2025 at 02:24:37PM +0800, Yafang Shao wrote:
> > > +++ b/kernel/livepatch/transition.c
> > > @@ -491,9 +491,18 @@ void klp_try_complete_transition(void)
> > >                       complete = false;
> > >                       break;
> > >               }
> > > +
> > > +             /* Avoid potential RCU stalls */
> > > +             if (need_resched()) {
> > > +                     complete = false;
> > > +                     break;
> > > +             }
> > >       }
> > >       read_unlock(&tasklist_lock);
> > >
> > > +     /* The above operation might be expensive. */
> > > +     cond_resched();
> > > +
> >
> > This is also nasty, yet another reason to use rcu_read_lock() if we can.
> 
> The RCU stalls still happen even if we use rcu_read_lock() as it is
> still in the RCU read-side critical section.
> 
> >
> > Also, with the new lazy preemption model, I believe cond_resched() is
> > pretty much deprecated.
> 
> I'm not familiar with the newly introduced PREEMPT_LAZY, but it
> appears to be a configuration option. Therefore, we still need this
> cond_resched() for users who don't have PREEMPT_LAZY set as the
> default.

IIRC, the goal is to get rid of PREEMPT_NONE and PREEMPT_VOLUNTARY (and
PREEMPT_DYNAMIC) and to remove almost all the cond_resched() calls.  So
we should really avoid adding them at this point.

The patch already breaks out of the loop for need_resched(), is the
cond_resched() really needed there?  For the PREEMPT_VOLUNTARY case I
think it should already get preempted anyway when it releases the lock.

And regardless, by that point it's fairly close to scheduling the
delayed work and returning back to the user.  That could happen even
sooner by skipping the "swapper" task loop.

-- 
Josh