Re: [RFC PATCH v1.9 00/14] livepatch: hybrid consistency model

[Petr Mladek]

On Thu 2016-04-28 13:53:53, Josh Poimboeuf wrote:
> 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.

Fair enough.


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

I am fine with the tristate. The boolean was mentioned
as a minimalistic solution in the generic discussion
about the name "universe".

Just an idea. What about using klp_transition_target and
task_struct.klp_transtion_state? It will be more clear that
these two variables are used only during the patch transition.

Best Regards,
Petr