From: Petr Mladek <pmladek@suse.com>
To: Jason Baron <jbaron@akamai.com>
Cc: linux-kernel@vger.kernel.org, live-patching@vger.kernel.org,
jpoimboe@redhat.com, jeyu@kernel.org, jikos@kernel.org,
mbenes@suse.cz
Subject: Re: [PATCH v2 1/3] livepatch: Add dynamic klp_object and klp_func iterators
Date: Thu, 7 Sep 2017 14:34:52 +0200 [thread overview]
Message-ID: <20170907123452.GC3143@pathway.suse.cz> (raw)
In-Reply-To: <a5c582babc6088a7da1121a3301ea29a5a58641d.1504128316.git.jbaron@akamai.com>
On Wed 2017-08-30 17:38:43, Jason Baron wrote:
> In preparation to introducing atomic replace, introduce iterators for klp_func
> and klp_object, such that objects and functions can be dynmically allocated
> (needed for atomic replace). This patch is intended to effectively be a no-op
./scripts/checkpatch.pl reports that these lines are too long.
It prefers a maximum 75 chars per line in the commit message ;-)
> until atomic replace is introduced.
>
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -24,6 +24,7 @@
> #include <linux/module.h>
> #include <linux/ftrace.h>
> #include <linux/completion.h>
> +#include <linux/list.h>
>
> #if IS_ENABLED(CONFIG_LIVEPATCH)
>
> @@ -44,6 +45,7 @@
> * @old_addr: the address of the function being patched
> * @kobj: kobject for sysfs resources
> * @stack_node: list node for klp_ops func_stack list
> + * @func_entry: used to link struct klp_func to struct klp_object
Please, make it clear that only dynamically allocated structures
are linked. Same for the other entries.
It might look superfluous when you read this patch. But it
will help a lot when you read the code one year from now.
> * @old_size: size of the old function
> * @new_size: size of the new function
> * @patched: the func has been added to the klp_ops list
[...]
> @@ -126,17 +134,95 @@ struct klp_patch {
> /* internal */
> struct list_head list;
> struct kobject kobj;
> + struct list_head obj_list;
> bool enabled;
> struct completion finish;
> };
>
> +static inline struct klp_object *obj_iter_next(struct klp_patch *patch,
> + struct klp_object *obj)
The function is far from trivial. I wonder if it is still a good
candidate for inlining.
Also it should get prefixed by klp_ because it is in a header
that can be included anywhere.
Next I still think that it will be easier to understand when
we make it more clear that only the dymanically allocated
objects are in the list. I mean renaming:
obj_entry -> dyn_obj_entry
obj_list -> dyn_obj_list
> +{
> + struct klp_object *next_obj = NULL;
> +
The code quite tricky. IMHO, it would deserve a comment.
/*
* Statically defined objects are in NULL-ended array.
* Only dynamic ones are in the obj_list.
*/
> + if (list_empty(&obj->obj_entry)) {
> + next_obj = obj + 1;
> + if (next_obj->funcs || next_obj->name)
> + goto out;
> + else
> + next_obj = NULL;
Please, add an empty line here to make it better readable.
> + if (!list_empty(&patch->obj_list))
> + next_obj = container_of(patch->obj_list.next,
> + struct klp_object,
> + obj_entry);
> + goto out;
> + }
Also here an empty line.
> + if (obj->obj_entry.next != &patch->obj_list)
> + next_obj = container_of(obj->obj_entry.next,
> + struct klp_object,
> + obj_entry);
> +out:
> + return next_obj;
> +}
> +static inline struct klp_object *obj_iter_init(struct klp_patch *patch)
> +{
> + if (patch->objs->funcs || patch->objs->name)
I would do something like
#define klp_is_null_obj(obj) (!obj->funcs && !obj->name)
Then it can be used here an in klp_obj_iter_next().
This will be even more useful in the func iterator
where the check is even more complicated.
> + return patch->objs;
> + else
> + return NULL;
> +}
> +
> #define klp_for_each_object(patch, obj) \
> - for (obj = patch->objs; obj->funcs || obj->name; obj++)
> + for (obj = obj_iter_init(patch); obj; obj = obj_iter_next(patch, obj))
> +
> +static inline struct klp_func *func_iter_next(struct klp_object *obj,
> + struct klp_func *func)
> +{
> + struct klp_func *next_func = NULL;
> +
> + if (list_empty(&func->func_entry)) {
> + next_func = func + 1;
> + if (next_func->old_name || next_func->new_func ||
> + next_func->old_sympos)
> + goto out;
> + else
> + next_func = NULL;
> + if (!list_empty(&obj->func_list))
> + next_func = container_of(obj->func_list.next,
> + struct klp_func,
> + func_entry);
I have just realized that a practice is to use list_entry() instead
of container_of() for list entries. It probably makes the code better
readable for a trained eye.
> + goto out;
> + }
> + if (func->func_entry.next != &obj->func_list)
> + next_func = container_of(func->func_entry.next,
> + struct klp_func,
> + func_entry);
> +out:
> + return next_func;
> +}
[...]
> #define klp_for_each_func(obj, func) \
> - for (func = obj->funcs; \
> - func->old_name || func->new_func || func->old_sympos; \
> - func++)
> + for (func = func_iter_init(obj); func; \
> + func = func_iter_next(obj, func))
Otherwise, I have basically the same comments about iter_func
like for iter_obj.
> int klp_register_patch(struct klp_patch *);
> int klp_unregister_patch(struct klp_patch *);
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index b9628e4..6004be3 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -729,7 +730,10 @@ static int klp_init_patch(struct klp_patch *patch)
> return ret;
> }
>
> + INIT_LIST_HEAD(&patch->obj_list);
Please, do this together with the other trivial initizalizations.
I mean to do it in the same place like in the other init functions:
patch->enabled = false;
patch->replaced = false;
+ INIT_LIST_HEAD(&patch->obj_list);
> klp_for_each_object(patch, obj) {
> + INIT_LIST_HEAD(&obj->obj_entry);
> + INIT_LIST_HEAD(&obj->func_list);
These two should be done in klp_init_object(). You move it there
in the second patch anyway.
> ret = klp_init_object(patch, obj);
> if (ret)
> goto free;
Best Regards,
Petr
next prev parent reply other threads:[~2017-09-07 12:34 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-30 21:38 [PATCH v2 0/3] livepatch: introduce atomic replace Jason Baron
2017-08-30 21:38 ` [PATCH v2 1/3] livepatch: Add dynamic klp_object and klp_func iterators Jason Baron
2017-09-07 12:34 ` Petr Mladek [this message]
2017-09-07 16:53 ` Joe Perches
2017-08-30 21:38 ` [PATCH v2 2/3] livepatch: add atomic replace Jason Baron
2017-09-11 13:53 ` Petr Mladek
2017-09-12 3:46 ` Jason Baron
2017-09-12 8:35 ` Petr Mladek
2017-09-13 6:47 ` Jason Baron
2017-09-14 13:32 ` Petr Mladek
2017-09-14 15:31 ` Jason Baron
2017-09-15 15:46 ` Petr Mladek
2017-09-16 0:10 ` Greg KH
2017-08-30 21:38 ` [PATCH v2 3/3] livepatch: Add a sysctl livepatch_mode for " Jason Baron
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=20170907123452.GC3143@pathway.suse.cz \
--to=pmladek@suse.com \
--cc=jbaron@akamai.com \
--cc=jeyu@kernel.org \
--cc=jikos@kernel.org \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mbenes@suse.cz \
/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;
as well as URLs for NNTP newsgroup(s).