Hi, Miroslav
> On Sep 5, 2024, at 18:10, Miroslav Benes <mbenes@xxxxxxx> wrote:
>
> Hi,
>
> the subject is missing "livepatch: " prefix and I would prefer something
> like
>
> "livepatch: Move struct klp_ops to struct klp_func"
>
> On Wed, 28 Aug 2024, Wardenjohn wrote:
>
>> Before introduce feature "using". Klp transition will need an
>> easier way to get klp_ops from klp_func.
>
> I think that the patch might make sense on its own as a
> cleanup/optimization as Petr suggested. If fixed. See below. Then the
> changelog can be restructured and the above can be removed.
>
After the previous discuss, maybe this patch of "livepatch: Move struct klp_ops to struct klp_func" is useful, while the second patch need to be considered later, right?
> Btw if it comes to it, I would much rather have something like "active"
> instead of "using".
>
>> This patch make changes as follows:
>> 1. Move klp_ops into klp_func structure.
>> Rewrite the logic of klp_find_ops and
>> other logic to get klp_ops of a function.
>>
>> 2. Move definition of struct klp_ops into
>> include/linux/livepatch.h
>>
>> With this changes, we can get klp_ops of a klp_func easily.
>> klp_find_ops can also be simple and straightforward. And we
>> do not need to maintain one global list for now.
>>
>> Signed-off-by: Wardenjohn <zhangwarden@xxxxxxxxx>
>
> Missing "Suggested-by: Petr Mladek <pmladek@xxxxxxxx> perhaps?
>
Oops, I do miss this message here.
>> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> index 51a258c24ff5..d874aecc817b 100644
>> --- a/include/linux/livepatch.h
>> +++ b/include/linux/livepatch.h
>> @@ -22,6 +22,25 @@
>> #define KLP_TRANSITION_UNPATCHED 0
>> #define KLP_TRANSITION_PATCHED 1
>>
>> +/**
>> + * 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_func. 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;
>
> Not needed anymore.
What is not needed any more, do you means the comment?
>
>> + struct list_head func_stack;
>> + struct ftrace_ops fops;
>> +};
>> +
>>
>> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>> index 52426665eecc..e4572bf34316 100644
>> --- a/kernel/livepatch/core.c
>> +++ b/kernel/livepatch/core.c
>> @@ -760,6 +760,8 @@ static int klp_init_func(struct klp_object *obj, struct klp_func *func)
>> if (!func->old_name)
>> return -EINVAL;
>>
>> + func->ops = NULL;
>> +
>
> Any reason why it is not added a couple of lines later alongside the rest
> of the initialization?
Do you mean I should add couple of lines after 'return -EINVAL' ?
>
>> /*
>> * NOPs get the address later. The patched module must be loaded,
>> * see klp_init_object_loaded().
>> diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c
>> index 90408500e5a3..8ab9c35570f4 100644
>> --- a/kernel/livepatch/patch.c
>> +++ b/kernel/livepatch/patch.c
>> @@ -20,18 +20,25 @@
>> #include "patch.h"
>> #include "transition.h"
>>
>> -static LIST_HEAD(klp_ops);
>>
>> struct klp_ops *klp_find_ops(void *old_func)
>> {
>> - struct klp_ops *ops;
>> + struct klp_patch *patch;
>> + struct klp_object *obj;
>> struct klp_func *func;
>>
>> - list_for_each_entry(ops, &klp_ops, node) {
>> - func = list_first_entry(&ops->func_stack, struct klp_func,
>> - stack_node);
>> - if (func->old_func == old_func)
>> - return ops;
>> + klp_for_each_patch(patch) {
>> + klp_for_each_object(patch, obj) {
>> + klp_for_each_func(obj, func) {
>> + /*
>> + * Ignore entry where func->ops has not been
>> + * assigned yet. It is most likely the one
>> + * which is about to be created/added.
>> + */
>> + if (func->old_func == old_func && func->ops)
>> + return func->ops;
>> + }
>> + }
>> }
>
> The function is not used anywhere after this patch.
>
Maybe there still other places will call this klp_find_ops? Is it safe to delete it?
Regards,
Wardenjohn
