Date: Thu, 28 Apr 2016 13:53:53 -0500
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Jiri Kosina <jikos@kernel.org>, Jessica Yu <jeyu@redhat.com>,
        Miroslav Benes <mbenes@suse.cz>, linux-kernel@vger.kernel.org,
        live-patching@vger.kernel.org, Vojtech Pavlik <vojtech@suse.com>
Subject: Re: [RFC PATCH v1.9 00/14] livepatch: hybrid consistency model
Message-ID: <20160428185353.dljx4aykcr4hdznm@treble>
References: <cover.1458933243.git.jpoimboe@redhat.com>
 <20160401133944.GQ5522@pathway.suse.cz>
 <20160405134430.q4pqm2oalivjka4s@treble.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20160405134430.q4pqm2oalivjka4s@treble.redhat.com>
User-Agent: Mutt/1.6.0.1 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2129
Lines: 51

On Tue, Apr 05, 2016 at 08:44:30AM -0500, Josh Poimboeuf wrote:
> On Fri, Apr 01, 2016 at 03:39:44PM +0200, Petr Mladek wrote:
> > > There's also a func->immediate flag which allows users to specify that
> > > certain functions in the patch can be applied without per-task
> > > consistency.  This might be useful if you want to patch a common
> > > function like schedule(), and the function change doesn't need
> > > consistency but the rest of the patch does.
> > 
> > We probably should not set func->transition flag when func->immediate
> > is set or when the related func->object is set. It currently happens
> > only when patch->immediate is set.
> 
> Agreed, I'll skip setting func->transition if func->immediate is set.

So I'm getting ready to post v2, and I think I changed my mind on this
one, for a couple of reasons:

1) It's conceptually simpler if func->transition gets set for all
   functions, so there are less edge cases to consider.

2) For unpatching, if func->transition is set, func->immediate results
   in the ftrace handler picking the old function immediately, which is
   more expected and in line with the name 'immediate'.  If 'transition'
   is not set then it doesn't switch to the old function until the
   klp_func gets removed from the func stack.

> > If we support only one transition at a time, a simple boolean
> > or even bit should be enough. The most descriptive name would
> > be klp_transition_patch_applied but it is quite long.
> 
> Yeah, I'll change it to a bool.

I could probably go either way on this one, but I'm leaving it as a
non-bool for now, because I think:

  klp_patch_target == KLP_PATCHED
  klp_patch_target == KLP_UNPATCHED
 
reads better and does a better job describing its purpose than:

  klp_target_patched
  !klp_target_patched

And also, the corresponding task_struct.patch_state variable is now a
tri-state variable, with KLP_UNDEFINED (-1) being the third option to
indicate there's currently no patch operation in progress.  And I think
it's easier to understand and implement if we use the same
KLP_PATCHED/UNPATCHED defines for both variables.

-- 
Josh