Hi Petr,
Thanks for your quick review!
> On Jul 18, 2022, at 5:50 AM, Petr Mladek <pmladek@xxxxxxxx> wrote:
>
> On Sun 2022-07-17 17:14:02, Song Liu wrote:
>> This is similar to modify_ftrace_direct_multi, but does not acquire
>> direct_mutex. This is useful when direct_mutex is already locked by the
>> user.
>>
>> --- a/kernel/trace/ftrace.c
>> +++ b/kernel/trace/ftrace.c
>> @@ -5691,22 +5691,8 @@ int unregister_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>> @@ -5717,12 +5703,8 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>> int i, size;
>> int err;
>>
>> - if (check_direct_multi(ops))
>> + if (WARN_ON_ONCE(!mutex_is_locked(&direct_mutex)))
>> return -EINVAL;
>
> IMHO, it is better to use:
>
> lockdep_assert_held_once(&direct_mutex);
>
> It will always catch the problem when called without the lock and
> lockdep is enabled.
Will fix.
>
>> - if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
>> - return -EINVAL;
>> -
>> - mutex_lock(&direct_mutex);
>>
>> /* Enable the tmp_ops to have the same functions as the direct ops */
>> ftrace_ops_init(&tmp_ops);
>> @@ -5730,7 +5712,7 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>>
>> err = register_ftrace_function(&tmp_ops);
>> if (err)
>> - goto out_direct;
>> + return err;
>>
>> /*
>> * Now the ftrace_ops_list_func() is called to do the direct callers.
>> @@ -5754,7 +5736,64 @@ int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>> /* Removing the tmp_ops will add the updated direct callers to the functions */
>> unregister_ftrace_function(&tmp_ops);
>>
>> - out_direct:
>> + return err;
>> +}
>> +
>> +/**
>> + * modify_ftrace_direct_multi_nolock - Modify an existing direct 'multi' call
>> + * to call something else
>> + * @ops: The address of the struct ftrace_ops object
>> + * @addr: The address of the new trampoline to call at @ops functions
>> + *
>> + * This is used to unregister currently registered direct caller and
>> + * register new one @addr on functions registered in @ops object.
>> + *
>> + * Note there's window between ftrace_shutdown and ftrace_startup calls
>> + * where there will be no callbacks called.
>> + *
>> + * Caller should already have direct_mutex locked, so we don't lock
>> + * direct_mutex here.
>> + *
>> + * Returns: zero on success. Non zero on error, which includes:
>> + * -EINVAL - The @ops object was not properly registered.
>> + */
>> +int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsigned long addr)
>> +{
>> + if (check_direct_multi(ops))
>> + return -EINVAL;
>> + if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
>> + return -EINVAL;
>> +
>> + return __modify_ftrace_direct_multi(ops, addr);
>> +}
>> +EXPORT_SYMBOL_GPL(modify_ftrace_direct_multi_nolock);
>> +
>> +/**
>> + * modify_ftrace_direct_multi - Modify an existing direct 'multi' call
>> + * to call something else
>> + * @ops: The address of the struct ftrace_ops object
>> + * @addr: The address of the new trampoline to call at @ops functions
>> + *
>> + * This is used to unregister currently registered direct caller and
>> + * register new one @addr on functions registered in @ops object.
>> + *
>> + * Note there's window between ftrace_shutdown and ftrace_startup calls
>> + * where there will be no callbacks called.
>> + *
>> + * Returns: zero on success. Non zero on error, which includes:
>> + * -EINVAL - The @ops object was not properly registered.
>> + */
>> +int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
>> +{
>> + int err;
>> +
>> + if (check_direct_multi(ops))
>> + return -EINVAL;
>> + if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
>> + return -EINVAL;
>> +
>> + mutex_lock(&direct_mutex);
>> + err = __modify_ftrace_direct_multi(ops, addr);
>> mutex_unlock(&direct_mutex);
>> return err;
>> }
>
> I would personally do:
>
> int __modify_ftrace_direct_multi(struct ftrace_ops *ops,
> unsigned long addr, bool lock)
> {
> int err;
>
> if (check_direct_multi(ops))
> return -EINVAL;
> if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> return -EINVAL;
>
> if (lock)
> mutex_lock(&direct_mutex);
>
> err = __modify_ftrace_direct_multi(ops, addr);
>
> if (lock)
> mutex_unlock(&direct_mutex);
The "if (lock) lock" pattern bothers me a little. But I agrees this is
a matter of taste. If other folks prefers this way, I will make the
change.
Thanks,
Song
>
> return err;
> }
>
> int modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
> {
> __modify_ftrace_direct_multi(ops, addr, true);
> }
>
> int modify_ftrace_direct_multi_nolock(struct ftrace_ops *ops, unsigned long addr)
> {
> __modify_ftrace_direct_multi(ops, addr, false);
> }
>
> To avoid duplication of the checks. But it is a matter of taste.
>
> Best Regards,
> Petr
