Date:   Tue, 10 Apr 2018 16:56:58 +0300
From:   Evgenii Shatokhin <eshatokhin@...tuozzo.com>
To:     Miroslav Benes <mbenes@...e.cz>, Petr Mladek <pmladek@...e.com>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>,
        Jiri Kosina <jikos@...nel.org>,
        Jason Baron <jbaron@...mai.com>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        Jessica Yu <jeyu@...nel.org>, live-patching@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 05/10] livepatch: Support separate list for replaced
 patches.

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

Powered by blists - more mailing lists