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
