* [PATCH] livepatch.h: Add comment to klp transition state
@ 2024-04-29 7:26 zhangwarden
2024-05-05 21:00 ` Josh Poimboeuf
0 siblings, 1 reply; 4+ messages in thread
From: zhangwarden @ 2024-04-29 7:26 UTC (permalink / raw
To: jpoimboe, mbenes, jikos, pmladek, joe.lawrence
Cc: live-patching, linux-kernel, Wardenjohn
From: Wardenjohn <zhangwarden@gmail.com>
livepatch.h use KLP_UNDEFINED\KLP_UNPATCHED\KLP_PATCHED for klp transition state.
When livepatch is ready but idle, using KLP_UNDEFINED seems very confusing.
In order not to introduce potential risks to kernel, just update comment
to these state.
---
include/linux/livepatch.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 9b9b38e89563..b6a214f2f8e3 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -18,9 +18,9 @@
#if IS_ENABLED(CONFIG_LIVEPATCH)
/* task patch states */
-#define KLP_UNDEFINED -1
-#define KLP_UNPATCHED 0
-#define KLP_PATCHED 1
+#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 */
/**
* struct klp_func - function structure for live patching
--
2.37.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] livepatch.h: Add comment to klp transition state
2024-04-29 7:26 [PATCH] livepatch.h: Add comment to klp transition state zhangwarden
@ 2024-05-05 21:00 ` Josh Poimboeuf
2024-05-06 2:04 ` zhang warden
0 siblings, 1 reply; 4+ messages in thread
From: Josh Poimboeuf @ 2024-05-05 21:00 UTC (permalink / raw
To: zhangwarden
Cc: mbenes, jikos, pmladek, joe.lawrence, live-patching, linux-kernel
On Mon, Apr 29, 2024 at 03:26:28PM +0800, zhangwarden@gmail.com wrote:
> From: Wardenjohn <zhangwarden@gmail.com>
>
> livepatch.h use KLP_UNDEFINED\KLP_UNPATCHED\KLP_PATCHED for klp transition state.
> When livepatch is ready but idle, using KLP_UNDEFINED seems very confusing.
> In order not to introduce potential risks to kernel, just update comment
> to these state.
> ---
> include/linux/livepatch.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 9b9b38e89563..b6a214f2f8e3 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -18,9 +18,9 @@
> #if IS_ENABLED(CONFIG_LIVEPATCH)
>
> /* task patch states */
> -#define KLP_UNDEFINED -1
> -#define KLP_UNPATCHED 0
> -#define KLP_PATCHED 1
> +#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 */
Instead of the comments, how about we just rename them to
KLP_TRANSITION_IDLE
KLP_TRANSITION_UNPATCHED
KLP_TRANSITION_PATCHED
which shouldn't break userspace AFAIK.
--
Josh
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] livepatch.h: Add comment to klp transition state
2024-05-05 21:00 ` Josh Poimboeuf
@ 2024-05-06 2:04 ` zhang warden
2024-05-06 8:37 ` Petr Mladek
0 siblings, 1 reply; 4+ messages in thread
From: zhang warden @ 2024-05-06 2:04 UTC (permalink / raw
To: Josh Poimboeuf
Cc: mbenes, jikos, pmladek, joe.lawrence, live-patching, linux-kernel
> On May 6, 2024, at 05:00, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
>
> On Mon, Apr 29, 2024 at 03:26:28PM +0800, zhangwarden@gmail.com wrote:
>> From: Wardenjohn <zhangwarden@gmail.com>
>>
>> livepatch.h use KLP_UNDEFINED\KLP_UNPATCHED\KLP_PATCHED for klp transition state.
>> When livepatch is ready but idle, using KLP_UNDEFINED seems very confusing.
>> In order not to introduce potential risks to kernel, just update comment
>> to these state.
>> ---
>> include/linux/livepatch.h | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 9b9b38e89563..b6a214f2f8e3 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -18,9 +18,9 @@
>> #if IS_ENABLED(CONFIG_LIVEPATCH)
>>
>> /* task patch states */
>> -#define KLP_UNDEFINED -1
>> -#define KLP_UNPATCHED 0
>> -#define KLP_PATCHED 1
>> +#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 */
>
> Instead of the comments, how about we just rename them to
>
> KLP_TRANSITION_IDLE
> KLP_TRANSITION_UNPATCHED
> KLP_TRANSITION_PATCHED
>
> which shouldn't break userspace AFAIK.
>
> --
> Josh
Hi Josh!
Renaming them may be a better way as my previous patch. I would like to know why renaming KLP_*** into
KLP_TRANSITION_*** will not break userspace while
Renaming KLP_UNDEWFINED to KLP_IDLE would break the userspace.
Meanwhile, I will resubmit another patch renaming the macros as your suggestion ASAP. Thank ~~ :)
--
Wardenjohn
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] livepatch.h: Add comment to klp transition state
2024-05-06 2:04 ` zhang warden
@ 2024-05-06 8:37 ` Petr Mladek
0 siblings, 0 replies; 4+ messages in thread
From: Petr Mladek @ 2024-05-06 8:37 UTC (permalink / raw
To: zhang warden
Cc: Josh Poimboeuf, mbenes, jikos, joe.lawrence, live-patching,
linux-kernel
On Mon 2024-05-06 10:04:26, zhang warden wrote:
>
>
> > On May 6, 2024, at 05:00, Josh Poimboeuf <jpoimboe@kernel.org> wrote:
> >
> > On Mon, Apr 29, 2024 at 03:26:28PM +0800, zhangwarden@gmail.com wrote:
> >> From: Wardenjohn <zhangwarden@gmail.com>
> >>
> >> livepatch.h use KLP_UNDEFINED\KLP_UNPATCHED\KLP_PATCHED for klp transition state.
> >> When livepatch is ready but idle, using KLP_UNDEFINED seems very confusing.
> >> In order not to introduce potential risks to kernel, just update comment
> >> to these state.
> >> ---
> >> include/linux/livepatch.h | 6 +++---
> >> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> >> index 9b9b38e89563..b6a214f2f8e3 100644
> >> --- a/include/linux/livepatch.h
> >> +++ b/include/linux/livepatch.h
> >> @@ -18,9 +18,9 @@
> >> #if IS_ENABLED(CONFIG_LIVEPATCH)
> >>
> >> /* task patch states */
> >> -#define KLP_UNDEFINED -1
> >> -#define KLP_UNPATCHED 0
> >> -#define KLP_PATCHED 1
> >> +#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 */
> >
> > Instead of the comments, how about we just rename them to
> >
> > KLP_TRANSITION_IDLE
> > KLP_TRANSITION_UNPATCHED
> > KLP_TRANSITION_PATCHED
> >
> > which shouldn't break userspace AFAIK.
Great idea! It is better then nothing.
> Renaming them may be a better way as my previous patch. I would like to know why renaming KLP_*** into
> KLP_TRANSITION_*** will not break userspace while
> Renaming KLP_UNDEWFINED to KLP_IDLE would break the userspace.
As I already wrote in [1], both "task->patch_state == KLP_UNDEFINED"
and "KLP_IDLE" are misleading. They are not talking
about the state of the patch but about the state of the transition.
We could not rename the variables because it would break userspace.
But we could rename the state names at least.
[1] https://lore.kernel.org/r/Zg7EpZol5jB_gHH9@alley
Best Regards,
Petr
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-05-06 8:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-29 7:26 [PATCH] livepatch.h: Add comment to klp transition state zhangwarden
2024-05-05 21:00 ` Josh Poimboeuf
2024-05-06 2:04 ` zhang warden
2024-05-06 8:37 ` Petr Mladek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).