On 27.08.2024 22:30, Michal Wajdeczko wrote:
>
>
> On 27.08.2024 16:46, Lucas De Marchi wrote:
>> On Tue, Aug 27, 2024 at 12:20:13AM GMT, Michal Wajdeczko wrote:
>>> Currently, the 'static stub' API only allows function redirection
>>> for calls made from the kthread of the current test, which prevents
>>> the use of this functionality when the tested code is also used by
>>> other threads, outside of the KUnit test, like from the workqueue.
>>>
>>> Add another set of macros to allow redirection to the replacement
>>> functions, which, unlike the KUNIT_STATIC_STUB_REDIRECT, will
>>> affect all calls done during the test execution.
>>>
>>> These new stubs, named 'global', must be declared using dedicated
>>> KUNIT_DECLARE_GLOBAL_STUB() macro and then can be placed either as
>>> global static variables or as part of the other structures.
>>>
>>> To properly maintain stubs lifecycle, they can be activated only
>>> from the main KUnit context. Some precaution is taken to avoid
>>> changing or deactivating replacement functions if one is still
>>> used by other thread.
>>>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
>>> ---
>>> Cc: Rae Moar <rmoar@xxxxxxxxxx>
>>> Cc: David Gow <davidgow@xxxxxxxxxx>
>>> Cc: Daniel Latypov <dlatypov@xxxxxxxxxx>
>>> Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
>>> ---
>>> v2: s/FIXED_STUB/GLOBAL_STUB (David, Lucas)
>>> make it little more thread safe (Rae, David)
>>> wait until stub call finishes before test end (David)
>>> wait until stub call finishes before changing stub (David)
>>> allow stub deactivation (Rae)
>>> prefer kunit log (David)
>>> ---
>>> include/kunit/static_stub.h | 158 ++++++++++++++++++++++++++++++++++++
>>> lib/kunit/static_stub.c | 49 +++++++++++
>>> 2 files changed, 207 insertions(+)
>>>
>>> diff --git a/include/kunit/static_stub.h b/include/kunit/static_stub.h
>>> index bf940322dfc0..42a70dcefb56 100644
>>> --- a/include/kunit/static_stub.h
>>> +++ b/include/kunit/static_stub.h
>>> @@ -12,12 +12,15 @@
>>>
>>> /* If CONFIG_KUNIT is not enabled, these stubs quietly disappear. */
>>> #define KUNIT_STATIC_STUB_REDIRECT(real_fn_name, args...) do {} while (0)
>>> +#define KUNIT_GLOBAL_STUB_REDIRECT(stub_name, args...) do {} while (0)
>>> +#define KUNIT_DECLARE_GLOBAL_STUB(stub_name, stub_type)
>>>
>>> #else
>>>
>>> #include <kunit/test.h>
>>> #include <kunit/test-bug.h>
>>>
>>> +#include <linux/cleanup.h> /* for CLASS */
>>> #include <linux/compiler.h> /* for {un,}likely() */
>>> #include <linux/sched.h> /* for task_struct */
>>>
>>> @@ -109,5 +112,160 @@ void __kunit_activate_static_stub(struct kunit
>>> *test,
>>> */
>>> void kunit_deactivate_static_stub(struct kunit *test, void
>>> *real_fn_addr);
>>>
>>> +/**
>>> + * struct kunit_global_stub - Represents a context of global function
>>> stub.
>>> + * @replacement: The address of replacement function.
>>> + * @owner: The KUnit test that owns the stub, valid only when @busy > 0.
>>> + * @busy: The stub busyness counter incremented on entry to the
>>> replacement
>>> + * function, decremented on exit, used to signal if the stub
>>> is idle.
>>> + * @idle: The completion state to indicate when the stub is idle again.
>>> + *
>>> + * This structure is for KUnit internal use only.
>>> + * See KUNIT_DECLARE_GLOBAL_STUB().
>>> + */
>>> +struct kunit_global_stub {
>>> + void *replacement;
>>> + struct kunit *owner;
>>> + atomic_t busy;
>>> + struct completion idle;
>>> +};
>>> +
>>> +/**
>>> + * KUNIT_DECLARE_GLOBAL_STUB() - Declare a global function stub.
>>> + * @stub_name: The name of the stub, must be a valid identifier
>>> + * @stub_type: The type of the function that this stub will replace
>>> + *
>>> + * This macro will declare new identifier of an anonymous type that will
>>> + * represent global stub function that could be used by KUnit. It can
>>> be stored
>>> + * outside of the KUnit code. If the CONFIG_KUNIT is not enabled this
>>> will
>>> + * be evaluated to an empty statement.
>>> + *
>>> + * The anonymous type introduced by this macro is mostly a wrapper to
>>> generic
>>> + * struct kunit_global_stub but with additional dummy member, that is
>>> never
>>> + * used directly, but is needed to maintain the type of the stub
>>> function.
>>> + */
>>> +#define KUNIT_DECLARE_GLOBAL_STUB(stub_name, stub_type) \
>>> +union { \
>>> + struct kunit_global_stub base; \
>>> + typeof(stub_type) dummy; \
>>> +} stub_name
>>> +
>>> +/* Internal struct to define guard class */
>>> +struct kunit_global_stub_guard {
>>> + struct kunit_global_stub *stub;
>>> + void *active_replacement;
>>> +};
>>> +
>>> +/* Internal class used to guard stub calls */
>>> +DEFINE_CLASS(kunit_global_stub_guard,
>>> + struct kunit_global_stub_guard,
>>> + ({
>>> + struct kunit_global_stub *stub = _T.stub;
>>> + bool active = !!_T.active_replacement;
>>
>> I'd call this `bool active_replacement` as it's not the same thing as
>> the active below.
>
> IMO 'active_replacement' would be even more confusing as by that name we
> identify the address and here it's a flag
>
> OTOH the 'active' in both places means more/less the same (in init below
> it mean stub was 'activated' and in exit here that we used 'activated'
> replacement function)
>
>>
>>> +
>>> + if (active && !atomic_dec_return(&stub->busy))
>>> + complete_all(&stub->idle);
>>> + }),
>>> + ({
>>> + class_kunit_global_stub_guard_t guard;
>>> + bool active = !!atomic_inc_not_zero(&stub->busy);
>>> +
>>> + guard.stub = stub;
>>> + guard.active_replacement = active ?
>>> READ_ONCE(stub->replacement) : NULL;
>>> +
>>> + guard;
>>> + }),
>>> + struct kunit_global_stub *stub)
>>> +
>>> +/**
>>> + * KUNIT_GLOBAL_STUB_REDIRECT() - Call a fixed function stub if
>>> activated.
>>> + * @stub: The function stub declared using KUNIT_DECLARE_GLOBAL_STUB()
>>> + * @args: All of the arguments passed to this stub
>>> + *
>>> + * This is a function prologue which is used to allow calls to the
>>> current
>>> + * function to be redirected if a KUnit is running. If the KUnit is not
>>> + * running or stub is not yet activated the function will continue
>>> execution
>>> + * as normal.
>>> + *
>>> + * The function stub must be declared with
>>> KUNIT_DECLARE_GLOBAL_STUB() that is
>>> + * stored in a place that is accessible from both the test code,
>>> which will
>>> + * activate this stub using kunit_activate_global_stub(), and from
>>> the function,
>>> + * where we will do this redirection using KUNIT_GLOBAL_STUB_REDIRECT().
>>> + *
>>> + * Unlike the KUNIT_STATIC_STUB_REDIRECT(), this redirection will work
>>> + * even if the caller is not in a KUnit context (like a worker thread).
>>> + *
>>> + * Example:
>>> + *
>>> + * .. code-block:: c
>>> + *
>>> + * KUNIT_DECLARE_GLOBAL_STUB(func_stub, int (*)(int n));
>>> + *
>>> + * int real_func(int n)
>>> + * {
>>> + * KUNIT_GLOBAL_STUB_REDIRECT(func_stub, n);
>>> + * return n + 1;
>>> + * }
>>> + *
>>> + * int replacement_func(int n)
>>> + * {
>>> + * return n + 100;
>>> + * }
>>> + *
>>> + * void example_test(struct kunit *test)
>>> + * {
>>> + * KUNIT_EXPECT_EQ(test, real_func(1), 2);
>>> + * kunit_activate_global_stub(test, func_stub, replacement_func);
>>> + * KUNIT_EXPECT_EQ(test, real_func(1), 101);
>>> + * }
>>> + */
>>> +#define KUNIT_GLOBAL_STUB_REDIRECT(stub, args...) do
>>> { \
>>> + if (kunit_is_running()) { \
>>> + typeof(stub) *__stub = &(stub); \
>>> + CLASS(kunit_global_stub_guard,
>>> guard)(&__stub->base); \
>>> + typeof(__stub->dummy) replacement =
>>> guard.active_replacement; \
>>> + if (unlikely(replacement)) { \
>>> + kunit_info(__stub->base.owner, "%s: redirecting to
>>> %ps\n", \
>>> + __func__, replacement); \
>>> + return replacement(args); \
>>> + } \
>>> + } \
>>> +} while (0)
>>> +
>>> +void __kunit_activate_global_stub(struct kunit *test, struct
>>> kunit_global_stub *stub,
>>> + void *replacement_addr);
>>> +
>>> +/**
>>> + * kunit_activate_global_stub() - Setup a fixed function stub.
>>
>> s/fixed/global/ here and every where else below
>
> oops
>
>>
>>> + * @test: Test case that wants to activate a fixed function stub
>>> + * @stub: The location of the function stub pointer
>>> + * @replacement: The replacement function
>>> + *
>>> + * This helper setups a function stub with the replacement function.
>>> + * It will also automatically deactivate the stub at the test end.
>>> + *
>>> + * The redirection can be disabled with kunit_deactivate_global_stub().
>>> + * The stub must be declared using KUNIT_DECLARE_GLOBAL_STUB().
>>> + */
>>> +#define kunit_activate_global_stub(test, stub, replacement) do
>>> { \
>>> + typeof(stub) *__stub = &(stub); \
>>> + typecheck_fn(typeof(__stub->dummy), (replacement)); \
>>> + __kunit_activate_global_stub((test), &__stub->base,
>>> (replacement)); \
>>> +} while (0)
>>> +
>>> +void __kunit_deactivate_global_stub(struct kunit *test, struct
>>> kunit_global_stub *stub);
>>> +
>>> +/**
>>> + * kunit_deactivate_global_stub() - Disable a fixed function stub.
>>> + * @test: Test case that wants to deactivate a fixed function stub
>>> + * @stub: The location of the function stub pointer
>>> + *
>>> + * The stub must be declared using KUNIT_DECLARE_GLOBAL_STUB().
>>> + */
>>> +#define kunit_deactivate_global_stub(test, stub) do { \
>>> + typeof(stub) *__stub = &(stub); \
>>> + __kunit_deactivate_global_stub((test), &__stub->base); \
>>> +} while (0)
>>> +
>>> #endif
>>> #endif
>>> diff --git a/lib/kunit/static_stub.c b/lib/kunit/static_stub.c
>>> index 92b2cccd5e76..799a7271dc5b 100644
>>> --- a/lib/kunit/static_stub.c
>>> +++ b/lib/kunit/static_stub.c
>>> @@ -121,3 +121,52 @@ void __kunit_activate_static_stub(struct kunit
>>> *test,
>>> }
>>> }
>>> EXPORT_SYMBOL_GPL(__kunit_activate_static_stub);
>>> +
>>> +static void sanitize_global_stub(void *data)
>>> +{
>>> + struct kunit *test = kunit_get_current_test();
>>> + struct kunit_global_stub *stub = data;
>>> +
>>> + KUNIT_EXPECT_NE(test, 0, atomic_read(&stub->busy));
>>
>> shouldn't sanitize_ be unconditional and do nothing in this case?
>
> I just didn't like early return here, but maybe it's more correct
hmm, in fact we don't need to check stub->busy prior to calling
kunit_release_action() since our goal is to detect whether stub was
activated, but this action will be released/called only if we have added
this action after the stub activation, so we can just rely on the action
management code and just keep the EXPECT_NE here as a guard
>
>>
>>> + KUNIT_EXPECT_PTR_EQ(test, test, READ_ONCE(stub->owner));
>>> +
>>> + reinit_completion(&stub->idle);
>>> + if (!atomic_dec_and_test(&stub->busy)) {
>>> + kunit_info(test, "waiting for %ps\n", stub->replacement);
>>> + KUNIT_EXPECT_EQ(test, 0,
>>> wait_for_completion_interruptible(&stub->idle));
>>
>> what's preventing stub->busy going to 1 again after this?
>
> at the redirection point in kunit_global_stub_guard we have
>
> atomic_inc_not_zero(&stub->busy);
>
> and the activation/deactivation can only be done from the main KUnit
> thread (which is here)
>
>>
>> Lucas De Marchi
>>
>>> + }
>>> +
>>> + WRITE_ONCE(stub->owner, NULL);
>>> + WRITE_ONCE(stub->replacement, NULL);
>>> +}
>>> +
>>> +/*
>>> + * Helper function for kunit_activate_global_stub(). The macro does
>>> + * typechecking, so use it instead.
>>> + */
>>> +void __kunit_activate_global_stub(struct kunit *test,
>>> + struct kunit_global_stub *stub,
>>> + void *replacement_addr)
>>> +{
>>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, stub);
>>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, replacement_addr);
>>> + if (atomic_read(&stub->busy))
>>> + kunit_release_action(test, sanitize_global_stub, stub);
>>> + else
>>> + init_completion(&stub->idle);
>>> + WRITE_ONCE(stub->owner, test);
>>> + WRITE_ONCE(stub->replacement, replacement_addr);
>>> + KUNIT_ASSERT_EQ(test, 1, atomic_inc_return(&stub->busy));
>>> + KUNIT_ASSERT_EQ(test, 0, kunit_add_action_or_reset(test,
>>> sanitize_global_stub, stub));
>>> +}
>>> +EXPORT_SYMBOL_GPL(__kunit_activate_global_stub);
>>> +
>>> +/*
>>> + * Helper function for kunit_deactivate_global_stub(). Use it instead.
>>> + */
>>> +void __kunit_deactivate_global_stub(struct kunit *test, struct
>>> kunit_global_stub *stub)
>>> +{
>>> + if (atomic_read(&stub->busy))
>>> + kunit_release_action(test, sanitize_global_stub, stub);
>>> +}
>>> +EXPORT_SYMBOL_GPL(__kunit_deactivate_global_stub);
>>> --
>>> 2.43.0
>>>
