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 -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
