Re: [RFC PATCH 3/9] livepatch: move patching functions into patch.c

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Fri, Feb 13, 2015 at 03:28:28PM +0100, Miroslav Benes wrote:
> 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. 

I'm not sure about moving the sysfs code to its own file, mainly because
of enabled_store():

1. It needs the klp_mutex.  It's really nice and clean to keep the
   klp_mutex a static variable in core.c (which I plan on doing in v2 of
   the patch set).

2. It's one of the main entry points into the klp code, along with
   register/unregister and enable/disable.  It makes a lot of sense to
   keep all of those entry points in the same file IMO.

> 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.

I would vote to keep enable/disable in core.c for the same reasons as
stated above for enabled_store().  It's possible that
__klp_enable_patch() and __klp_disable_patch() could be moved elsewhere.
Personally I like them where they are, since they call into both
"transition" functions and "patch" functions.

So, big surprise, I agree with my own code splitting decisions ;-)

> 
> 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 <linux/livepatch.h>
> > +
> > +/**
> > + * 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.

Good catch, thanks.

-- 
Josh