From: Joe Lawrence <joe.lawrence@redhat.com>
To: Petr Mladek <pmladek@suse.com>
Cc: Jiri Kosina <jikos@kernel.org>,
Josh Poimboeuf <jpoimboe@redhat.com>,
Miroslav Benes <mbenes@suse.cz>, Jason Baron <jbaron@akamai.com>,
Evgenii Shatokhin <eshatokhin@virtuozzo.com>,
live-patching@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4] livepatch: Remove the redundant enabled flag in struct klp_patch
Date: Mon, 21 Jan 2019 17:50:50 -0500 [thread overview]
Message-ID: <20190121225050.GD8766@redhat.com> (raw)
In-Reply-To: <20190116161720.796-5-pmladek@suse.com>
On Wed, Jan 16, 2019 at 05:17:20PM +0100, Petr Mladek wrote:
> Livepatches can not longer get enabled and disabled repeatedly.
nit: s/not longer/no longer/g
> The list klp_patches contains only enabled patches and eventually
> the patch in transition. As a result, the enabled flag in
> struct klp_patch provides redundant information and can get
> removed.
>
> The flag is replaced by helper function klp_patch_enabled().
> It simplifies the code. Also it helps to understand the semantic,
> especially for the patch in transition.
>
> Alternative solution was to remove klp_target_state. But this
> would be unfortunate. The three state variable helps to
> catch bugs and regressions. Also it makes it easier to get
> the state a lockless way in klp_update_patch_state().
smaller nit: s/get the state/get the state in/
>
> Suggested-by: Josh Poimboeuf <jpoimboe@redhat.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> include/linux/livepatch.h | 2 --
> kernel/livepatch/core.c | 23 +++++++++++++++--------
> kernel/livepatch/transition.c | 7 +++----
> kernel/livepatch/transition.h | 1 +
> 4 files changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 53551f470722..fa68192e6bb2 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -155,7 +155,6 @@ struct klp_object {
> * @kobj: kobject for sysfs resources
> * @obj_list: dynamic list of the object entries
> * @kobj_added: @kobj has been added and needs freeing
> - * @enabled: the patch is enabled (but operation may be incomplete)
> * @forced: was involved in a forced transition
> * @free_work: patch cleanup from workqueue-context
> * @finish: for waiting till it is safe to remove the patch module
> @@ -171,7 +170,6 @@ struct klp_patch {
> struct kobject kobj;
> struct list_head obj_list;
> bool kobj_added;
> - bool enabled;
> bool forced;
> struct work_struct free_work;
> struct completion finish;
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 684766d306ad..8e644837e668 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -59,6 +59,17 @@ static bool klp_is_module(struct klp_object *obj)
> return obj->name;
> }
>
> +static bool klp_patch_enabled(struct klp_patch *patch)
> +{
> + if (patch == klp_transition_patch) {
> + WARN_ON_ONCE(klp_target_state == KLP_UNDEFINED);
> +
> + return klp_target_state == KLP_PATCHED;
> + }
> +
> + return !list_empty(&patch->list);
> +}
> +
> /* sets obj->mod if object is not vmlinux and module is found */
> static void klp_find_object_module(struct klp_object *obj)
> {
> @@ -335,7 +346,7 @@ static ssize_t enabled_store(struct kobject *kobj, struct kobj_attribute *attr,
>
> mutex_lock(&klp_mutex);
>
> - if (patch->enabled == enabled) {
> + if (klp_patch_enabled(patch) == enabled) {
> /* already in requested state */
> ret = -EINVAL;
> goto out;
> @@ -369,7 +380,7 @@ static ssize_t enabled_show(struct kobject *kobj,
> struct klp_patch *patch;
>
> patch = container_of(kobj, struct klp_patch, kobj);
> - return snprintf(buf, PAGE_SIZE-1, "%d\n", patch->enabled);
> + return snprintf(buf, PAGE_SIZE-1, "%d\n", klp_patch_enabled(patch));
> }
>
> static ssize_t transition_show(struct kobject *kobj,
> @@ -862,7 +873,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
> INIT_LIST_HEAD(&patch->list);
> INIT_LIST_HEAD(&patch->obj_list);
> patch->kobj_added = false;
> - patch->enabled = false;
> patch->forced = false;
> INIT_WORK(&patch->free_work, klp_free_patch_work_fn);
> init_completion(&patch->finish);
> @@ -919,7 +929,7 @@ static int __klp_disable_patch(struct klp_patch *patch)
> {
> struct klp_object *obj;
>
> - if (WARN_ON(!patch->enabled))
> + if (WARN_ON(!klp_patch_enabled(patch)))
> return -EINVAL;
>
> if (klp_transition_patch)
> @@ -941,7 +951,6 @@ static int __klp_disable_patch(struct klp_patch *patch)
> smp_wmb();
>
> klp_start_transition();
> - patch->enabled = false;
> klp_try_complete_transition();
>
> return 0;
> @@ -955,7 +964,7 @@ static int __klp_enable_patch(struct klp_patch *patch)
> if (klp_transition_patch)
> return -EBUSY;
>
> - if (WARN_ON(patch->enabled))
> + if (list_empty(&patch->list))
> return -EINVAL;
>
> if (!patch->kobj_added)
> @@ -994,7 +1003,6 @@ static int __klp_enable_patch(struct klp_patch *patch)
> }
>
> klp_start_transition();
> - patch->enabled = true;
> klp_try_complete_transition();
>
> return 0;
> @@ -1093,7 +1101,6 @@ void klp_discard_replaced_patches(struct klp_patch *new_patch)
> if (old_patch == new_patch)
> return;
>
> - old_patch->enabled = false;
> klp_unpatch_objects(old_patch);
> klp_free_patch_start(old_patch);
> schedule_work(&old_patch->free_work);
> diff --git a/kernel/livepatch/transition.c b/kernel/livepatch/transition.c
> index a3a6f32c6fd0..a40b58660640 100644
> --- a/kernel/livepatch/transition.c
> +++ b/kernel/livepatch/transition.c
> @@ -31,7 +31,7 @@
>
> struct klp_patch *klp_transition_patch;
>
> -static int klp_target_state = KLP_UNDEFINED;
> +int klp_target_state = KLP_UNDEFINED;
>
> /*
> * This work can be performed periodically to finish patching or unpatching any
> @@ -354,6 +354,7 @@ static bool klp_try_switch_task(struct task_struct *task)
> void klp_try_complete_transition(void)
> {
> unsigned int cpu;
> + int target_state = klp_target_state;
> struct task_struct *g, *task;
> struct klp_patch *patch;
> bool complete = true;
> @@ -412,7 +413,7 @@ void klp_try_complete_transition(void)
> * klp_complete_transition() but it is called also
> * from klp_cancel_transition().
> */
> - if (!patch->enabled) {
> + if (target_state == KLP_UNPATCHED) {
> klp_free_patch_start(patch);
> schedule_work(&patch->free_work);
> }
> @@ -545,8 +546,6 @@ void klp_reverse_transition(void)
> klp_target_state == KLP_PATCHED ? "patching to unpatching" :
> "unpatching to patching");
>
> - klp_transition_patch->enabled = !klp_transition_patch->enabled;
> -
> klp_target_state = !klp_target_state;
>
> /*
> diff --git a/kernel/livepatch/transition.h b/kernel/livepatch/transition.h
> index f9d0bc016067..b9f3e96d8c13 100644
> --- a/kernel/livepatch/transition.h
> +++ b/kernel/livepatch/transition.h
> @@ -5,6 +5,7 @@
> #include <linux/livepatch.h>
>
> extern struct klp_patch *klp_transition_patch;
> +extern int klp_target_state;
>
> void klp_init_transition(struct klp_patch *patch, int state);
> void klp_cancel_transition(void);
> --
> 2.13.7
>
With commit msg nits,
Acked-by: Joe Lawrence <joe.lawrence@redhat.com>
-- Joe
next prev parent reply other threads:[~2019-01-21 22:50 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-16 16:17 [PATCH 0/4] livepatch: Followup changes for the atomic replace patchset Petr Mladek
2019-01-16 16:17 ` [PATCH 1/4] livepatch: Introduce klp_for_each_patch macro Petr Mladek
2019-01-21 12:10 ` Miroslav Benes
2019-01-21 22:34 ` Joe Lawrence
2019-01-16 16:17 ` [PATCH 2/4] livepatch: Handle failing allocation of shadow variables in the selftest Petr Mladek
2019-01-21 12:14 ` Miroslav Benes
2019-01-30 8:46 ` Petr Mladek
2019-01-31 8:40 ` Miroslav Benes
2019-01-21 22:40 ` Joe Lawrence
2019-01-30 8:56 ` Petr Mladek
2019-01-16 16:17 ` [PATCH 3/4] livepatch: Module coming and going callbacks can proceed all listed patches Petr Mladek
2019-01-21 14:45 ` Miroslav Benes
2019-01-21 22:47 ` Joe Lawrence
2019-01-16 16:17 ` [PATCH 4/4] livepatch: Remove the redundant enabled flag in struct klp_patch Petr Mladek
2019-01-21 22:50 ` Joe Lawrence [this message]
2019-01-22 10:06 ` Miroslav Benes
2019-01-23 18:27 ` Joe Lawrence
2019-01-29 20:00 ` Josh Poimboeuf
2019-01-30 9:44 ` Petr Mladek
2019-02-01 16:03 ` [PATCH 0/4] livepatch: Followup changes for the atomic replace patchset Joe Lawrence
2019-02-04 9:40 ` Petr Mladek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190121225050.GD8766@redhat.com \
--to=joe.lawrence@redhat.com \
--cc=eshatokhin@virtuozzo.com \
--cc=jbaron@akamai.com \
--cc=jikos@kernel.org \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mbenes@suse.cz \
--cc=pmladek@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome