lkml.org 
[lkml]   [2018]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v10 05/10] livepatch: Support separate list for replaced patches.
From
Date
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
> .
>

\
 
 \ /
  Last update: 2018-04-10 15:57    [W:0.391 / U:0.404 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site