Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752721AbbBMO2e (ORCPT ); Fri, 13 Feb 2015 09:28:34 -0500 Received: from cantor2.suse.de ([195.135.220.15]:56757 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752023AbbBMO2d (ORCPT ); Fri, 13 Feb 2015 09:28:33 -0500 Date: Fri, 13 Feb 2015 15:28:28 +0100 (CET) From: Miroslav Benes To: Josh Poimboeuf cc: Seth Jennings , Jiri Kosina , Vojtech Pavlik , Masami Hiramatsu , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 3/9] livepatch: move patching functions into patch.c In-Reply-To: <35666b8f283baf00be64ece874913e7ec8b13256.1423499826.git.jpoimboe@redhat.com> Message-ID: References: <35666b8f283baf00be64ece874913e7ec8b13256.1423499826.git.jpoimboe@redhat.com> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2592 Lines: 73 On Mon, 9 Feb 2015, Josh Poimboeuf wrote: > Move functions related to the actual patching of functions and objects > into a new patch.c file. I am definitely for splitting the code to several different files. Otherwise it would be soon unmanageable. However I don't know if this patch is the best possible. Maybe it is just nitpicking so let's not spend too much time on this :) Without this patch there are several different groups of functions in core.c: 1. infrastructure such as global variables, klp_init and some helper functions 2. (un)registration and initialization of the patch 3. enable/disable with patching/unpatching, ftrace handler 4. sysfs code 5. module notifier 6. relocations I would move sysfs code away to separate file. If we decide to move patching code I think it would make sense to move enable/disable functions along with it. Or perhaps __klp_enable_patch and __klp_disable_patch only. It is possible though that the result would be much worse. Or we can move some other group of functions... [...] > diff --git a/kernel/livepatch/patch.h b/kernel/livepatch/patch.h > new file mode 100644 > index 0000000..bb34bd3 > --- /dev/null > +++ b/kernel/livepatch/patch.h > @@ -0,0 +1,25 @@ > +#include > + > +/** > + * struct klp_ops - structure for tracking registered ftrace ops structs > + * > + * A single ftrace_ops is shared between all enabled replacement functions > + * (klp_func structs) which have the same old_addr. This allows the switch > + * between function versions to happen instantaneously by updating the klp_ops > + * struct's func_stack list. The winner is the klp_func at the top of the > + * func_stack (front of the list). > + * > + * @node: node for the global klp_ops list > + * @func_stack: list head for the stack of klp_func's (active func is on top) > + * @fops: registered ftrace ops struct > + */ > +struct klp_ops { > + struct list_head node; > + struct list_head func_stack; > + struct ftrace_ops fops; > +}; > + > +struct klp_ops *klp_find_ops(unsigned long old_addr); > + > +extern int klp_patch_object(struct klp_object *obj); > +extern void klp_unpatch_object(struct klp_object *obj); Is there a reason why klp_find_ops is not extern and the other two functions are? I think it is redundant and it is better to be consistent. Regards, Miroslav -- 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/