Re: [PATCH v2 2/3] livepatch,sched: Add livepatch task switching to cond_resched()

[Josh Poimboeuf]

On Fri, Feb 24, 2023 at 05:01:22PM +0100, Petr Mladek wrote:
> > @@ -381,6 +381,14 @@ void __klp_sched_try_switch(void)
> >  	if (unlikely(!klp_patch_pending(current)))
> >  		goto out;
> >  
> > +	/*
> > +	 * Enforce the order of the TIF_PATCH_PENDING read above and the
> > +	 * klp_target_state read in klp_try_switch_task().  The corresponding
> > +	 * write barriers are in klp_init_transition() and
> > +	 * klp_reverse_transition().
> > +	 */
> > +	smp_rmb();
> 
> This barrier has basically the same purpose as the implicit read
> barrier in klp_update_patch_state().
> 
> The comment in klp_update_patch_state() says that the read barrier
> actually has two purposes. The 1st one is easy. It is the one
> described above.
> 
> It took me quite some time to understand the 2nd purpose again.
> The original comment was:
> 
> 	 * 2) Enforce the order of the TIF_PATCH_PENDING read and a future read
> 	 *    of func->transition, if klp_ftrace_handler() is called later on
> 	 *    the same CPU.  See __klp_disable_patch().
> 
> I think that a better description would be:
> 
> 	 * 2) Make sure that this CPU sees func->transition enabled when
> 	 *    it sees the TIF_PATCH_PENDING enabled. This is important when
> 	 *    the current task is transitioning itself and then calls
> 	 *    klp_ftrace_handler() later. It ensures that the ftrace handler
> 	 *    would check the state change that we did here.
> 	 *    The corresponding write barrier is in __klp_enable_patch()
> 	 *    and __klp_disable_patch().
> 
> Note that the previous comment wasn't correct. IMHO, the related write
> barrier is needed in both __klp_enable_patch() and __klp_disable_patch().

That 2nd comment also confused me.  Yours is definitely better!

> > @@ -661,9 +670,19 @@ void klp_reverse_transition(void)
> >  	 */
> >  	klp_synchronize_transition();
> >  
> > -	/* All patching has stopped, now start the reverse transition. */
> > +	/* All patching has stopped, now start the reverse transition: */
> > +
> 
> Is the extra empty line intended?

Due to the additional comment and whitespace added below, I added
whitespace here to try to imply that the comment doesn't only apply to
the following two lines, but also the code after it.  I'm open to
suggestions :-)

> >  	klp_transition_patch->enabled = !klp_transition_patch->enabled;
> >  	klp_target_state = !klp_target_state;
> > +
> > +	/*
> > +	 * Enforce the order of the klp_target_state write and the
> > +	 * TIF_PATCH_PENDING writes in klp_start_transition() to ensure
> > +	 * klp_update_patch_state() and __klp_sched_try_switch() don't set
> > +	 * task->patch_state to the wrong value.
> > +	 */
> > +	smp_wmb();
> > +
> >  	klp_start_transition();
> >  }
> 
> This made me to revisit all the barriers in the livepatch code.
> The good thing is that it seems that all the barriers are correct,
> including the new ones proposed in this patchset.

That's good news :-)

> But some comments are a bit misleading. I would like to update
> them a bit. I have started working on it but it goes slowly.
> I often get lost...
> 
> I am not sure about the ordering. I do not want to block this patchset
> by the clean up of the comments. The currently proposed ones are
> good enough. Feel free to send v3.
> 
> Or would you prefer to wait for my clean up of the comments?

Sounds good, I'll send v3 soon and you can base your updates on top.

-- 
Josh