From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932245AbdIGMe4 (ORCPT ); Thu, 7 Sep 2017 08:34:56 -0400 Received: from mx2.suse.de ([195.135.220.15]:54552 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932097AbdIGMey (ORCPT ); Thu, 7 Sep 2017 08:34:54 -0400 Date: Thu, 7 Sep 2017 14:34:52 +0200 From: Petr Mladek To: Jason Baron 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 Message-ID: <20170907123452.GC3143@pathway.suse.cz> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > #include > #include > +#include > > #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