On 12/06/2018 05:14 AM, Petr Mladek wrote: > On Thu 2018-12-06 10:23:40, Miroslav Benes wrote: >> On Thu, 6 Dec 2018, Petr Mladek wrote: >> >>> On Wed 2018-12-05 14:32:53, Joe Lawrence wrote: >>>>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c >>>>> index 972520144713..e01dfa3b58d2 100644 >>>>> --- a/kernel/livepatch/core.c >>>>> +++ b/kernel/livepatch/core.c >>>>> @@ -45,7 +45,7 @@ >>>>> */ >>>>> DEFINE_MUTEX(klp_mutex); >>>>> >>>>> -/* Registered patches */ >>>>> +/* Actively used patches. */ >>>>> LIST_HEAD(klp_patches); >>>> >>>> By itself, this comment makes me wonder if there are un-active and/or >>>> un-used patches that I need to worry about. After this patchset, >>>> klp_patches will include patches that have been enabled and those that >>>> have been replaced, but the replacement transition is still in progress. >>>> >>>> If that sounds accurate, how about adding to the comment: >>>> >>>> /* Actively used patches: enabled or replaced and awaiting transition */ >>> >>> The replaced patches are not in the list. This is why I used the word >>> "actively". >> After writing out my suggestion I realized that's why you chose "actively" and almost erased my comment. I think the extra text would help a fresh reader of the code, so ... >> The replaced patches are removed in klp_discard_replaced_patches(), which >> is called from klp_complete_transition(). Joe is right. The patches are in >> the list if a transition is still in progress. > > These are patches that are being replaced. The replaced (after the > transition finishes) are not in the list. > > By other word, Joe's text could be understand that replaced patches > will never get removed from the list. > > So, is the text below acceptable? > > /* > * Actively used patches: enabled or in transition. Note that replaced > * or disabled patches are not listed even though the related kernel > * module still can be loaded. > */ Yes this works and is more accurate than my original suggestion. Thanks, -- Joe
