Received-SPF: pass (google.com: domain of linux-kernel+bounces-133838-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Content-Type: text/plain;
	charset=utf-8
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.500.231\))
Subject: Re: [PATCH] livepatch: Add KLP_IDLE state
From: zhang warden <zhangwarden@gmail.com>
In-Reply-To: <4506bb59-5524-8b0e-f97c-a0252d4784ad@redhat.com>
Date: Sat, 6 Apr 2024 15:36:37 +0800
Cc: Petr Mladek <pmladek@suse.com>,
 jpoimboe@kernel.org,
 mbenes@suse.cz,
 jikos@kernel.org,
 live-patching@vger.kernel.org,
 linux-kernel@vger.kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <EF3BD11E-D4B9-4490-91FC-41066452D32B@gmail.com>
References: <20240402030954.97262-1-zhangwarden@gmail.com>
 <ZgwNn5+/Ryh05OOm@redhat.com> <Zg7EpZol5jB_gHH9@alley>
 <4506bb59-5524-8b0e-f97c-a0252d4784ad@redhat.com>
To: Joe Lawrence <joe.lawrence@redhat.com>


Hi Joe and Petr :
> On Apr 5, 2024, at 01:50, Joe Lawrence <joe.lawrence@redhat.com> =
wrote:
>=20
> 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@gmail.com =
wrote:
>>>> From: Wardenjohn <zhangwarden@gmail.com>
>>>>=20
>>>> 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.
>>>>=20
>>>> 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(-)
>>>>=20
>>>> 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 @@
>>>>=20
>>>> /* task patch states */
>>>> #define KLP_UNDEFINED -1
>>>> +#define KLP_IDLE       -1
>>>=20
>>> Hi Wardenjohn,
>>>=20
>>> Quick question, does this patch intend to:
>>>=20
>>> - 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
>>>=20
>>> 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.
>>=20
>> Having two names for the same state adds more harm than good.
>>=20
>> Honestly, neither "task->patch_state =3D=3D KLP_UNDEFINED" nor =
"KLP_IDLE"
>> make much sense.
>>=20
>> 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:
>>=20
>>  klp_target_state -> klp_transition_target_state
>>  task->patch_state -> task->klp_transition_state
>>  KLP_UNKNOWN -> KLP_IDLE
>>=20
>=20
> 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 :)
>=20

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 =E2=80=9CKLP_IDLE" state into the same =
value of =E2=80=9CKLP_UNDEFINED" is to make it compatible to =
=E2=80=9CKLP_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:
>>=20
>>  /proc/<pid>/patch_state -> klp_transition_state
>>=20
>> which might break userspace tools =3D> likely not acceptable.
>>=20
>>=20
>> My opinion:
>>=20
>> It would be nice to clean this up but it does not look worth the
>> effort.
>>=20

Maybe we can just fix the code state instead of renaming the proc =
interface?

>=20
> Agreed.  Instead of changing code and the sysfs interface, we could
> still add comments like:
>=20
>  /* 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 */
>=20
>  /* klp transition target state */
>  static int klp_target_state =3D KLP_UNDEFINED;
>=20
>  struct task_struct =3D {
>      .patch_state    =3D KLP_UNDEFINED,   /* klp transition state */
>=20
> Maybe just one comment is enough?  Alternatively, we could elaborate =
in
> Documentation/livepatch/livepatch.rst if it's really confusing.
>=20
> Wardenjohn, since you're probably reading this code with fresh(er) =
eyes,
> would any of the above be helpful?
>=20
> --=20
> Joe

Adding comments will help to understand the code logic. If introducing =
=E2=80=9CKLP_IDLE" is not suitable right now, I am still happy to add =
some comment into the code if you agree.

Best Regards,
Wardenjohn