lkml.org 
[lkml]   [2018]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v10 05/10] livepatch: Support separate list for replaced patches.
    On Mon, Mar 26, 2018 at 12:11:07PM +0200, Petr Mladek wrote:
    > On Fri 2018-03-23 17:44:10, Josh Poimboeuf wrote:
    > > On Fri, Mar 23, 2018 at 10:45:07AM +0100, Petr Mladek wrote:
    > > > On Tue 2018-03-20 15:15:02, Josh Poimboeuf wrote:
    > > > > On Tue, Mar 20, 2018 at 01:25:38PM +0100, Petr Mladek wrote:
    > > > > > On Mon 2018-03-19 16:43:24, Josh Poimboeuf wrote:
    > > > > > > On Mon, Mar 19, 2018 at 04:02:07PM +0100, Petr Mladek wrote:
    > > > > > > > > Along those lines, I'd also propose that we constrain our existing patch
    > > > > > > > > stacking even further. Right now we allow a new patch to be registered
    > > > > > > > > on top of a disabled patch, though we don't allow the new patch to be
    > > > > > > > > enabled until the previous patch gets enabled. I'd propose we no longer
    > > > > > > > > allow that condition. We should instead enforce that all existing
    > > > > > > > > patches are *enabled* before allowing a new patch to be registered on
    > > > > > > > > top. That way the patch stacking is even more sane, and there are less
    > > > > > > > > "unusual" conditions to worry about. We have enough of those already.
    > > > > > > > > Each additional bit of flexibility has a maintenance cost, and this one
    > > > > > > > > isn't worth it IMO.
    > > > > > > >
    > > > > > > > Again, this might make some things easier but it might also bring
    > > > > > > > problems.
    > > > > > > >
    > > > > > > > For example, we would need to solve the situation when the last
    > > > > > > > patch is disabled and cannot be removed because the transition
    > > > > > > > was forced. This might be more common after removing the immediate
    > > > > > > > feature.
    > > > > > >
    > > > > > > I would stop worrying about forced patches so much :-)
    > > > > >
    > > > > > I have already seen blocked transition several times. It is true that
    > > > > > it was with kGraft. But we just do not have enough real life experience
    > > > > > with the upstream livepatch code.
    > > > >
    > > > > But we're talking about patching on top of a *disabled* patch. Forced
    > > > > or not, why would the patch be disabled in the first place?
    > > >
    > > > For example, it might be disabled because the transition stalled for
    > > > too long and the user reverted it. Or just because it is possible
    > > > to disable it.
    > >
    > > If they haven't previously forced any patches, and they reverted the
    > > topmost patch because it stalled, they can easily unload the patch.
    > >
    > > If they *have* previously forced a patch, they can force enable the
    > > topmost patch as well, or if that's not safe they can reboot (that's
    > > what you get for forcing a patch...)
    >
    > IMHO, the reboot is the very last option for people that are using
    > livepatching.

    ... but it may be a natural consequence of forcing a patch.

    > > > > We were just recently discussing the possibility of not allowing the
    > > > > disabling of patches at all. If we're not going that far, let's at
    > > > > least further restrict it, for the sanity of our code, so we don't have
    > > > > to worry about a nasty mismatched stack of enabled/disabled/enabled/etc,
    > > > > at least for the cases where 'replace' patches aren't used.
    > > >
    > > > I am not completely sure where all these fears come from. From my
    > > > point of view, this change is pretty safe and trivial thanks to NOPs
    > > > and overall design. It would be a shame to do not have it. But I
    > > > might be blind after spending so much time with the feature.
    > >
    > > 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.
    >
    > This would be true if we keep the replaced patches on the stack. But
    > if we remove the replaced patches then there never will be disabled
    > patches in the middle.
    >
    > OK, there might be disabled patches in the middle during the
    > transition. But this is the situation where we basically could
    > not manipulate the stack.

    No, please read it again. I wasn't talking about replaced 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.

    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?

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

    --
    Josh

    \
     
     \ /
      Last update: 2018-04-06 21:51    [W:5.208 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site