Message-ID: <20170907123452.GC3143@pathway.suse.cz>
Date:   Thu, 7 Sep 2017 14:34:52 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Jason Baron <jbaron@...mai.com>
Cc:     linux-kernel@...r.kernel.org, live-patching@...r.kernel.org,
        jpoimboe@...hat.com, jeyu@...nel.org, jikos@...nel.org,
        mbenes@...e.cz
Subject: Re: [PATCH v2 1/3] livepatch: Add dynamic klp_object and klp_func
 iterators

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

Powered by blists - more mailing lists