On Tue 2024-04-02 09:52:31, Joe Lawrence wrote: > On Tue, Apr 02, 2024 at 11:09:54AM +0800, zhangwarden@xxxxxxxxx wrote: > > From: Wardenjohn <zhangwarden@xxxxxxxxx> > > > > In livepatch, using KLP_UNDEFINED is seems to be confused. > > When kernel is ready, livepatch is ready too, which state is > > idle but not undefined. What's more, if one livepatch process > > is finished, the klp state should be idle rather than undefined. > > > > Therefore, using KLP_IDLE to replace KLP_UNDEFINED is much better > > in reading and understanding. > > --- > > include/linux/livepatch.h | 1 + > > kernel/livepatch/patch.c | 2 +- > > kernel/livepatch/transition.c | 24 ++++++++++++------------ > > 3 files changed, 14 insertions(+), 13 deletions(-) > > > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > > index 9b9b38e89563..c1c53cd5b227 100644 > > --- a/include/linux/livepatch.h > > +++ b/include/linux/livepatch.h > > @@ -19,6 +19,7 @@ > > > > /* task patch states */ > > #define KLP_UNDEFINED -1 > > +#define KLP_IDLE -1 > > Hi Wardenjohn, > > Quick question, does this patch intend to: > > - Completely replace KLP_UNDEFINED with KLP_IDLE > - Introduce KLP_IDLE as an added, fourth potential state > - Introduce KLP_IDLE as synonym of sorts for KLP_UNDEFINED under certain > conditions > > I ask because this patch leaves KLP_UNDEFINED defined and used in other > parts of the tree (ie, init/init_task.c), yet KLP_IDLE is added and > continues to use the same -1 enumeration. Having two names for the same state adds more harm than good. Honestly, neither "task->patch_state == KLP_UNDEFINED" nor "KLP_IDLE" make much sense. The problem is in the variable name. It is not a state of a patch. It is the state of the transition. The right solution would be something like: klp_target_state -> klp_transition_target_state task->patch_state -> task->klp_transition_state KLP_UNKNOWN -> KLP_IDLE But it would also require renaming: /proc/<pid>/patch_state -> klp_transition_state which might break userspace tools => likely not acceptable. My opinion: It would be nice to clean this up but it does not look worth the effort. Best Regards, Petr
