Date:   Fri, 24 Feb 2023 08:42:15 -0800
From:   Josh Poimboeuf <jpoimboe@kernel.org>
To:     Petr Mladek <pmladek@suse.com>
Cc:     live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
        Seth Forshee <sforshee@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Song Liu <song@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Joe Lawrence <joe.lawrence@redhat.com>,
        Miroslav Benes <mbenes@suse.cz>,
        Jiri Kosina <jikos@kernel.org>, Ingo Molnar <mingo@redhat.com>,
        Rik van Riel <riel@surriel.com>
Subject: Re: [PATCH v2 2/3] livepatch,sched: Add livepatch task switching to
 cond_resched()
Message-ID: <20230224164215.ubnftmmhqqkh5zps@treble>
References: <cover.1676672328.git.jpoimboe@kernel.org>
 <9f09bff809fc026618108e8bbaac67ef2f8e6d3d.1676672328.git.jpoimboe@kernel.org>
 <Y/YgARbqsyvzebAl@alley>
 <20230224013402.hpq5ka4exetrdh4t@treble>
 <Y/jfUlPm7uCWvHPl@alley>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <Y/jfUlPm7uCWvHPl@alley>
Precedence: bulk

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