Re: [PATCH v1 18/19] Documentation/livepatch: Update documentation for state, callbacks, and shadow variables

[Joe Lawrence <joe.lawrence@redhat.com>]

On 1/15/25 03:24, Petr Mladek wrote:
> This commit updates the livepatch documentation to reflect recent changes
> in the behavior of states, callbacks, and shadow variables.
> 
> Key changes include:
> 
> - Per-state callbacks replace per-object callbacks, invoked only when a
>   livepatch introduces or removes a state.
> - Shadow variable lifetime is now tied to the corresponding livepatch
>   state lifetime.
> - The "version" field in `struct klp_state` has been replaced with the
>   "block_disable" flag for improved compatibility handling.
> - The "data" field has been removed from `struct klp_state`; shadow
>   variables are now the recommended way to store state-related data.
> 
> This update ensures the documentation accurately describes the current
> livepatch functionality.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Hi Petr, I'm finally getting around to looking through these patches.
I've made it as far as the selftest cleanups, then skipped ahead to the
Documentation here.  Before diving into code review, I just wanted to
clarify some things for my own understanding.  Please correct anything
below that is incorrect.  IMHO it is easy to step off the intended path
here :D

The original livepatch system states operated with a numeric .version
field.  New livepatches could only be loaded if providing a new set of
states, or an equal-or-better version of states already loaded by
existing livepatches.

With that in mind, a livepatch state could be thought of as an
indication of "a context needing special handling in a (versioned) way".
 Livepatches claiming to deal with a particular state, needed to do so
with its latest or current versioning.  A livepatch without a particular
state was not bound to any restriction on that state, so nothing
prevented subsequent atomic replace patches from blowing away existing
states (those patches cleaned up their states on their disabling), or
subsequent non-atomic replace patches from adding to the collective
livepatch state.


This patchset does away with .version and adds .block_disable.  This is
very different from versioning as prevents the associated state from
ever going away.  In an atomic-replace series of livepatches, this means
once a state is introduced, all following patches must contain that
state.  In non-atomic-replace series, there is no restriction on
subsequent patches, but the original patch introducing the state cannot
ever be disabled/unloaded.  (I'm not going to consider future hybrid
mixed atomic/not use cases as my brain is already full.)

Finally, the patchset adds .is_shadow and .callbacks.  A short sequence
of livepatches may look like:

  klp_patch A               |  klp_patch B
    .states[x]              |    .states[y]
      .id            = 42   |      .id            = 42
      .callbacks            |      .callbacks
      .block_disable        |      .block_disable
      .is_shadow            |      .is_shadow

is there any harm or confusion if the two patches' state 42 contained
disparate .callbacks, .block_disable, or .is_shadow contents?

I /think/ this is allowed by the patchset (though I haven't gotten too
deep in my understanding), but I feel that I'm starting to stretch my
intuitive understanding of these livepatching states.  Applying them to
a series of atomic-replace livepatches is fairly straightforward.  But
then for a non-atomic-replace series, it starts getting weird as
multiple callback sets will exist in multiple patches.

In a perfect world, we could describe livepatching states absent
callbacks and shadow variables.  The documentation is a bit circular as
one needs to understand them before fully grasping the purpose of the
states.  But to use them, you will first need to understand how to set
them up in the states. :)  Maybe there is no better way, but first I
need to correct my understanding.

Thanks,

-- 
Joe