Message-ID: <20180406195049.dtfebzfdkbvv6yex@treble>
Date:   Fri, 6 Apr 2018 14:50:49 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     Jiri Kosina <jikos@...nel.org>, Miroslav Benes <mbenes@...e.cz>,
        Jason Baron <jbaron@...mai.com>,
        Joe Lawrence <joe.lawrence@...hat.com>,
        Jessica Yu <jeyu@...nel.org>,
        Evgenii Shatokhin <eshatokhin@...tuozzo.com>,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [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

Powered by blists - more mailing lists