Messages in this thread | | | Subject | Re: [PATCH v10 05/10] livepatch: Support separate list for replaced patches. | From | Evgenii Shatokhin <> | Date | Tue, 10 Apr 2018 16:56:58 +0300 |
| |
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 > . >
| |