Hi Joe and Petr :
> On Apr 5, 2024, at 01:50, Joe Lawrence <joe.lawrence@xxxxxxxxxx> wrote:
>
> On 4/4/24 11:17, Petr Mladek wrote:
>> 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
>>
>
> Yes, this is exactly how I think of these when reading the code. The
> model starts to make a lot more sense once you look at it thru this lens :)
>
For Joe's questions:
1. I do want to replace KLP_UNDEFINED with KLP_IDLE for livepatch patch states are all known instead of undefined.
2. The reason why I tried to make “KLP_IDLE" state into the same value of “KLP_UNDEFINED" is to make it compatible to “KLP_UNDEFINE"
Since Petr said that this will break some userspace tools, maybe there may have a chance to fix it in the future? If you think it bring more harm than good.
>> 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.
>>
Maybe we can just fix the code state instead of renaming the proc interface?
>
> Agreed. Instead of changing code and the sysfs interface, we could
> still add comments like:
>
> /* task patch transition target states */
> #define KLP_UNDEFINED -1 /* idle, no transition in progress */
> #define KLP_UNPATCHED 0 /* transitioning to unpatched state */
> #define KLP_PATCHED 1 /* transitioning to patched state */
>
> /* klp transition target state */
> static int klp_target_state = KLP_UNDEFINED;
>
> struct task_struct = {
> .patch_state = KLP_UNDEFINED, /* klp transition state */
>
> Maybe just one comment is enough? Alternatively, we could elaborate in
> Documentation/livepatch/livepatch.rst if it's really confusing.
>
> Wardenjohn, since you're probably reading this code with fresh(er) eyes,
> would any of the above be helpful?
>
> --
> Joe
Adding comments will help to understand the code logic. If introducing “KLP_IDLE" is not suitable right now, I am still happy to add some comment into the code if you agree.
Best Regards,
Wardenjohn
