Re: [PATCH v10 05/10] livepatch: Support separate list for replaced patches.

[Evgenii Shatokhin]

On 10.04.2018 16:21, Miroslav Benes wrote:
> > > > > I think you're missing my point.  This isn't about your patch set, per
> > > > > se.  It's really about our existing code.  Today, our patch stack
> > > > > doesn't follow real stack semantics, because patches in the middle might
> > > > > be disabled.  I see that as a problem.
> > >
> > > No, please read it again.  I wasn't talking about replaced patches.
> >
> > I was confused by wording "in the middle". It suggested that there
> > might had been enabled patches on the top and the bottom of the stack
> > and some disabled patches in between at the same time (or vice versa).
> > This was not true.
> >
> > Do I understand it correctly that you do not like that the patches
> > on the stack might be in two states (disabled/enabled). This might
> > be translated that you do not like the state when the patch is
> > registered and disabled.
> >
> > I wonder if the problem is in the "stack" abstraction. Would it help
> > if we call it "sorted list" or whatever more suitable?
> >
> > Another possibility would be to get rid of the enable/disable states.
> > I mean that the patch will be automatically enabled during
> > registration and removed during unregistration. Or we could rename
> > the operation do add/remove or anything else. In fact, this is how
> > it worked in kGraft.
>
> I've already wondered couple of times why we had separate enable/disable.
> If there is someone who knows, remind me, please. I wouldn't be against a
> simplification here.
>
> On the other hand, it is kind of nice to keep the registration and
> enablement separate. It is more flexible if someone needs it.
>
> Anyway, we should solve it together with the stacking. It is tightly
> connected.
>   
> > AFAIK, the enable/disabled state made more sense for immediate
> > patches that could not be unloaded. We still need to keep the patches
> > when the transaction is forced. The question is if we need to keep
> > the sysfs entries for loaded but unused patches.
> >
> >
> > > > > If 'replace' were the only mode, then we wouldn't even need a patch
> > > > > stack because it wouldn't really matter much whether the previous patch
> > > > > is enabled or disabled.  I think this is in agreement with the point
> > > > > you're making.
> > > > >
> > > > > But we still support non-replace patches.  My feeling is that we should
> > > > > either do a true stack, or no stack at all.  The in-between thing is
> > > > > going to be confusing, not only for us, but for patch authors and end
> > > > > users.
> > > >
> > > > I see it like two different modes. We either have a stack of patches
> > > > that depend on each other.
> > >
> > > But if they depend on each other, they can use 'replace' and a stack
> > > isn't needed.
> >
> > Yes but see below.
> >
> >
> > > And If they *don't* depend on each other, then the stack is overly
> > > restrictive, for no good reason.
> > >
> > > Either way, why do we still need a stack?
> >
> > Good question. I suggest to agree on some terms first:
> >
> >     + Independent patches make unrelated changes. Any new function
> >       must not rely on changes done by any other patch.
> >
> >     + Dependent patches mean that a later patch depend on changes
> >       done by an earlier patch. For example, a new function might
> >       use function added by an earlier patch.
> >
> >     + Each cumulative patch include all necessary changes. I would say
> >       that it is self-containing and independent. Except that they should
> >       be able to continue using changes made by earlier patches (shadow
> >       variables, callbacks).
> >
> >
> > Then we could say:
> >
> >     + The stack helps to enforce dependencies between dependent
> >       patches. But there is needed also some external solution
> >       that forces loading the patches in the right order.
> >
> >     + The "replace" feature is useful for cumulative patches.
> >       It allows to remove existing changes and remove unused stuff.
> >       But cumulative patches might be done even _without_ the atomic
> >       replace.
> >
> >     + Cumulative patches _with_ "replace" flag might be in theory
> >       installed in random order because they handle not-longer patched
> >       functions. Well, some incompatibility might be caused by shadow
> >       variables and callbacks. Therefore it still might make sense
> >       to install them in the right order.
> >
> >       Cumulative patches _without_ "replace" flag must be installed
> >       in the right order because they do not handle not-longer patched
> >       functions.
> >
> >       Anyway, for cumulative patches is important the external
> >       ordering (loading modules) and not the stack.
> >
> >
> > Back to your question:
> >
> > The stack is needed for dependent non-cumulative patches.
> >
> > The cumulative patches with "replace" flag seems the be
> > the most safe and most flexible solution. The question is
> > if we want to force all users to use this mode.
> >
> >
> > > > Or we have replace patches that are
> > > > standalone and we allow a smooth transfer from one to another one.
> > > >
> > > > Anyway, for us, it is much more important the removal of replaced
> > > > patches. We could probably live without the possibility to replace
> > > > disabled patches.
> > >
> > > I think replacing disabled patches is ok, *if* we get rid of the
> > > illusion of a stack.  The current stack isn't really a stack, it's just
> > > awkward.
> >
> > I personally do not have problems with it. As I said, I see this as
> > two different modes how the life patches are distributed. The stack
> > is needed for dependent patches. The cumulative patches with
> > "replace" flag are self-contained and independent. They might
> > replace anything.
>
> I agree here. Practically we use only cumulative replacement patches at
> SUSE. So with that in mind I don't care about the stacking much. But, it
> may make sense for someone else. Evgenii mentioned they used it for
> hotfixes. Therefore I'm reluctant to remove it completely.

Well, it was convenient in some cases to provide a hot fix for a given 
bug on top of our official cumulative patch. So far, such fixes were 
only used on a few of the customers' machines (where they were needed 
ASAP). It just made it easier to see where is the common set of fixes 
and where is the customer-specific addition.

I think, we can use cumulative patches in such cases too without much 
additional effort. For example, we can encode the distinction (base set 
of fixes + addition) in the module name or somewhere else.

So, I think, it is fine for us, if stacking support is removed. 
Especially if that makes the implementation of livepatch less complex 
and more reliable.

>   
> > Well, it would make sense to reduce the amount of possible
> > situations and use cases. The question is what is acceptable
> > to others and if it needs to be done as part of this patch set.
>
> Yes. Input from actual users would be tremendously useful here.
>
> Miroslav
> .