Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753023AbbBMOjq (ORCPT ); Fri, 13 Feb 2015 09:39:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52933 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751839AbbBMOjp (ORCPT ); Fri, 13 Feb 2015 09:39:45 -0500 Date: Fri, 13 Feb 2015 08:39:27 -0600 From: Josh Poimboeuf To: Miroslav Benes Cc: Seth Jennings , Jiri Kosina , Vojtech Pavlik , Masami Hiramatsu , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 2/9] livepatch: separate enabled and patched states Message-ID: <20150213143927.GC27180@treble.redhat.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4488 Lines: 135 On Fri, Feb 13, 2015 at 01:57:38PM +0100, Miroslav Benes wrote: > On Mon, 9 Feb 2015, Josh Poimboeuf wrote: > > > Once we have a consistency model, patches and their objects will be > > enabled and disabled at different times. For example, when a patch is > > disabled, its loaded objects' funcs can remain registered with ftrace > > indefinitely until the unpatching operation is complete and they're no > > longer in use. > > > > It's less confusing if we give them different names: patches can be > > enabled or disabled; objects (and their funcs) can be patched or > > unpatched: > > > > - Enabled means that a patch is logically enabled (but not necessarily > > fully applied). > > > > - Patched means that an object's funcs are registered with ftrace and > > added to the klp_ops func stack. > > > > Also, since these states are binary, represent them with boolean-type > > variables instead of enums. > > They are binary now but will it hold also in the future? I cannot come up > with any other possible state of the function right now, but that doesn't > mean there isn't any. It would be sad to return it back to enums one day > :) I really can't think of any reason why they would become non-binary. IMO it's more likely we could add more boolean variables, but if that got out of hand we could just switch to using bit flags. Either way I don't see a problem with changing them later if we need to. > Also would it be useful to expose patched variable for functions and > objects in sysfs? Not that I know of. Do you have a use case in mind? I view "patched" as an internal variable, corresponding to whether the object or its functions are registered with ftrace/klp_ops. It doesn't mean "patched" in a way that would really make sense to the user, because of the gradual nature of the patching process. > > Two small things below... Agreed to both, thanks. > > > Signed-off-by: Josh Poimboeuf > > --- > > include/linux/livepatch.h | 15 ++++----- > > kernel/livepatch/core.c | 79 +++++++++++++++++++++++------------------------ > > 2 files changed, 45 insertions(+), 49 deletions(-) > > > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h > > index 95023fd..22a67d1 100644 > > --- a/include/linux/livepatch.h > > +++ b/include/linux/livepatch.h > > @@ -28,11 +28,6 @@ > > > > #include > > > > -enum klp_state { > > - KLP_DISABLED, > > - KLP_ENABLED > > -}; > > - > > /** > > * struct klp_func - function structure for live patching > > * @old_name: name of the function to be patched > > @@ -42,6 +37,7 @@ enum klp_state { > > * @kobj: kobject for sysfs resources > > * @state: tracks function-level patch application state > > * @stack_node: list node for klp_ops func_stack list > > + * @patched: the func has been added to the klp_ops list > > */ > > struct klp_func { > > /* external */ > > @@ -59,8 +55,8 @@ struct klp_func { > > > > /* internal */ > > struct kobject kobj; > > - enum klp_state state; > > struct list_head stack_node; > > + int patched; > > }; > > @state remains in the comment above > > > /** > > @@ -90,7 +86,7 @@ struct klp_reloc { > > * @kobj: kobject for sysfs resources > > * @mod: kernel module associated with the patched object > > * (NULL for vmlinux) > > - * @state: tracks object-level patch application state > > + * @patched: the object's funcs have been add to the klp_ops list > > */ > > struct klp_object { > > /* external */ > > @@ -101,7 +97,7 @@ struct klp_object { > > /* internal */ > > struct kobject *kobj; > > struct module *mod; > > - enum klp_state state; > > + int patched; > > }; > > > > /** > > @@ -111,6 +107,7 @@ struct klp_object { > > * @list: list node for global list of registered patches > > * @kobj: kobject for sysfs resources > > * @state: tracks patch-level application state > > + * @enabled: the patch is enabled (but operation may be incomplete) > > */ > > struct klp_patch { > > /* external */ > > @@ -120,7 +117,7 @@ struct klp_patch { > > /* internal */ > > struct list_head list; > > struct kobject kobj; > > - enum klp_state state; > > + int enabled; > > }; > > Dtto > > Miroslav -- Josh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/