[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20230224164215.ubnftmmhqqkh5zps@treble>
Date: Fri, 24 Feb 2023 08:42:15 -0800
From: Josh Poimboeuf <jpoimboe@...nel.org>
To: Petr Mladek <pmladek@...e.com>
Cc: live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
Seth Forshee <sforshee@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Song Liu <song@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Joe Lawrence <joe.lawrence@...hat.com>,
Miroslav Benes <mbenes@...e.cz>,
Jiri Kosina <jikos@...nel.org>, Ingo Molnar <mingo@...hat.com>,
Rik van Riel <riel@...riel.com>
Subject: Re: [PATCH v2 2/3] livepatch,sched: Add livepatch task switching to
cond_resched()
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
Powered by blists - more mailing lists