Received-SPF: pass (google.com: domain of linux-kernel+bounces-131912-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <4506bb59-5524-8b0e-f97c-a0252d4784ad@redhat.com>
Date: Thu, 4 Apr 2024 13:50:40 -0400
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Content-Language: en-US
To: Petr Mladek <pmladek@suse.com>, zhangwarden@gmail.com
Cc: jpoimboe@kernel.org, mbenes@suse.cz, jikos@kernel.org,
 live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240402030954.97262-1-zhangwarden@gmail.com>
 <ZgwNn5+/Ryh05OOm@redhat.com> <Zg7EpZol5jB_gHH9@alley>
From: Joe Lawrence <joe.lawrence@redhat.com>
Subject: Re: [PATCH] livepatch: Add KLP_IDLE state
In-Reply-To: <Zg7EpZol5jB_gHH9@alley>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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>
>>>
>>> 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 :)

> 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.
> 

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